-
Notifications
You must be signed in to change notification settings - Fork 442
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
Allow disabling cc
's ability to run commands via the CC_FORCE_DISABLE
environment variable
#1226
base: main
Are you sure you want to change the base?
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate that this is toplevel and not on Build
, but changing everywhere that produces Command
s to take a &Build
was far too invasive a change. This already is pushing it.
…BLE` environment variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding checks to Build::compile_objects
?
It is the function responsible for doing the actual compilation, all other functions compile/try_compile/compile_intermediates/try_compile_intermediates all delegate to it.
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. |
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.rs
is 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::Err
would 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.