Skip to content

Fix is_flag_supported not respecting emit_rerun_if_env_changed (#1147)#1148

Merged
NobodyXu merged 1 commit intorust-lang:mainfrom
oconnor663:fix_emit
Jul 12, 2024
Merged

Fix is_flag_supported not respecting emit_rerun_if_env_changed (#1147)#1148
NobodyXu merged 1 commit intorust-lang:mainfrom
oconnor663:fix_emit

Conversation

@oconnor663
Copy link
Copy Markdown
Contributor

@oconnor663 oconnor663 commented Jul 12, 2024

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

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?

@hintron hintron mentioned this pull request Jul 12, 2024
Closed
@NobodyXu
Copy link
Copy Markdown
Contributor

NobodyXu commented Jul 12, 2024

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.

NobodyXu
NobodyXu approved these changes Jul 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
@github-actions github-actions bot mentioned this pull request Jul 12, 2024
Merged
@oconnor663 oconnor663 deleted the fix_emit branch July 12, 2024 03:57
@oconnor663 oconnor663 mentioned this pull request Jul 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NobodyXu NobodyXu NobodyXu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@oconnor663 @hintron @NobodyXu