| Message ID | 20250227064410.49465-1-jst@mbs-solutions.de |
|---|---|
| State | New |
| Headers | show |
| Series | cargo: remove LD_LIBRARY_PATH wrapper | expand |
On Tue, 2025-01-21 at 09:12 +0100, Jan Strater-Büddefeld via lists.openembedded.org wrote:
Fixes [YOCTO #15579]
This commit removes the LD_LIBRARY_PATH wrapper around `cargo`.
Setting the LD_LIBRARY_PATH causes many problems.
Some build scripts will not run because the build scripts execute
binaries from the host system that are not compatible with the target
libraries.
Even a simple `cargo help build` can fail because it uses `less`
internally, which should not be linked against the target libraries.
There might be cases where the LD_LIBRARY_PATH is needed for some hosts,
like the case described in 388e7cac9f90e79ce8c3c1683d8ee0f4df1bc907
but one can always set LD_LIBRARY_PATH manually if needed.
A current working workaround without this commit would be always using
`cargo.real` instead of `cargo`, which can lead to confusion.
Signed-off-by: Jan Strater-Büddefeld <jst@mbs-solutions.de><mailto:jst@mbs-solutions.de>
---
meta/recipes-devtools/rust/cargo_1.81.0.bb | 8 --------
1 file changed, 8 deletions(-)
That wrapper was added for a reason. Is that reason still valid and how
do you plan to mitigate against the issues it was originally added for?
The commit message needs to mention this.
Cheers,
Richard
I have rebased the commit and reworded the commit message. Is the new patch better? Let me know, what else I can do. This is my first contribution to an open source project, so I'm not familiar with the process yet.
Kindly regards,
Jan Strater-Büddefeld
Hi, On Thu, 2025-02-27 at 06:48 +0000, Jan Strater-Büddefeld wrote: > > > On Tue, 2025-01-21 at 09:12 +0100, Jan Strater-Büddefeld via > > > lists.openembedded.org wrote: > > > > Fixes [YOCTO #15579] > > > > > > > > This commit removes the LD_LIBRARY_PATH wrapper around `cargo`. > > > > Setting the LD_LIBRARY_PATH causes many problems. > > > > Some build scripts will not run because the build scripts > > > > execute > > > > binaries from the host system that are not compatible with the > > > > target > > > > libraries. > > > > Even a simple `cargo help build` can fail because it uses > > > > `less` > > > > internally, which should not be linked against the target > > > > libraries. > > > > > > > > There might be cases where the LD_LIBRARY_PATH is needed for > > > > some hosts, > > > > like the case described in > > > > 388e7cac9f90e79ce8c3c1683d8ee0f4df1bc907 > > > > but one can always set LD_LIBRARY_PATH manually if needed. > > > > > > > > A current working workaround without this commit would be > > > > always using > > > > `cargo.real` instead of `cargo`, which can lead to confusion. > > > > > > > > Signed-off-by: Jan Strater-Büddefeld <jst@mbs-solutions.de> > > > > --- > > > > meta/recipes-devtools/rust/cargo_1.81.0.bb | 8 -------- > > > > 1 file changed, 8 deletions(-) > > > That wrapper was added for a reason. Is that reason still valid > > > and how > > > do you plan to mitigate against the issues it was originally > > > added for? > > > The commit message needs to mention this. > > > > > > Cheers, > > > > > > Richard > I have rebased the commit and reworded the commit message. Is the new > patch better? Let me know, what else I can do. This is my first > contribution to an open source project, so I'm not familiar with the > process yet. The commit message is much better, yes, thanks. This is a really tricky problem though, particularly for your first contribution. My concern in merging something like this is that it effectively fixes one use case at the expense of breaking others. Ideally we'd find a different fix which fixes both your issue and the one that was originally being fixed. I appreciate that is very hard to do though. I'm not sure what to recommend. I have vague memories of the original issue but not enough to be able to give useful pointers. Whilst we probably don't test that tumbleweed combination now, I would be worried that some other distro will replicate that type of combination of host/uninative/nativesdk libraries and see the issue again. Suggesting the user set LD_LIBRARY_PATH manually when needed isn't too helpful as we don't know when the user would need to do this or how they'd then add this. One idea, could we add base_libdir to the code in cargo that is adding libdir? Cheers, Richard
diff --git a/meta/recipes-devtools/rust/cargo_1.82.0.bb b/meta/recipes-devtools/rust/cargo_1.82.0.bb index db18ecfda9..05ecc32ae6 100644 --- a/meta/recipes-devtools/rust/cargo_1.82.0.bb +++ b/meta/recipes-devtools/rust/cargo_1.82.0.bb @@ -45,12 +45,6 @@ do_install () { } do_install:append:class-nativesdk() { - # To quote the cargo docs, "Cargo also sets the dynamic library path when compiling - # and running binaries with commands like `cargo run` and `cargo test`". Sadly it - # sets to libdir but not base_libdir leading to symbol mismatches depending on the - # host OS. Fully set LD_LIBRARY_PATH to contain both to avoid this. - create_wrapper ${D}/${bindir}/cargo LD_LIBRARY_PATH=${libdir}:${base_libdir} - ENV_SETUP_DIR=${D}${base_prefix}/environment-setup.d mkdir "${ENV_SETUP_DIR}" CARGO_ENV_SETUP_SH="${ENV_SETUP_DIR}/cargo.sh"
Fixes [YOCTO #15579] Fixes [YOCTO #15735] This commit removes the LD_LIBRARY_PATH wrapper around 'cargo'. Setting the LD_LIBRARY_PATH causes many problems. Some build scripts will not run because the build scripts execute binaries from the host system that are not compatible with the target libraries. Even a simple 'cargo help build' will probably fail because it uses 'less' internally, which should not be linked against the target libraries. This will break some commands like 'cargo test' and 'cargo run' for some hosts that have the target libraries located in base_libdir, see commit 388e7cac9f90e79ce8c3c1683d8ee0f4df1bc907. Originally, this wrapper was added for the host system tumbleweed-ty-3. I don't know if 'cargo' on the current version of this host still fails without the wrapper. If so, one would have to set the LD_LIBRARY_PATH manually. This tradeoff is acceptable because the wrapper causes more problems than it solves. A current working workaround without this commit would be to always use 'cargo.real' instead of 'cargo', which can lead to confusion. I was not able to use testsdk because it already failed on master. Signed-off-by: Jan Strater-Büddefeld <jst@mbs-solutions.de> --- meta/recipes-devtools/rust/cargo_1.82.0.bb | 6 ------ 1 file changed, 6 deletions(-)