Skip to content

cargo doc doesn't detect dirty sources from parent directories #12266

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

Closed
P1n3appl3 opened this issue Jun 13, 2023 · 8 comments · Fixed by #15359
Closed

cargo doc doesn't detect dirty sources from parent directories #12266

P1n3appl3 opened this issue Jun 13, 2023 · 8 comments · Fixed by #15359
Labels
A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug Command-doc S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix

Comments

@P1n3appl3
Copy link
Contributor

Problem

When sources aren't in a subdirectory of the cargo manifest, cargo doc fails to detect changes to those sources. The issue only occurs with cargo doc (not check or build). It isn't affected by the working directory (using --manifest-path), and using an absolute vs. relative path for the source makes no difference.

Steps

Use the following directory structure and Cargo.toml with an empty lib.rs:

manifest
└── Cargo.toml
lib.rs
[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[lib]
path = "../lib.rs"

Running cargo doc --manifest-path=manifest/Cargo.toml -v should succeed. Now add a syntax error to lib.rs. Subsequent cargo doc runs will say that the crate is Fresh and exit without detecting the syntax error.

Removing manifest/target manually or by cargo cleaning will force a rebuild, but changes to lib.rs seem to never cause rebuilds.

Possible Solution(s)

No response

Notes

This behavior was encountered on Fuchsia, where we primarily use a different build system (GN+ninja) but have experimental support for generating cargo manifests for rust targets. The Cargo.tomls are generated in an out directory and contain absolute paths pointing to the source root for each crate. We initially thought the issue had to do with the absolute paths or our use of symlinks, but it appears that change detection for cargo doc is broken for ANY sources that aren't in the same dir or subdirs of the manifest.

Version

Verified the behavior with both nightly `cargo 1.72.0-nightly (49b6d9e17 2023-06-09)` and stable `cargo 1.70.0 (ec8a8a0ca 2023-04-25)`
@P1n3appl3 P1n3appl3 added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Jun 13, 2023
@weihanglo
Copy link
Member

Thanks for the report. I believe this is a known issue around the current fingerprint system (recompile detection). Cargo depends on file mtime to track whether it need to re-compile or not.

For normal cargo build there are dep-info files which can be tracked under target directory. Unfortunately, for cargo doc there is no such file emitted from rustdoc1. One of the naive solution here is walking through the filesystem from the root of the package. Finding the last modified file2 and comparing it with the output of cargo doc (index.html or something). Anything outside the root of the current package will be ignored. That's pretty much the root cause.

For resolving this, the long-term fix is having rustdoc to something equivalent to dep-info files. In the short-term, maybe we could create a symlink for lib.rs. But that means you either need to symlink the whole directory starting from lib.rs to the package root, or Cargo needs to be smart enough to understand module systems, which is currently out-of-scope of what Cargo can do. I don't have concrete short-term plans 😞.

Footnotes

  1. https://doc.rust-lang.org/nightly/nightly-rustc/cargo/core/compiler/fingerprint/index.html#rustdoc-mtime-handling

  2. https://github.com/rust-lang/cargo/blob/028077394a5a1d278fe353ea687d852f761ffaa3/src/cargo/sources/path.rs#L584-L591

@weihanglo
Copy link
Member

weihanglo commented Jun 14, 2023

triage: I think it is blocked on external (rustdoc emit something else to help tracking rebuild), but is still welcome to any short-term or better alternatives.

Edited: linked to #9931 which is largely relevant, and rust-lang/rust#91982 which is where this is blocked.

@rustbot label -S-triage +S-blocked-external +Command-doc +A-rebuild-detection

@rustbot rustbot added A-rebuild-detection Area: rebuild detection and fingerprinting Command-doc S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix and removed S-triage Status: This issue is waiting on initial triage. labels Jun 14, 2023
@P1n3appl3
Copy link
Contributor Author

The compiler has the capability to only emit a depinfo file. Would it be possible for cargo to invoke rustc --emit dep-info every time the doc command is run? That way cargo doc would have correct rebuild-detection without requiring rustdoc to grow the depinfo functionality itself.

@weihanglo
Copy link
Member

The Cargo compilation model is that each invocation rustc rustdoc or whatever command is a unit of work. Each unit of work tracks its own fingerprint to determine rebuild or not. As you proposed, by invoking an additional rustc for the root crate, it should work I feel like. That sounds like a reasonable hack if dep-info from rustdoc is not planned in any near future.

Some concerns with this hack:

  • Native dep-info from rustdoc can contain more accurate info than a rustc for that crate, such as external HTML files or CSS stylesheets.
  • Even it is only an additional rustc invocation, it still cost something. Not to say if you have a large project.

I've updated #12266 (comment) to include relevant issues btw.

@zopsicle
Copy link

zopsicle commented Aug 16, 2023

One of the naive solution here is walking through the filesystem from the root of the package.

An easy short-term fix would be to consider the directory that contains the library crate root of the package (as specified by lib.path in Cargo.toml) in addition to the directory that contains the manifest (which is what I assume you mean by "root of the package"). Would that work or would it cause other problems?

@weihanglo
Copy link
Member

It doesn't work for #[path = …] attributes. To be fair, my symlink suggestion doesn't work with that either 😞.

@epage
Copy link
Contributor

epage commented Feb 26, 2025

FYI rustdoc dep info support is being worked on in rust-lang/rust#137684

github-merge-queue bot pushed a commit that referenced this issue Mar 31, 2025
### What does this PR try to resolve?

This leverages the unstable `--emit=depinfo` option from rustdoc,
so that rustdoc invocation rebuild can be better tracked
without traversing the entire directory.

Some design decisions made in the current implementation:

* Rustdoc's depinfo doesn't and shouldn't emit to `target/doc`,
  as the directory is considered part of the final artifact directory.
  In regard to that, we specify the dep-info output path to
  the fingerprint directory of rustdoc invocation.
  It looks like this
  `target/debug/.fingerprint/serde-12d29d32b3b8b38f/doc-lib-serde.d`.
* We also start supporting `-Zchecksum-freshness` as a side effect.
  Could make it a separate PR if desired.
* `-Zbinary-dep-depinfo` is not enabled along with this,
  since doc generations don't really require any binary dependencies.

### How should we test and review this PR?

The tests added has covered these cases:

* target src outside package root, e.g., `lib.path = "../lib.rs"`
* `#[doc = include_str!("../outside/pkgroot")]`
* `#[path = "../outside/pkgroot"]`
* `env!`

### Additional information

Fixes #12266
Closes #15205
@weihanglo
Copy link
Member

See the tracking issue #15370.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug Command-doc S-blocked-external Status: ❌ blocked on something out of the direct control of the Cargo project, e.g., upstream fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants