Skip to content

cargo: Fix handling of relative sysroots#1371

Merged
illicitonion merged 2 commits intobazelbuild:mainfrom
kalcutter:cargo-fix-relative-sysroots
Jun 13, 2022
Merged

cargo: Fix handling of relative sysroots#1371
illicitonion merged 2 commits intobazelbuild:mainfrom
kalcutter:cargo-fix-relative-sysroots

Conversation

@kalcutter
Copy link
Copy Markdown
Contributor

Currently, cc_toolchain.sysroot is appended as an argument to "CC" and
"CXX". This argument, however, will typically be overridden by any
--sysroot flag already present in "CFLAGS" and "CXXFLAGS". When sysroot
is relative, this fails because cargo_build_script_runner is not
executed from the execroot.

Do not append --sysroot to "CC" and "CXX". Instead, modify any
--sysroot arguments with relative paths to be absolute.

Currently, `cc_toolchain.sysroot` is appended as an argument to "CC" and
"CXX". This argument, however, will typically be overridden by any
--sysroot flag already present in "CFLAGS" and "CXXFLAGS". When sysroot
is relative, this fails because cargo_build_script_runner is not
executed from the execroot.

Do not append `--sysroot` to "CC" and "CXX". Instead, modify any
--sysroot arguments with relative paths to be absolute.
@UebelAndre UebelAndre requested a review from illicitonion June 9, 2022 13:06
@illicitonion
Copy link
Copy Markdown
Collaborator

Thanks for the contribution! This definitely looks nicer, and I think should be fine because we now properly populate CFLAGS and CXXFLAGS (as of #1081), which should contain a --sysroot= flag if it needs to be set, but I'm also pretty sure we have 0 test coverage for needing a custom sysroot...

@sayrer do you still have a handy repro environment from #659 to make sure it's now safe to effectively revert #664, which this PR does?

@kalcutter
Copy link
Copy Markdown
Contributor Author

@illicitonion I would have included a test but I didn't see an easy way. Would you be OK to merge this without a test for 0.6.0 since it fixes real bugs? We can create an issue to improve the testing of cross-compilation?

Copy link
Copy Markdown
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - let's 🤞 and run with it :) Thanks again!

@illicitonion illicitonion merged commit 7adf721 into bazelbuild:main Jun 13, 2022
@kalcutter
Copy link
Copy Markdown
Contributor Author

Thanks! 🎉

@kalcutter kalcutter deleted the cargo-fix-relative-sysroots branch June 13, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants