#886 Enable BRP for detecting RPATH
Opened 8 months ago by churchyard. Modified 7 months ago

It would be nice if we detect RPATH in a brp script and fail the build if we do. Of course an opt-out would still be possible.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_beware_of_rpath

Basically add the mentioned check-rpaths to %__arch_install_post or %__os_install_post.

WDYT?


I'm surprised that we don't have that yet. I'm definitely +1 to enable it..

The python3 package has a 00001-rpath.patch which removes rpath when Python builds a C extension, but only if the rpath is equal to /usr/lib64 (equal to Python LIBDIR sysconfig variable which is /usr/lib64 in practice). I proposed this change upstream, but after I talked with different people, I consider that this patch should not be made upstream, and that we should remove the downstream patch (from Fedora Python package).

Read https://bugs.python.org/issue36659 for more information.

The idea would be to first make sure that the build of a (Python) package fails if it uses rpath. Once redhat-rpm-macros https://src.fedoraproject.org/rpms/redhat-rpm-config/ ("macros" file) is updated, we will be able to remove 00001-rpath.patch from the Python package.

Also, note that:

There is a tool called check-rpaths which is included in the rpmdevtools package

Is probably no longer true:

$ rpm -qf /usr/lib/rpm/check-rpaths*
rpm-build-4.14.2.1-4.fc30.1.x86_64
rpm-build-4.14.2.1-4.fc30.1.x86_64

Metadata Update from @churchyard:
- Issue tagged with: meeting

7 months ago

We talked about this in this week meeting (https://meetbot-raw.fedoraproject.org/fedora-meeting-1/2019-05-16/fpc.2019-05-16-16.00.txt):

  • #886 Enable BRP for detecting RPATH (geppetto, 16:24:31)
  • ACTION: Enable BRP for detecting RPATH (+1:6, 0:0, -1:0) (geppetto,
    16:39:17)

Metadata Update from @james:
- Issue untagged with: meeting
- Issue tagged with: writeup

7 months ago

So this now needs two things:

  • A rewrite of the "Beware of RPATH" section (which was needed even before this).

  • A special announcement about what this will break and providing a way to work around it.

  • Adding a definition of %__brp_check_rpaths, along with adding %{?__brp_check_rpaths} to the definition of %__os_install_post in the redhat-rpm-config package.

Concerns I have:

  • How many potential failures there could be. Do we have any idea? There are more than a few packages which trip rpmlint's binary-or-shlib-defines-rpath complaint, and my understanding is that these will indeed cause check-rpaths to fail (with "contains a standard rpath").

  • Pushback if the failure count is high.

  • check-rpath failures will be oddly verbose and contain information about setting a magic environment variable QA_RPATHS. This information is actually correct as far as I know, though it doesn't tell you that you have to do that in %install. It's also a bit odd because nothing else in the system seems to work that way. But the script comes from RPM upstream so I guess that's the way it is.

I'm thinking that maybe this should go through the actual change process (with our approval on it and the guidelines changes ready to go, of course). I'm still for it but... maybe this isn't really something FPC should just do.

I'm thinking that maybe this should go through the actual change process

That has been on my mind as well. I think we should do this as a Fedora change. It has all the benefits, the thing is:

  • documented
  • discussed
  • approved
  • announced
  • tracked

For the record, I ran rpmlint -c BinariesCheck on all of the x86_64 packages in the most recent rawhide compose and pulled out the "binary-or-shlib-defines-rpath" complaints. There are 365. Some of these use rpaths like $ORIGIN/. which I believe check-rpaths should allow.

A few packages, like condor, will run afoul of the check that $ORIGIN rpaths must come first (and also the issue of including the stock libary path):

condor.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/condor/accountant_log_fixer ['$ORIGIN/../lib', '/lib64', '/usr/lib64', '$ORIGIN/../lib/condor', '/usr/lib64/condor']

This one is fun (and seems to me to be rather broken):

esc.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/esc-1.1.2/lib/libcoolkeymgr-1.0.so.0.0.0 ['/usr/local/lib']

This seems pretty broken as well; shared libraries in /usr/share?

conky-manager.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/conky-manager ['/usr/share/conky-manager/libs']

These are unpleasant:

Io-language.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/io/addons/LZO/_build/dll/libIoLZO.so ['/builddir/build/BUILD/io-e64ff9/_build/dll']
jq.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/jq ['/builddir/build/BUILD/jq-1.6/.libs', '/usr/lib64']

And this is wrong on a couple of levels:

libp11-devel.x86_64: E: binary-or-shlib-defines-rpath /usr/share/doc/libp11-devel/examples/.libs/lt-auth ['/builddir/build/BUILD/libp11-0.4.10/src/.libs']

But by far the biggest thing is packages with an rpath of /usr/lib64, which I believe will be failed by check-rpaths. There are over 300 of these and include some pretty core package like acl, attr, binutils.

Doing this has left me with some questions, like whether this is OK:

lldb.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/lldb ['$ORIGIN/../lib64']

Since the binary is in /usr/bin, that is no different than an rpath of /usr/lib64. But don't believe check-rpaths will complain. And if that's OK, then... why does it complain about /usr/lib64?

If things were patched to not complain (or maybe just warn) about standard rpaths by default then I would think this would catch some pretty broken stuff while not causing too much disruption. Maybe that's a better place to start, but unfortunately the script comes from RPM upstream which means that changing it can be somewhat difficult. I think we might get by with some trickery like setting %__brp_check_rpaths to

QA_RPATHS=${QA_RPATHS-1} /usr/lib/rpm/check-rpaths

but I'd have to try it to see and I'm sure that someone would yell about the bashism.

Anyway, I guess I'll start a discussion on the devel list.

Login to comment on this ticket.

Metadata