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

Fix is_flag_supported not respecting emit_rerun_if_env_changed (#1147) #1148

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

oconnor663
Copy link
Contributor

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
Copy link

hintron commented Jul 12, 2024

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
Copy link
Collaborator

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.

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks!

@NobodyXu NobodyXu merged commit c00b52f into rust-lang:main Jul 12, 2024
25 checks passed
@github-actions github-actions bot mentioned this pull request Jul 12, 2024
@oconnor663 oconnor663 deleted the fix_emit branch July 12, 2024 03:57
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.

3 participants