-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
docs: further tweak rerun-if-env-changed docs #12489
Conversation
@@ -350,7 +350,8 @@ variables like `CC` and such, it is not possible to use this for environment | |||
variables like `TARGET` that [Cargo sets for build scripts][build-env]. The | |||
environment variables in use are those received by `cargo` invocations, not | |||
those received by the executable of the build script. | |||
|
|||
Instead cargo will automatically re-run the build script if the value of the environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort. However, Cargo internally doesn't really monitor those environment variables directly. Cargo has its own mechanism to handle those envs. See Fingerprints and Metadata. I may avoid adding this, or find a better way to doc the actual stuff being tracked by Cargo in general elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting, so its likely that for some of the environment variables that cargo sets we would need to do something hacky like write the contents to a file and then set rerun-if-changed for the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. You usually don't need to do that — Cargo tracks them for you.
If you see anything should be tracked but isn't, that could be another recompile detection issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cargo internally doesn't really monitor those environment variables directly
By this I mean Cargo doesn't track what it has set directly, but if you set something directly to the invocation, like PROFILE=something cargo build
, it'll be tracked as a plain old user-specified environment variable. There's nothing special.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the wording to be a bit more accurate from your description.
My concern is that a user may read the docs as they currently are and think: "But if I cant use rerun-if-env-changed
on variables set by cargo then I shouldnt use variables set by cargo since the cached build.rs results will go stale."
Thats why I think its important to mention that cargo handles this for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel like 9b89ff7 went way too far. It's my fault not get it clearer, but still felt the current patch complicates stuff.
Cargo will effectively re-run the build script every time one of the variables that Cargo sets changes.
After reading that statement, people may do this, observe nothing rerun and complain the doc is lying:
# User: hmm… cargo takes care of this for me.
# User: I dont need to add rerun-if-env-changed=NUM_JOBS
NUM_JOBS=1 cargo b # full build
NUM_JOBS=2 cargo b # no rebuild
Like I said earlier, “usually” you don't need to pay attention to those variables. Some of those are different, and CARGO_MANIFEST_DIR
is one of them not triggering a rebuild (see #8693). I think people should generally trust the rebuild detection in Cargo. When they find an issue, they report one like #8693.
As in #12403, you discovered rebuild too eagerly, not in reverse. Personally I don't feel like it worth putting the statement here. Maybe there is a better explanation but I cannot think of one now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, it looks like you think a larger rework is required than just tweaks to this paragraph.
I'll go ahead and close this.
Thanks for your time.
fb4db54
to
f7b214f
Compare
f7b214f
to
9b89ff7
Compare
Continuation of #12403 + #12482
I thought about this some more and I think its important we mention this as well.
Otherwise the user may think it is impossible to use the env vars set by cargo in the build.rs without incorrect behavior when those values change.
r? weihanglo