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

Relative cargo:rerun-if-changed paths are not resolved in dep-info files #9445

Closed
futile opened this issue May 1, 2021 · 5 comments · Fixed by #9596
Closed

Relative cargo:rerun-if-changed paths are not resolved in dep-info files #9445

futile opened this issue May 1, 2021 · 5 comments · Fixed by #9596
Labels
C-bug Category: bug

Comments

@futile
Copy link

futile commented May 1, 2021

Problem

If I have a dependency that has a build.rs build script that outputs a relative path (e.g., cargo:rerun-if-changed=build.rs), this path will appear unchanged in dep info files of my crate.

This path can then not be resolved by external build tools.

Steps

  1. Create a new cargo project (cargo new --bin dep-info-test && cd dep-info-test)
  2. Add a dependency that outputs a cargo:rerun-if-changed=<relative path>, e.g., libc: echo 'libc = "0.2"' >> Cargo.toml
  3. Build the project and check the resulting dep-info file: cargo build && cat target/debug/dep-info-test.d:
[...]
/abs/path/to/dep-info-test/target/debug/dep-info-test: /abs/path/to/dep-info-test/src/main.rs build.rs

Possible Solution(s)
It seems that cargo's internal change-detection mechanism (something with fingerprints?) resolves relative paths output by build scripts as relative to their CARGO_MANIFEST_DIR (I think), which should probably explicitly also be done for dep-info files.

Also, in the case of crates.io- & git-dependencies, cargo doesn't write paths to the dependencies' files itself into the dep-info file, as it regards such dependencies as 'read-only'. So for non-path dependencies absolute paths that point into a dependency's CARGO_MANIFEST_DIR should probably not appear in dep-info files anymore (but should still appear for path-dependencies).

Notes

Output of cargo version:

$ cargo version
cargo 1.53.0-nightly (0ed318d18 2021-04-23)

Same behavior with current stable:

$ cargo version
cargo 1.51.0 (43b129a20 2021-03-16)

Extra discussion material

I think the dep-info files are currently misleading, as they don't capture all conditions under which cargo needs to rebuild a project. For example, environment variables registered using cargo:rerun-if-env-changed are not captured in this file. Another example are .cargo/config files, which often don't exist, but influence compilation, so they would need to be in every dep-info file. These can also appear outside the project, i.e., in every parent directory down to the root-directory.

The fact that this file is incomplete is not mentioned in the current docs, but maybe should be mentioned there.

@futile futile added the C-bug Category: bug label May 1, 2021
@ehuss
Copy link
Contributor

ehuss commented May 1, 2021

Thanks for the report! This should be fixed via #9421, which I think should be in the next nightly tomorrow.

If you would like to extend the documentation to mention things that tools should be aware of (like environment variables), a PR would be appreciated!

@futile
Copy link
Author

futile commented May 1, 2021

Ohh missed that, but great it's already fixed! I'll try tomorrow/in the next few days with the new nightly.

One issue that might still exist is that the absolute path to the build.rs is usually not inserted by cargo for git/crates.io-dependencies as these seem to be considered read-only, but from the initial post in in the PR it looks like they might be present with the PR. I'll see if that's the case after trying it out.

@futile
Copy link
Author

futile commented May 2, 2021

Just tried it out with the current nightly (cargo 1.53.0-nightly (f3e13226d 2021-04-30)), which produces the following file:

/abs/path/to/dep-info-test/target/debug/dep-info-test: /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.94/build.rs /abs/path/to/dep-info-test/src/main.rs

So it's working 🎉 !

However, the absolute path to the build.rs inside cargo's local registry appears in the dep-info file now. This differs from cargo's usual behavior that doesn't add paths to its registry into dep-info files for git/crates.io-dependencies, as these are regarded read-only (I think). (For path-dependencies this aligns with cargo's current behavior.)

@futile
Copy link
Author

futile commented May 6, 2021

@ehuss ping; just wanted to ask if you want me to leave the issue open/open another issue for the small detail from my last comment, otherwise this is fixed, thanks a lot! :)

@ehuss
Copy link
Contributor

ehuss commented May 7, 2021

I'd like to keep it open to investigate what you mentioned, I haven't had time to take a look. It seems a little surprising, though I can't imagine it would have negative consequences, I'd like to understand it better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants