rust-lang / cc-rs

Rust library for build scripts to compile C/C++ code into a Rust library
https://docs.rs/cc
Apache License 2.0
1.79k stars 434 forks source link

Fix is_flag_supported not respecting emit_rerun_if_env_changed (#1147) #1148

Closed oconnor663 closed 1 month ago

oconnor663 commented 1 month ago

When I add a Cargo override to the example from #1147:

[patch.crates-io]
cc = { path = "/tmp/cc-rs" }

Then the extra rerun-if-env-changed directives go away (except for one that's maybe a separate small bug):

$ touch build.rs && cargo build -vvv 2>&1 | grep rerun-if-env-changed
[scratch 0.1.0] cargo:rerun-if-env-changed=CC_ENABLE_DEBUG_OUTPUT
[scratch 0.1.0] cargo:rerun-if-env-changed=CC_ENABLE_DEBUG_OUTPUT

I'm not sure how to add a unit test for this, since it doesn't look like emit_rerun_if_env_changed is currently tested.

It also seems like it's pretty easy to write more bugs like this, since anytime a feature is added to Build we need to remember to come to these lines and think about whether it should be inherited. I'm not sure how to make that better though. Thoughts?

hintron commented 1 month ago

It also seems like it's pretty easy to write more bugs like this, since anytime a feature is added to Build we need to remember to come to these lines and think about whether it should be inherited. I'm not sure how to make that better though. Thoughts?

Is there some way to do a self.clone() instead of Build::new(), so all the fields are inherited and only the fields that deviate need to be explicitly changed?

NobodyXu commented 1 month ago

Is there some way to do a self.clone() instead of Build::new(), so all the fields are inherited and only the fields that deviate need to be explicitly changed?

That will end up with too many configurations inherited, better start afresh and assign what's needed.