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

Provide cfg option to disable use of cc. #1284

Closed
wants to merge 3 commits into from

Conversation

adetaylor
Copy link
Collaborator

In the Chromium build system, we automatically convert Cargo.toml rules into rules for our build system, gn. We also run build.rs files as part of the Chromium build (because it turns out that the vast majority of them just set some feature flags for feature detection).

The build.rs for cxx does more sophisticated things - including building C++ code using cc. That's appropriate in a cargo context, but presents difficulty for our build system where C++ must be built using static build rules beyond the realm of cargo. At the moment we carry a patch for this, but it would be great not to have to do so.

This patch would enable us to use the Cargo.toml and build.rs for cxx without change. It's only one out of four patches we currently carry for cxx, but it would be great to get rid of it! Obviously this may seem like a bit of a niche use-case, but hopefully this helps anyone else who builds cxx using non-Cargo build systems, but reusing the Cargo.toml and build.rs.

The intention is that this change is a no-op except for folks who disable default features.

In the Chromium build system, we automatically convert Cargo.toml rules into
rules for our build system, gn. We also run build.rs files as part of the
Chromium build (because it turns out that the vast majority of them just set
some feature flags for feature detection).

The build.rs for cxx does more sophisticated things - including building C++
code using cc. That's appropriate in a cargo context, but presents difficulty
for our build system where C++ must be built using static build rules beyond the
realm of cargo. At the moment we carry a patch for this, but it would be great
not to have to do so.

This patch would enable us to use the Cargo.toml and build.rs for cxx without
change. It's only one out of four patches we currently carry for cxx, but it
would be great to get rid of it! Obviously this may seem like a bit of a niche
use-case, but hopefully this helps anyone else who builds cxx using non-Cargo
build systems, but reusing the Cargo.toml and build.rs.
These seem to require an explicit dependency.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

There are at least 3 alternatives that I hope could be pursued before this approach.

  1. Cargo already has a built-in way to disable the build script of a dependency. https://doc.rust-lang.org/1.73.0/cargo/reference/build-scripts.html#overriding-build-scripts. Can gnrt handle it similarly? (FWIW in Reindeer for buck2 we default to not running third-party build scripts. We opt-in individual crate after a human reviews that their build script does something sensible in a buck world. We have found this to be a better default.)

  2. If gn can set CXX=true AR=true during the build script execution, those environment variables are picked up by the cc crate and C++ code will not get compiled.

  3. If neither of the above are feasible, it still seems wrong to handle your situation through cxx's build script specifically. Your desire to not compile C++ code from a build script isn't specific to the cxx crate. If some other dependency were also compiling native code, you'd need to negotiate an equivalent PR into each one. It seems like you'd be better off PR-ing the cc crate. (But per 2, adding a Cargo feature to cc is not needed if CXX=true AR=true already accomplishes the desired outcome.)

@adetaylor
Copy link
Collaborator Author

Thanks. Option 2 sounds especially encouraging, I will take a look. I'll close this for now.

@adetaylor adetaylor closed this Nov 9, 2023
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.

None yet

2 participants