Spec URL: https://buckaroogeek.fedorapeople.org/reviews/rust-pathrs.spec SRPM URL: https://buckaroogeek.fedorapeople.org/reviews/rust-pathrs-0.2.4-1.fc45.src.rpm Description: "libpathrs provides a series of primitives for Linux programs to safely handle path operations inside an untrusted directory tree." Fedora Account System Username: buckaroogeek
Copr build: https://copr.fedorainfracloud.org/coprs/build/10244657 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2449216-rust-pathrs/fedora-rawhide-x86_64/10244657-rust-pathrs/fedora-review/review.txt Found issues: - No gcc, gcc-c++ or clang found in BuildRequires Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Taking this review ... will have some initial feedback soon.
Spec URL: https://buckaroogeek.fedorapeople.org/reviews/rust-pathrs.spec SRPM URL: https://buckaroogeek.fedorapeople.org/reviews/rust-pathrs-0.2.4-1.fc45.src.rpm refreshed
Created attachment 2134407 [details] The .spec file difference from Copr build 10244657 to 10249971
Copr build: https://copr.fedorainfracloud.org/coprs/build/10249971 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2449216-rust-pathrs/fedora-rawhide-x86_64/10249971-rust-pathrs/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Sorry for the delay. First review pass included below. In general, I would strongly recommend to make as little modifications that cannot be automated by rust2rpm settings (i.e. a rust2rpm.toml config file) as possible. You need to regenerate the .spec file for every new version, so any manual modifications you make are just additional (and usually error-prone) work on every version update. It also looks as if you modified the generates spec file a lot instead of teaching rust2rpm how to generate the correct spec file for this type of package. I would recommend to patch the Cargo.toml [[lib]] target to have both "rlib" and "cdylib" crate-type, which will make the results of running rust2rpm much closer to what you need. I think all things you manually did (except for the two small Cargo.toml metadata changes) could be persisted across rust2rpm regenerations by using a rust2rpm.toml config file. Let me know if you want help with that, it should make long-term maintenance of this package much easier. Other comments: 1. You're disabling the debuginfo packages ("%global debug_package %{nil}"). This is wrong for packages that ship ELF executables and libraries, please remove this statement. rust2rpm should not generate it for you in the first place. If you need to *add* it manually to make the package build, then something else is wrong (likely something setting wrong compiler flags). 2. Defining "so_ver 0.2" isn't really necessary. The Rust library interface and the shared library appear to follow the same compatibility guarantees (i.e. SemVer). So any "breaking" update for the Rust crate is already automatically a "breaking" update for the shared library. 3. Do not include the License texts separately and manually. They are already included in the published ".crate" archives. Additionally, you used the wrong URLs. They point at the GitHub HTML UI for those files, not the raw / plain-text file contents themselves. 4. There's missing documentation for the Cargo.toml metadata patch you apply. Looks like this is to update a dependency to the version that is available in Fedora. If this is an update that is as easy as it looks here, then you should submit this upstream (or, if a PR or upstream commit already exists, add a link to that to the spec file). Keeping patches like this downstream is just accumulating tech debt. :) 5. What are the BuildRequires for lld, clang, and fdupes for? They all appear to be unused, please remove them. 6. The License tag for the libpathrs subpackage is incomplete. You copied only "MIT OR Apache-2.0" into the spec file (but didn't add that to the License tag). The full "%cargo_license_summary" output is actually: ### BEGIN LICENSE SUMMARY ### # Apache-2.0 # Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT # BSD-2-Clause OR Apache-2.0 OR MIT # MIT OR Apache-2.0 # MPL-2.0 OR LGPL-3.0-or-later # Unlicense OR MIT # Zlib OR Apache-2.0 OR MIT ### END LICENSE SUMMARY ### All these need to be represented in the License tag of the "libpathrs" subpackage. 7. Why did you add "Requires: glibc-devel" to the libpathrs-devel subpackage? I don't think this is necessary. 8. There is an unowned directory in the -devel subpackage: "%{_includedir}/pathrs//pathrs.h" has a "//" but still doesn't own the containing "%dir %{_includedir}/pathrs". 9. The way you run tests is broken due to what appears to be a copy-paste mistake. You appear to have copy-pasted an em-dash as a %cargo_test argument instead of two hyphens "--". This makes %cargo_test treat is as a test name filter, and since no test name contains an em-dash, no tests are run at all. In general, I would recommend to also run %cargo_test with the "-a" flag, since you already pass that to all other macros. Otherwise you will *build* and ship features of the library that are not turned on when building and running tests.
Oh, and 10: You appear to have manually removed the rust-%{crate}+%feature-devel subpackages and the %cargo_install call. If you want to *intentionally* not provide the source code for the Rust library interface, then use the "cargo-install-lib = false" setting under "[package]" in rust2rpm.toml, instead of making such large manual modifications to the generated spec file.
Many thanks for the detailed notes and comments. I will review and respond after the weekend. A lot ot learn.
Apologies for the slow response. The irrigation district started delivery a week earlier than planned so I needed to get some farming done ASAP. My short term goal is to have libpathrs and libpathrs-devel available. libpathrs will be required for runc v1.5.x (with an opt out path available). Runc v1.5.0-rc.2 was released yesterday. I will then update the package with a subpackage for the python bindings. The go bindings are already consumable via the vendoring that golang rpms are using. My initial use of rust2rpm produced the following subpackages: devel, %{name}+default-devel, %{name}+_test_as_root-devel, %{name}+_test_race-devel, and %{name}+capi-devel. I did indeed manually remove this to focus on libpathrs and libpathrs-devel. I will try the "cargo-install-lib = false" setting instead. And can remove this for a subsequent release. And update to follow shortly. Many thanks
> My initial use of rust2rpm produced the following subpackages: devel, %{name}+default-devel, %{name}+_test_as_root-devel, %{name}+_test_race-devel, and %{name}+capi-devel. Yes, those would be expected. They would be needed if we ever want to package software that uses pathrs as a Rust dependency directly. > I did indeed manually remove this to focus on libpathrs and libpathrs-devel. I will try the "cargo-install-lib = false" setting instead. And can remove this for a subsequent release. Yup, using this setting should be a cleaner solution, and require less manual work on update. Let me know if you need any help or if the rust2rpm / rust2rpm.toml manual pages aren't clear / comprehensive enough to figure out what to do.
[fedora-review-service-build] All comments were very helpful. I learned a lot while working through these. Thank you for taking the time. I updated the spec and src rpm at the links above. I also uploaded a current version to COPR at: https://copr.fedorainfracloud.org/coprs/buckaroogeek/docker-stack_pre-release/packages/ I have not written a rust2rpm.toml file that captures the necessary features of the spec file for regeneration but will do so in a forthcoming update. 1. Corrected. 2. Removed. 3. Removed. 4. Added a comment. I updateed open-enum to the version available in Fedora after checking the changes made. I have not checked with upstream however. 5. Removed. I was looking at the OpenSUSE spec file for libpathrs. The author seems to work for or is assosciated with OpenSUSE. 6. Updated to include %cargo_license_summary output. 7. Dropped 8. Corrected ownership 9. Tests updates 10. I have not yet implemented a rust2rpm.toml but will do so once I get this underway.
[fedora-review-service-build]
Copr build: https://copr.fedorainfracloud.org/coprs/build/10309794 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2449216-rust-pathrs/fedora-rawhide-x86_64/10309794-rust-pathrs/fedora-review/review.txt Found issues: - No gcc, gcc-c++ or clang found in BuildRequires Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Hi Fabio - Would you have time to check the recent update? As noted, I do not have a tested rust2rpm.toml file but that can be added after the initial release. I can also build the forthcoming v1.5 runc without libpathrs support if needed.
Sorry for the delay, I've had too much on my plate recently. I think using a rust2rpm.toml config file *now* will make your package maintenance life easier sooner ... so I made one, based on your current spec file (plus fixes for issues 2. and 3. below): https://decathorpe.fedorapeople.org/libpathrs/rust2rpm.toml === There's some remaining issues: 1. License tag is not correct. The License tag for the source package should just be the license of the sources, i.e. what rust2rpm generates for you ("MPL-2.0 OR LGPL-3.0-or-later"). The Licenses of statically linked library dependencies only influence the License tag of the "libpathrs" subpackage, nothing else. From the build log: ### BEGIN LICENSE SUMMARY ### # Apache-2.0 # Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT # BSD-2-Clause OR Apache-2.0 OR MIT # MIT OR Apache-2.0 # MPL-2.0 OR LGPL-3.0-or-later # Unlicense OR MIT # Zlib OR Apache-2.0 OR MIT ### END LICENSE SUMMARY ### The License tag you added does not contain the string "OR", so it can't be correct. It should look something like this: # Apache-2.0 # Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT # BSD-2-Clause OR Apache-2.0 OR MIT # MIT OR Apache-2.0 # MPL-2.0 OR LGPL-3.0-or-later # Unlicense OR MIT # Zlib OR Apache-2.0 OR MIT License: %{shrink: (MPL-2.0 OR LGPL-3.0-or-later) AND Apache-2.0 AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Apache-2.0) AND (Unlicense OR MIT) AND (Zlib OR Apache-2.0 OR MIT) } 2. You used the "wrong" escaping / syntax for skipping tests. It looks like you intended to filter out *four* specific tests. Instead this happened: > test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 7044 filtered out; finished in 0.00s Just set up which tests to skip in rust2rpm.toml, it will use the correct syntax for you. And if you do, the result will be this instead: > test result: FAILED. 375 passed; 4421 failed; 6 ignored; 0 measured; 2242 filtered out; finished in 7.74s So ... tests fail quite spectacularly. What the tests are attempting to do also looks quite suspicious (running chown?), so I'm not sure that this *can* be fixed without talking to upstream. 3. The C library .so file version is globbed in the %files list, this is not allowed. I fixed this in the linked rust2rpm.toml file. 4. The %{_includedir}/pathrs folder is unowned. Also fixed in the linked rust2rpm.toml file. === With the linked rust2rpm.toml file (and a small Cargo.toml patch to make C library package generation actually work as intended), the only thing you need to adjust manually after running is to copy the License summary and adjust the License tag in the libpathrs subpackage, nothing else.
Just chiming in to say that I'm the author of libpathrs and one of the upstream runc maintainers. Please let me know if there is anything I can do to help with packaging -- I packaged this for openSUSE but never got around to figuring out how Rust packaging on Fedora would work with this kind of crate, so I'm really quite grateful someone else has done this for me.
> What the tests are attempting to do also looks quite suspicious (running chown?), so I'm not sure that this *can* be fixed without talking to upstream. All of those tests should be gated behind a _test_as_root feature, if you run the tests without that feature they should pass without privileges (this is what we do in CI and in the openSUSE specfile). Unfortunately, the lack of ergonomic test skipping in Rust means this needs to be set a compile-time rather than runtime.
> All of those tests should be gated behind a _test_as_root feature Oh, thanks, that's good to know. Looks like we need to drop the "-a" / "--all-features" flag from cargo test then.
(In reply to Fabio Valentini from comment #18) > > All of those tests should be gated behind a _test_as_root feature > > Oh, thanks, that's good to know. Looks like we need to drop the "-a" / > "--all-features" flag from cargo test then. Yes. The OpenSUSE spec file uses --feature capi so I am using that (-f capi). In reply to cyphar from comment #16) > Just chiming in to say that I'm the author of libpathrs and one of the > upstream runc maintainers. Please let me know if there is anything I can do > to help with packaging -- I packaged this for openSUSE but never got around > to figuring out how Rust packaging on Fedora would work with this kind of > crate, so I'm really quite grateful someone else has done this for me. Thanks for checking in. I see that CRI-O 1.36 list go-pathrs as a dependency now as do other packages. I have been reviewing your OpenSUSE spec file. Is using clang a hard requirement?
(In reply to cyphar from comment #16) > Just chiming in to say that I'm the author of libpathrs and one of the > upstream runc maintainers. Would you care to be added as a maintainer? Assuming you have packager status in Fedora, of course.
(In reply to Brad Smith from comment #19) > In reply to cyphar from comment #16) > > Just chiming in to say that I'm the author of libpathrs and one of the > > upstream runc maintainers. Please let me know if there is anything I can do > > to help with packaging -- I packaged this for openSUSE but never got around > > to figuring out how Rust packaging on Fedora would work with this kind of > > crate, so I'm really quite grateful someone else has done this for me. > > Thanks for checking in. I see that CRI-O 1.36 list go-pathrs as a dependency > now as do other packages. I have been reviewing your OpenSUSE spec file. > > Is using clang a hard requirement? So, lld is a hard requirement for all architectures -- GNU ld doesn't like the hacks you need to do to get symbol versioning working with Rust, so lld is a requirement. On *some* architectures (aarch64, if memory serves) rustc will pass special linker flags based on what -linker= flag you have configured (to work around some arch-specific issues on a per-linker basis), and in my experience that meant you needed to use clang (-fuse-ld=lld isn't enough to stop rustc from passing bad flags to lld). It is possible that the newest Rust versions (>=1.90, which default to lld) don't need these workarounds but most of my packaging / testing work was dealing with old Rust versions. You can patch around this by removing the symbol versioning stuff (alas, I haven't made that a feature or rustcfg...) but you probably want to have symbol versioning (there will likely be some breaking API changes in the near future -- I hope to get v0.2.5 out with some critical fixes soon-ish before we ship runc 1.5 :/). (In reply to Brad Smith from comment #20) > Would you care to be added as a maintainer? Assuming you have packager > status in Fedora, of course. Sadly I'm not a Fedora packager, I have a decent amount of experience doing openSUSE packaging but you folks definitely approach things a little differently. ;) I've been messing around with some Fedora packaging stuff at $new_job and it is a bit surprising how much stuff is different.
Spec URL: https://buckaroogeek.fedorapeople.org/pathrs/rust-pathrs.spec SRPM URL: https://buckaroogeek.fedorapeople.org/pathrs/rust-pathrs-0.2.4-1.fc45.src.rpm Description: "libpathrs provides a series of primitives for Linux programs to safely handle path operations inside an untrusted directory tree." Fedora Account System Username: buckaroogeek TOML URL: https://buckaroogeek.fedorapeople.org/pathrs/rust2rpm.toml
Updated spec and src rpm, along with toml file listed above. Updated COPR: https://copr.fedorainfracloud.org/coprs/buckaroogeek/docker-stack_pre-release/package/rust-pathrs/ I narrowed down the tests that are skipped with the following notes: %{cargo_test -f capi -- -- %{shrink: --skip _global_fs_dir --skip _opath --skip utils::sysctl::tests }} The _global_fs_dir string will ignore a few additional tests not just failing. The _opath string skips significantly more tests (failing and ok). Both sets of failures are due to 'Invalid cross-device' link errors. I could not see a way to tighten these skips without adding a large number of lines. The sysctl tests triggered panics in rs code without and obvious root cause. I tried adding additional debug info but that did not help me (not familiar with rust). Thank again for the reviews.
@cyphar - If you change your mind, please let me know. Hopefully I can get this package submitted before runc update arrives!
(In reply to Brad Smith from comment #23) > I narrowed down the tests that are skipped with the following notes: > > %{cargo_test -f capi -- -- %{shrink: > --skip _global_fs_dir > --skip _opath > --skip utils::sysctl::tests > }} > > The _global_fs_dir string will ignore a few additional tests not just > failing. The _opath string skips significantly more tests (failing and ok). > Both sets of failures are due to 'Invalid cross-device' link errors. I could > not see a way to tighten these skips without adding a large number of lines. There is an issue/regression with the O_PATH resolver when running in Fedora's infrastructure (or actually, most container workloads) that I hadn't noticed before -- a read-only /proc/sys (default on most container setups) is correctly detected as an overmount (EXDEV/"Invalid cross-device link") when checking the value of /proc/sys/fs/protected_symlinks but that incorrectly causes the lookup to fail, which is the issue you're seeing. I opened an issue about it the other day[1], and I'll include a fix in 0.2.5. Sorry about that, I really should set up a CI job to run the tests in containers as well. :/ Ideally we would also permit those kinds of overmounts (i.e., ones that mount the same underlying path but with ro set) in general but implementing that properly is a little difficult on pre-6.8 kernels (with statmount it should be fairly trivial to do in a race-free way) so I might punt that to 0.2.6. > The sysctl tests triggered panics in rs code without and obvious root cause. > I tried adding additional debug info but that did not help me (not familiar > with rust). I suspect this is also an issue with `/proc/sys`, and I should adjust the tests to have an xfail/skip if /proc/sys is mounted read-only (you would expect an error in this case) but if you could leave a comment in [1] with the error trace that would be great! [1]: https://github.com/cyphar/libpathrs/issues/372
(In reply to cyphar from comment #21) > (In reply to Brad Smith from comment #19) > > > > Is using clang a hard requirement? > > So, lld is a hard requirement for all architectures -- GNU ld doesn't like > the hacks you need to do to get symbol versioning working with Rust, so lld > is a requirement. > On *some* architectures (aarch64, if memory serves) rustc will pass special > linker flags based on what -linker= flag you have configured (to work around > some arch-specific issues on a per-linker basis), and in my experience that > meant you needed to use clang (-fuse-ld=lld isn't enough to stop rustc from > passing bad flags to lld). It is possible that the newest Rust versions > (>=1.90, which default to lld) don't need these workarounds but most of my > packaging / testing work was dealing with old Rust versions. So, I'm not sure whether this will work. Rust >= 1.90 only defaults to lld upstream, and only for x86_64-unknown-linux targets AIUI - and as far as I know, this change hasn't made it to Fedora - we still use GNU ld by default everywhere. We also need to pass some special linker flags / scripts that I'm not sure lld supports (but it might?). In the latest submission, you're now also mixing and matching "-a" and "-f capi" feature flags for the %cargo_* macros - this is a bad idea, it will give you inconsistent results and potentially unnecessary recompilations. I would recommend to pass "-a" only to "%cargo_generate_buildrequires" and pass "-f capi" to all other macros. That should make sure that the settings passed to build, install, and license summary are the same. And it looks like at least some of the remaining test failures are known upstream - it would be great if you could add links to the corresponding tickets to the explanation why they're skipped.
(In reply to cyphar from comment #25) > and I'll include a fix in 0.2.5. Many thanks for reviewing and scheduling updates. I am glad that these comments are helpful. Comment referring to github issue added! (In reply to Fabio Valentini from comment #26) > I would recommend to pass "-a" only to "%cargo_generate_buildrequires" and > pass "-f capi" to all other macros. That should make sure that the settings > passed to build, install, and license summary are the same. > Is there a way to specify this in rust2rpm.toml? The have the capi feature enabled in the feature table which generated the settings you mention above (i.e. the mix of -a and -f capi). I manually updated the spec file as noted. Thank you for the helpful comments. Updated all files. Spec URL: https://buckaroogeek.fedorapeople.org/pathrs/rust-pathrs.spec SRPM URL: https://buckaroogeek.fedorapeople.org/pathrs/rust-pathrs-0.2.4-1.fc45.src.rpm Description: "libpathrs provides a series of primitives for Linux programs to safely handle path operations inside an untrusted directory tree." Fedora Account System Username: buckaroogeek TOML URL: https://buckaroogeek.fedorapeople.org/pathrs/rust2rpm.toml
> > I would recommend to pass "-a" only to "%cargo_generate_buildrequires" and > > pass "-f capi" to all other macros. That should make sure that the settings > > passed to build, install, and license summary are the same. > > > > Is there a way to specify this in rust2rpm.toml? The have the capi feature > enabled in the feature table which generated the settings you mention above > (i.e. the mix of -a and -f capi). I manually updated the spec file as noted. Sadly no. This is a bit of a special case that can't fully be expressed in rust2rpm.toml settings only. Note that there's a minor incongruency left - the "-a" flags are still present in some rust2rpm.toml script fields. --- Doing another review pass now.
(In reply to Fabio Valentini from comment #28) > > Note that there's a minor incongruency left - the "-a" flags are still > present in some rust2rpm.toml script fields. > Fixed. Should have looked in the toml file.
(In reply to Fabio Valentini from comment #26) > (In reply to cyphar from comment #21) > > (In reply to Brad Smith from comment #19) > > > > > > Is using clang a hard requirement? > > > > So, lld is a hard requirement for all architectures -- GNU ld doesn't like > > the hacks you need to do to get symbol versioning working with Rust, so lld > > is a requirement. > > On *some* architectures (aarch64, if memory serves) rustc will pass special > > linker flags based on what -linker= flag you have configured (to work around > > some arch-specific issues on a per-linker basis), and in my experience that > > meant you needed to use clang (-fuse-ld=lld isn't enough to stop rustc from > > passing bad flags to lld). It is possible that the newest Rust versions > > (>=1.90, which default to lld) don't need these workarounds but most of my > > packaging / testing work was dealing with old Rust versions. > > So, I'm not sure whether this will work. > > Rust >= 1.90 only defaults to lld upstream, and only for > x86_64-unknown-linux targets AIUI - and as far as I know, this change hasn't > made it to Fedora - we still use GNU ld by default everywhere. We also need > to pass some special linker flags / scripts that I'm not sure lld supports > (but it might?). Hmmm. Well, if you only build the static version then you can avoid the problem and use GNU ld (build.rs handles this already), but I'm guessing the Fedora packaging guidelines disallow that? I wanted to get it to work with GNU ld but after fighting with it for weeks I ended up just going with what seems like the only working option. The core issue is that Rust doesn't support symbol versioning or custom version scripts properly[1], so you need to pass a custom version script to the linker but because Rust provides its own version script to hide internal symbols and GNU ld does not support setting the --version-script flag more than once -- meaning you just get linker errors with GNU ld. I had an awfully hacky setup where I created a tmpfs bindir with ld and collect2 wrapper bash scripts that messed with the arguments to make them work with GNU ld, and while that did compile it ended up producing useless symbols that I just couldn't get working -- and I'm sure that kind of awful hack would also not be looked upon favourably in Fedora packaging either. :/ [1]: https://internals.rust-lang.org/t/support-symbol-versioning-with-export-name/23626
> Hmmm. Well, if you only build the static version then you can avoid the problem and use GNU ld (build.rs handles this already), but I'm guessing the Fedora packaging guidelines disallow that? There's a strong preference for shared libraries, but static objects are not entirely forbidden: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries Maybe somebody more familiar with the Rust toolchain in Fedora can help us out here? The package built as-is from the latest spec file doesn't seem to have versioned symbols, and going by the aforementioned backwards-incompatible changes that will happen at some point (without bumping the soname?), it sounds like that would just break then. There's only one minor thing in the latest version (the License tag is #FIXME again - just copy-paste the previous comments and value again, it was correct before the last regen with rust2rpm). Other than those two things, I think the package looks good now.
(I pinged the Rust toolchain maintainer and hopefully he'll be able to chime in here.)
(In reply to Fabio Valentini from comment #31) > > There's only one minor thing in the latest version (the License tag is > #FIXME again - just copy-paste the previous comments and value again, it was > correct before the last regen with rust2rpm). > > Other than those two things, I think the package looks good now. Thank you for sticking with the review. I will move forward with the package then and make sure the license tag is correct!
(In reply to Fabio Valentini from comment #32) > (I pinged the Rust toolchain maintainer and hopefully he'll be able to chime in here.) That's me, but I think @nickc should be better suited to help with linker issues. IMO it's also not a big problem to use lld, but it would be nice to figure out how to get GNU ld working too. For reference, the version script appears to be mostly from here: https://github.com/cyphar/libpathrs/blob/797b29eb696b48050773d5b4101759527ca0f1c6/build.rs#L69-L107
Thanks for taking a look. For what it's worth, I'm not sure why this even builds - since build.rs sets "-fuse-ld=lld", but lld shouldn't be available without BuildRequires: lld?
Oh! The build script only sets that under `LIBPATHRS_CAPI_BUILDMODE=cdylib`, which is a custom environment variable set by its Makefile -- but this spec is using more direct cargo macros. You could set that manually, but the Makefile also uses stuff like hack/with-crate-type.sh, and I don't know how important that is.
(In reply to cyphar from comment #21) > So, lld is a hard requirement for all architectures -- GNU ld doesn't like > the hacks you need to do to get symbol versioning working with Rust, so lld > is a requirement. Not that I am objecting to the use of LLD, but what "hacks" are needed for Rust support that GNU ld does not support ? Perhaps there is a way for the binutils linker to support them ...
From what I can tell, lld supports getting passed two "-Wl,--version-script=<path>" args, whereas GNU ld doesn't?
(In reply to Fabio Valentini from comment #38) Hi FAbio, > From what I can tell, lld supports getting passed two > "-Wl,--version-script=<path>" args, whereas GNU ld doesn't? Do you mean that LLD combines the two arguments or that the second one overrides the first ? Looking at the code for LD, I think that the former action (ie combining) takes place, although I would need to confirm this with a real life test to be sure. Is there a test case that demonstrates what you want to achieve ? Cheers Nick
The multiple version scripts thing is a hack I use to get around the lack of support for symbol versioning in Rust[1], so I'm not sure it's worth adding a GNU ld feature just for that hack. My impression is that it replaces the version script because the default Rust one uses anonymous version nodes but the one for libpathrs uses named ones and usually linkers don't accept both (at least GNU ld doesn't). Again, this is not ideal -- ideally Rust would natively support this and there would just be one version script generated by rustc that combines my symbol version nodes and its own internal version script data. As I outlined above, I had a different hack to use GNU ld by creating a shell script wrapper for ld and collect2 to remove the problematic --version-script argument before passing things to ld, but there were other issues with that setup that felt a bit more fundamental (basically the symbol versions were completely broken, I don't remember the exact details so I would need to try to reproduce the issues again...). One of the other concerns flagged by the Rust folks was that symbol versioning and LTO have very poor interactions (even in LLD) and so it's possible that this is just a hard problem and LLD happens to work well-enough-ish but GNU ld happens to have a few different issues. I'm not sure. [1]: https://internals.rust-lang.org/t/support-symbol-versioning-with-export-name/23626
OK, well if you need any GNU ld specific help please feel free to ask me. Or ask on the upstream mailing list. :-)
Is there a way to change from LD to LLD when using the cargo macros in a spec file? I added `-fuse-ld=lld` to LDFLAGS in the spec file. + LDFLAGS='-Wl,-z,relro -Wl,--as-needed -Wl,-z,pack-relative-relocs -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-hardened-ld-errors -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -Wl,--build-id=sha1 -specs=/usr/lib/rpm/redhat/redhat-package-notes -fuse-ld=lld' Build worked. But using readelf to check for LLD did not verify LLD (on an F44 vm): [vagrant@localhost vagrant]$ readelf --string-dump .comment /usr/lib64/libpathrs.so.0.2.4 | grep -i LLD readelf: Warning: Section '.comment' was not dumped because it does not exist
And [vagrant@localhost vagrant]$ readelf -d /usr/lib64/libpathrs.so.0.2.4 Dynamic section at offset 0x87be8 contains 29 entries: Tag Type Name/Value 0x0000000000000001 (NEEDED) Shared library: [libgcc_s.so.1] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] 0x0000000000000001 (NEEDED) Shared library: [ld-linux-x86-64.so.2] 0x000000000000000e (SONAME) Library soname: [libpathrs.so.0.2]
> I added `-fuse-ld=lld` to LDFLAGS in the spec file. I don't think this will work. Can you try adding export LIBPATHRS_CAPI_BUILDMODE=cdylib before "%cargo_cbuild" / "%cargo_build"? If all else fails, I think we need to fall back to using the upstream Makefile (and potentially patching it as needed).
1. With %build export LIBPATHRS_CAPI_BUILDMODE=cdylib %cargo_build -f capi ... Result: ... -Cforce-frame-pointers=yes -Clink-arg=-specs=/usr/lib/rpm/redhat/redhat-package-notes --cap-lints=warn -C link-arg=-Wl,-soname,libpathrs.so.0 -C link-arg=-Wl,--version-script=/tmp/libpathrs-version-script.BlObHB -C link-arg=-fuse-ld=lld --cfg cdylib` error: linking with `cc` failed: exit status: 1 and ipt=/tmp/libpathrs-version-script.BlObHB" "-fuse-ld=lld" = note: some arguments are omitted. use `--verbose` to show all linker arguments = note: collect2: fatal error: cannot find 'ld' compilation terminated. 2. Add BuildRequires: lld Success with build. 3. Given the remarks from @cyphar: >> Is using clang a hard requirement? > So, lld is a hard requirement for all architectures -- GNU ld doesn't like the hacks you need to do to get symbol versioning > working with Rust, so lld is a requirement. > On *some* architectures (aarch64, if memory serves) rustc will pass special linker flags based on what -linker= flag you have > configured (to work around some arch-specific issues on a per-linker basis), and in my experience that meant you needed to > use clang (-fuse-ld=lld isn't enough to stop rustc from passing bad flags to lld). It is possible that the newest Rust > versions (>=1.90, which default to lld) don't need these workarounds but most of my packaging / testing work was dealing > with old Rust versions. Adding %global toolchain clang ... BuildRequires: clang Also builds successfully. Thoughts?
Spec URL: https://buckaroogeek.fedorapeople.org/pathrs/rust-pathrs.spec SRPM URL: https://buckaroogeek.fedorapeople.org/pathrs/rust-pathrs-0.2.4-1.fc45.src.rpm Description: "libpathrs provides a series of primitives for Linux programs to safely handle path operations inside an untrusted directory tree." Fedora Account System Username: buckaroogeek TOML URL: https://buckaroogeek.fedorapeople.org/pathrs/rust2rpm.toml These files reflect settings in (3) above (using clang). Add comments to top of toml to capture the 3 manual changes needed after rpm2rust is executed.