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

Add option to emit rerun-if-env-changed metadata #701

Merged
merged 3 commits into from
Oct 29, 2022

Conversation

nathaniel-daniel
Copy link
Contributor

Partially fixes #230 by emitting directives for environment variables. Looking at #443, it seems uncontroversial to add an option to emit this metadata for environment variables.

This PR adds an option called emit_rerun_if_env_changed to control this behavior, defaulted to false. I added a check in getenv to print the metadata if this flag is true. Since I use the print function, cargo_metadata must also be true for this flag to have any effect.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This is mostly good, but I'd like to avoid emitting this for the same var multiple times.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This looks better. In the future it would be nice to avoid doing this for vars cargo sets on its own, but I don't know how much that matters.

@thomcc thomcc merged commit 0b25dbf into rust-lang:main Oct 29, 2022
@thomcc
Copy link
Member

thomcc commented Oct 29, 2022

Thinking more, I'm really unsure why we wouldn't default this to true.

@nathaniel-daniel
Copy link
Contributor Author

Thanks for the merge/fixes.

This looks better. In the future it would be nice to avoid doing this for vars cargo sets on its own, but I don't know how much that matters.

I figured it didn't matter too much, and wasn't sure how much overhead checking a list would be.

Thinking more, I'm really unsure why we wouldn't default this to true.

I was going off of #443 (comment) for this PR. I guess its conceivable that a build script may not support being rerun, so emitting rerun metadata might break those since I think cargo metadata is emitted by default. However, I couldn't find the exact rationale behind defaulting to false.

I wanted to create an uncontroversial PR to improve the situation around rerunning, since progress there seemed to have stalled for half a decade and I run into this issue a lot.

Also, is #230 considered fixed by this? Is there nothing that can be done for files aside from expecting the user to emit metadata themselves?

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.

Use rerun-if features to detect when code needs rebuilding
2 participants