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

rerun-if-env-changed=CARGO_MANIFEST_DIR changing doesn't trigger a rebuild on change #8693

Open
alecmocatta opened this issue Sep 9, 2020 · 11 comments
Labels
A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables A-rebuild-detection Area: rebuild detection and fingerprinting S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@alecmocatta
Copy link

CARGO_MANIFEST_DIR changing doesn't trigger a rebuild, even if rerun-if-env-changed=CARGO_MANIFEST_DIR is specified in the build.rs.

Thus env!("CARGO_MANIFEST_DIR") and env::var("CARGO_MANIFEST_DIR").unwrap() can differ in binaries executed by cargo run/test/bench.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

@alecmocatta which version of rustc and cargo did you try this with?
I thought these bugs would be fixed by rust-lang/rust#71858, but maybe that has not reached stable yet.

@alecmocatta
Copy link
Author

Latest nightly, 2020-09-09

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

Cc @petrochenkov @ehuss

@petrochenkov
Copy link
Contributor

This is an issue (or non-issue) with cargo rather than rustc.
Environment variables known to cargo are treated differently than user-defined ones, for example #8421 seems to filter away at least some of them from dependencies and not rebuild on their change.

@alecmocatta
Copy link
Author

Yes sorry this is a potential issue with cargo, I was too hasty in my filing here. Is it possible to move the issue?

Alternately as this is a known issue as @RalfJung tracked down https://github.com/rust-lang/cargo/pull/8421/files#r485577035 feel free to close if appropriate.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

It is possible to move this to the cargo repo but I do not have the required permissions.

IMO this issue reflects a reasonable request, so I'd keep it open for now. I'd probably change the title; I would not even expect rerun-if-env-changed=CARGO_MANIFEST_DIR to be needed.

@Mark-Simulacrum Mark-Simulacrum transferred this issue from rust-lang/rust Sep 9, 2020
@Mark-Simulacrum Mark-Simulacrum changed the title rerun-if-env-changed=CARGO_MANIFEST_DIR doesn't trigger a rebuild on change CARGO_MANIFEST_DIR changing doesn't trigger a rebuild on change Sep 9, 2020
@Mark-Simulacrum Mark-Simulacrum changed the title CARGO_MANIFEST_DIR changing doesn't trigger a rebuild on change rerun-if-env-changed=CARGO_MANIFEST_DIR changing doesn't trigger a rebuild on change Sep 9, 2020
@Mark-Simulacrum
Copy link
Member

I personally am less certain that this should be considered a bug without the rerun-if-env-changed, as most crates don't want to rebuild when moving their directory around. Indeed, e.g., perf.rust-lang.org relies on that currently...

@alecmocatta
Copy link
Author

IMO the least surprising behaviour is triggering a rebuild if the value returned by any env! invocation in a crate would change.

as most crates don't want to rebuild when moving their directory around

I'm envisaging this would only happen if said crate had env!("CARGO_MANIFEST_DIR"). Which seems reasonable to me? If someone doesn't want this behaviour they can change to env::var("CARGO_MANIFEST_DIR").unwrap().

@alexcrichton
Copy link
Member

Cargo does explicilty not rebuild a crate when it's folder is renamed, but it's true that the compilation would otherwise differ due to environment variables like this. What we can do, however, is Cargo can avoid recompiling on a directry name if this env var isn't used, but it would recompile if it was used. I think that would keep everything working generally as expected for other users which currently rely on this behavior not recompiling.

@ehuss ehuss added the A-rebuild-detection Area: rebuild detection and fingerprinting label Sep 19, 2020
@Veetaha
Copy link

Veetaha commented Aug 2, 2023

I've just been faced with this problem. I think the issue should be named more clearly as "Build script isn't rebuilt if CARGO_MANIFEST_DIR compile-time variable changes, i.e. the source dir was moved".

This bug reproduces in my build script that depends on a crate that uses env!("CARGO_MANIFEST_DIR") to reference sources from its source folder (inside of .cargo/registry/src) with protobuf files. When I mount the sources under a different absolute path in a docker container and mount the target dir cache I see that build fails because the build script wasn't recompiled with a different CARGO_MANIFEST_DIR value for the library that the build script depends on, and thus the script can't find the protobuf files it needs.

@weihanglo
Copy link
Member

In #12403 and #12482 we make it clear that the current behavior of rerun-if-env-changed only taking user-defined environment into account. rerun-if-env-changed isn't aware of env vars set for crates and build scripts.

It remains open to whether those env vars should trigger a rebuild, or we could treat CARGO_MANIFEST_DIR differently than others.

@weihanglo weihanglo added A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables beta-nominated Nominated to backport to the beta branch. S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Aug 13, 2023
@weihanglo weihanglo removed the beta-nominated Nominated to backport to the beta branch. label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-environment-variables Area: environment variables A-rebuild-detection Area: rebuild detection and fingerprinting S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants