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.
check-rpaths
%__arch_install_post
%__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
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):
Metadata Update from @james: - Issue untagged with: meeting - Issue tagged with: writeup
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").
binary-or-shlib-defines-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.
QA_RPATHS
%install
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:
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.
rpmlint -c BinariesCheck
$ORIGIN/.
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):
$ORIGIN
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?
/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.
/usr/lib64
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?
/usr/bin
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.
So I took a look at it, to try to assess the impact and maybe propose a system-wide change for F35 to enable rpath detection (and failure of those builds) by default.
Currently in my analysis of all the x86_64 packages, 92 fail.
Methodology used for analysis:
Repoquery all the x86_64 packages in rawhide which yielded 9514 items.
Build them in copr with this change in redhat-rpm-config:
diff --git a/macros b/macros index f062f94..b5ccb07 100644 --- a/macros +++ b/macros @@ -234,7 +234,7 @@ print(result) # Expanded at end of %install scriptlet. # -%__arch_install_post /usr/lib/rpm/check-buildroot +%__arch_install_post /usr/lib/rpm/check-rpaths /usr/lib/rpm/check-buildroot
Copr repo: https://copr.fedorainfracloud.org/coprs/cstratak/rpath/
And then analyze all the failures through a script with this regex: .+'check-rpaths' detected a broken RPATH.+
@tibbs Do you see any blockers in moving this forward? It has already been discussed and the consensus is that this should move forward, so I'd be happy to work on it. Does anyone else has any feedback?
Could you share the full list?
There must be a way to opt out. The script currently says:
* Examples: * - to ignore standard and empty RPATHs, execute 'rpmbuild' like * $ QA_RPATHS=$(( 0x0001|0x0010 )) rpmbuild my-package.src.rpm
This is impossible in mock, koji etc. It should follow standard disablement described in https://docs.fedoraproject.org/en-US/packaging-guidelines/#_brp_buildroot_policy_scripts + additionally it could allow setting a macro with the specific errors to ignore. E.g. %global %_brp_rpath_allow 0x0001|0x0010
%global %_brp_rpath_allow 0x0001|0x0010
/usr/lib/rpm/check-buildroot
/usr/lib/rpm/check-rpaths
Sure. I'll add it to a subsequent comment as to not make the comment too long.
There must be a way to opt out. The script currently says: * Examples: * - to ignore standard and empty RPATHs, execute 'rpmbuild' like * $ QA_RPATHS=$(( 0x0001|0x0010 )) rpmbuild my-package.src.rpm This is impossible in mock, koji etc. It should follow standard disablement described in https://docs.fedoraproject.org/en-US/packaging-guidelines/#_brp_buildroot_policy_scripts + additionally it could allow setting a macro with the specific errors to ignore. E.g. %global %_brp_rpath_allow 0x0001|0x0010
Indeed there should be a proper opt-out mechanism. I haven't implemented anything of the sort yet, but if it moves to a system-wide change that will be included.
I believe you accidentally disabled /usr/lib/rpm/check-buildroot by moving it to an argument of /usr/lib/rpm/check-rpaths. For the purpose of your check, it doesn't really matter, but when you do the change, please make sure both are executed.
Thanks for the heads-up!
Full list of packages:
audiofile abc binutils esc ettercap freeradius fortune-mod fcl eb conky-manager condor community-mysql czmq cfitsio compat-guile18 glib2 gnokii koffice-kivio kicad jq komparator k3guitune laszip levmar hdf gpick kdepim3 gpgme Io-language kdegames3 gupnp-dlna kdebase3 libcommuni lutok libburn libminc libisoburn liboping librfid mingw-qt5-qtdeclarative libkkc openjade libdxfrw libosip2 libeXosip2 NLopt libprelude mingw-qt5-qt3d mod_wsgi libXcm ncview libdkimpp mingw-qt5-qttools mcpp mingw-qt5-qtbase mongo-c-driver nightview openscap plotmm pam_yubico perl-SDL pinentry pam_mount python2.7 rb_libtorrent rrdtool rarian qwtpolar qucs scipy tracker SDL_image sofia-sip scap-workbench woff2 xeus yaz stp suitesparse usnic-tools sqlite2 vanessa_logger xbsql tracker-miners WindowMaker xmms sylfilter verbiste zvbi xdotool texlive-base zinnia
I had completely forgotten about this effort. Thanks for picking it back up.
Honestly things don't look that bad; the package list seems smaller than it was when I looked two years ago. With the above-mentioned opt-out and tweaking options actually implemented, I don't see why the checking couldn't be enabled after the usual distro-wide coordination, assuming the RPM folks don't have any objections. It would, however, be good to make sure that we broadly understand what these packages are doing wrong, and what they should be doing to fix the issue instead of just disabling the check.
Once all of that is done, I suppose the actual changes to the guidelines would be pretty simple.
On a slightly related note, I see that the call to /usr/lib/rpm/check-buildroot in %__arch_install_post is also not conditionalized. It's kind of odd; I don't really understand the distinction between %__os_install_post and %__arch_install_post given that the latter only contains one thing and they are both just incorporated unconditionally into %__spec_install_post.
%__spec_install_post
Seems to me that everything (including the change to call check-rpaths) should be uniformly conditionalized and placed in %__os_install_post for consistency.
Metadata Update from @tibbs: - Issue untagged with: writeup - Issue tagged with: meeting
The change has been implemented through https://fedoraproject.org/wiki/Changes/Broken_RPATH_will_fail_rpmbuild
Packaging guidelines update: https://pagure.io/packaging-committee/pull-request/1071
Wow, congrats for your tenacity cstratak!
On a related note, maybe the guidelines will have to amended to account for the fact that the buildroot policy script was updated in RPM upstream to check and apply the same rules for RUNPATH also for RPM => 4.17, at least when RPM is updated.
https://github.com/rpm-software-management/rpm/pull/1487/files
Can this issue be closed, or was there something else that you wanted the FPC to discuss/decide? I'm moving the issue to needinfo because nothing has really happened in months, but it might be better to just close and reference it in a new issue if you have follow on questions/requests.
Metadata Update from @james: - Issue untagged with: meeting - Issue tagged with: needinfo
The BRP was enabled, but the guidelines wrt it are outdated.
Log in to comment on this ticket.