Allow disabling cc's ability to run commands via the CC_FORCE_DISABLE environment variable
#1226
+169
−73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thomcc
commented
Oct 2, 2024
| @@ -1,5 +1,7 @@ | |||
| disallowed-methods = [ | |||
| { path = "std::env::var_os", reason = "Please use Build::getenv" }, | |||
| { path = "std::env::var", reason = "Please use Build::getenv" }, | |||
| { path = "std::process::Command::new", reason = "Please use crate::command_new()" }, | |||
| { path = "cc::tool::Tool::to_command", reason = "Bypasses `is_disabled()` check. Use try_to_command() instead." }, | |||
There was a problem hiding this comment.
This is just for internals, obviously users can continue to use Tool::to_command as much as they want.
| @@ -213,15 +214,15 @@ impl Tool { | |||
| Some(fname) if fname.contains("clang") => match clang_driver { | |||
| Some("cl") => ToolFamily::Msvc { clang_cl: true }, | |||
| _ => ToolFamily::Clang { | |||
| zig_cc: is_zig_cc(&path, cargo_output), | |||
| zig_cc: is_zig_cc(&path, cargo_output).unwrap_or(false), | |||
There was a problem hiding this comment.
unwrap_or(false) here is a bit janky. I don't think it makes much of a difference what we report when cc is disabled, though.
| @@ -4220,7 +4244,60 @@ fn android_clang_compiler_uses_target_arg_internally(clang_path: &Path) -> bool | |||
| false | |||
| } | |||
|
|
|||
| fn autodetect_android_compiler(target: &str, gnu: &str, clang: &str) -> String { | |||
| /// Returns true if `cc` has been disabled by `CC_FORCE_DISABLE`. | |||
| pub(crate) fn is_disabled() -> bool { | |||
There was a problem hiding this comment.
It's unfortunate that this is toplevel and not on Build, but changing everywhere that produces Commands to take a &Build was far too invasive a change. This already is pushing it.
…BLE` environment variable
NobodyXu
requested changes
Oct 3, 2024
|
Tbh this does seem a more general problem with the architecture of Cargo's build scripts rather than cc per se. But if using cc as a proxy helps as a mitigation then I'm not against it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This came up in RustConf as a desire from users/maintainers of 3rd-party build systems, such as Buck2, Bazel, and their ilk.
The idea is that they should be in full control over building all native dependencies, and currently it's (apparently) very annoying and time-consuming to ensure that no
build.rsis compiling/assembling/linking/etc native libraries on it's own. By setting this flag, they can get an error on any crate that attempts this.In terms of the implementation, this was more invasive than I'd like, and I'm open to suggestions about alternative ways of doing this. I failed to locate a single choke-point where returning a
Result::Errwould sufficiently neuter our functionality.I think the approach I've taken is awkward, and would be borderline-unmaintainable were it not for
clippy::disallowed_methods. That said, we do have access to that clippy lint, which I think pushes this over the edge.