Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo: Fix handling of relative sysroots #1371

Merged
merged 2 commits into from
Jun 13, 2022
Merged

cargo: Fix handling of relative sysroots #1371

merged 2 commits into from
Jun 13, 2022

Conversation

kalcutter
Copy link
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
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
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
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
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