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

Default emit_rerun_if_env_changed to true #738

Merged
merged 2 commits into from Oct 29, 2022

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Oct 29, 2022

As mentioned in #701 (comment), I don't think there's a reason to default this to false. rerun-if-env-changed is purely additive (unlike rerun-if-changed, which has a fairly good default cleared if you emit it at all), so it should never hurt.


I've also added code to skip emitting it for vars that provided by cargo, otherwise every use of cc will end up with cargo:rerun-if-env-changed invocations for pointless things like DEBUG, OPT_LEVEL, and so on. I don't think these are likely to hurt anything, but they definitely don't help, and cargo's docs say not to.

An alternative to this would be to add a different function for vars provided by cargo, but that sounds like a headache to maintain, as reviewers could easily not notice the wrong function getting called for a given variable.


I'd like to get this in before the release, but mostly for the change of the default (since changing this after the release would be breaking) rather than skipping the cargo vars (which is not actually necessary).

@thomcc thomcc requested a review from JohnTitor October 29, 2022 07:59
@thomcc thomcc mentioned this pull request Oct 29, 2022
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, makes sense to me 👍

@thomcc thomcc merged commit f4ce3f6 into rust-lang:main Oct 29, 2022
@thomcc thomcc deleted the rerun-env-default-on branch October 29, 2022 11:38
@briansmith
Copy link

I think this is a breaking change. In my CI, I was (perhaps wrongly) setting a bunch of environment variables when doing cargo test but then not setting them when I ran cargo test --doc immediately after. Previously this worked fine because it didn't re-run my build script (and so didn't re-run anything using cc-rs) to run the doctests but afterwords my CI broke. I have fixed it on my end but I want to notify you and others that this is breaking for some as I think I that might be a surprise.

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.

None yet

3 participants