Skip to content

Forward CXX env and arguments from cargo_build_script #1004

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ikalchev
Copy link
Contributor

@ikalchev ikalchev commented Nov 2, 2021

We need to forward the C++ command line arguments and env to bin.rs
to make sure cargo_build libraries are compiled with the same options
as cc_* targets.

Fixes #1003

We need to forward the C++ command line arguments and env to ``bin.rs``
to make sure cargo_build libraries are compiled with the same options
as `cc_*` targets.

Fixes bazelbuild#1003
@google-cla google-cla bot added the cla: yes label Nov 2, 2021
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.

@ikalchev Thanks for putting this together!

@hlopko Do we have any prior art on wiring up a custom cc_toolchain in a unit analysis test, so that we could test that args get properly propagated from there?

@JasperDeSutter
Copy link

These arguments can contain paths relative to the exec root, i.e. "external/androidndk/...' when cross-compiling for android (and perhaps other targets).
The cargo build script runner doesn't resolve these without a symlink or rewriting the paths to be absolute. I think a symlink would be the best option because a heuristic for detecting paths to external seems very fragile.

@illicitonion
Copy link
Collaborator

@JasperDeSutter What kind of symlink are you suggesting?

@JasperDeSutter
Copy link

@JasperDeSutter What kind of symlink are you suggesting?

A directory symlink mapping exec_root/external to CARGO_MANIFEST_DIR/external before running the build script. I'm not sure how many bazel rules this breaks, but I hacked it into cargo_build_script_runner's bin.rs and that seems to work.
The reason we would need this is because the build script is executed from CARGO_MANIFEST_DIR instead of exec_root.

@illicitonion
Copy link
Collaborator

@JasperDeSutter What kind of symlink are you suggesting?

A directory symlink mapping exec_root/external to CARGO_MANIFEST_DIR/external before running the build script. I'm not sure how many bazel rules this breaks, but I hacked it into cargo_build_script_runner's bin.rs and that seems to work. The reason we would need this is because the build script is executed from CARGO_MANIFEST_DIR instead of exec_root.

I see! Yeah, this could work, and would I imagine cover most of the use-cases people actually hit - it's possible that toolchains can point at files in-repo rather than in a separate workspace (hidden behind external), but it's probably rare...

I had been considering doing some on-the-fly flag editing to basically for each flag, if resolving it from the exec-root it happens to be an existing file/directory, to absolutify the flag, but that feels pretty invasive. The symlink feels like a pretty reasonable middle-ground to make most things work most of the time without breaking much (and we can add a "don't create external symlink" attribute to cargo_build_script if we really need)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forward CXX env and arguments from cargo_build_script
3 participants