Regression: `RUSTDOCFLAGS=--show-coverage cargo doc` deletes any existing docs

[jyn514]

Problem

I tried this code: cargo doc && RUSTDOCFLAGS='-Z unstable-options '--show-coverage cargo doc

I expected to see this happen: There is documentation in target/doc/crate_name

Instead, this happened: The docs get deleted.

searched nightlies: from nightly-2021-04-28 to nightly-2021-04-29 regressed nightly: nightly-2021-04-29 searched commits: from https://github.com/rust-lang/rust/commit/727d101561f9b1e81c6282943292d990288ca479 to https://github.com/rust-lang/rust/commit/ca075d268d2ce315964e1dd195cfe837b8a53f4d regressed commit: https://github.com/rust-lang/rust/commit/27bd3f51a90733aabee45a9019a3aa785eccfd05

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-unknown-linux-gnu Reproduce with: ```bash cargo bisect-rustc --script ./docs-exist.sh --preserve --start 2021-04-28 --end 2021-04-29 ```

Looks like this is related to the recent rustdoc changes (rust-lang/cargo#9419 or rust-lang/cargo#9404).

Steps

1. cargo doc
2. RUSTDOCFLAGS=--show-coverage cargo doc
3. [ -e ${CARGO_TARGET_DIR:-target}/doc/crate_name ]

Possible Solution(s)

Only delete the docs folder if the sources or toolchain changes, not the invocation.

Notes

For context, this causes problems for docs.rs because we always run coverage after the docs are generated. We can work around it for now by running coverage before instead, but it would be nice to fix this. In the meantime I've pinned docs.rs to an older toolchain.

[Nemo157]

The issue seems most likely to be https://github.com/rust-lang/cargo/pull/9419/files#diff-cf607b2deb0f9069b2b23668b40154d77dc081de1ca316a38988324119b2ed3eR652-R658.

There are other issues with the --show-coverage integration, because cargo doesn't know that it's output is text rather than files running it multiple times doesn't re-output the text:

> cargo rustdoc -- --show-coverage -Z unstable-options
 Documenting empty-library v1.0.0 (/tmp/tmp.CODWh0TKuw/empty-library-1.0.0)
+-------------------------------------+------------+------------+------------+------------+
| File                                | Documented | Percentage |   Examples | Percentage |
+-------------------------------------+------------+------------+------------+------------+
| src/lib.rs                          |          0 |       0.0% |          0 |       0.0% |
+-------------------------------------+------------+------------+------------+------------+
| Total                               |          0 |       0.0% |          0 |       0.0% |
+-------------------------------------+------------+------------+------------+------------+
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s

> cargo rustdoc -- --show-coverage -Z unstable-options
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s

Looking into that I noticed that there appears to be a similar issue with cargo rustc -- --print commands, they cause the .rlib to be deleted.

> ls /home/nemo157/.cargo/shared-target/debug/deps/libempty_library-ffad2230ba3abdb6.rlib
ls: cannot access '/home/nemo157/.cargo/shared-target/debug/deps/libempty_library-ffad2230ba3abdb6.rlib': No such file or directory

> cargo rustc
   Compiling empty-library v1.0.0 (/tmp/tmp.CODWh0TKuw/empty-library-1.0.0)
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s

> ls /home/nemo157/.cargo/shared-target/debug/deps/libempty_library-ffad2230ba3abdb6.rlib
/home/nemo157/.cargo/shared-target/debug/deps/libempty_library-ffad2230ba3abdb6.rlib

> cargo rustc -- --print crate-name
   Compiling empty-library v1.0.0 (/tmp/tmp.CODWh0TKuw/empty-library-1.0.0)
empty_library
    Finished dev [unoptimized + debuginfo] target(s) in 0.07s

> ls /home/nemo157/.cargo/shared-target/debug/deps/libempty_library-ffad2230ba3abdb6.rlib
ls: cannot access '/home/nemo157/.cargo/shared-target/debug/deps/libempty_library-ffad2230ba3abdb6.rlib': No such file or directory

[jyn514]

For context, this causes problems for docs.rs because we always run coverage after the docs are generated. We can work around it for now by running coverage before instead, but it would be nice to fix this

Hmm, doing this means we have to unconditionally run coverage even if the build fails, which wastes time. We also assume in several places that creating coverage never fails; I guess we could change that but it would be a pain.

[ehuss]

Is there a reason cargo deletes things between each invocation?

It deletes the directory in order to remove any stale files (for example, removed items). We could certainly revert that, since it isn't super important (in particular revert 44c549e6505c418a0c41360a5edd6edf008fda40, and fixup the test which relies on that behavior). It could also be possible that rustdoc could take on that responsibility?

I'm not really sure how --show-coverage fits within Cargo's model, since it appears to not generate any output files? Cargo has to make some assumptions about the behavior of the tools it runs, particularly that certain inputs result in certain outputs.

[jyn514]

It deletes the directory in order to remove any stale files (for example, removed items).

Right, I understand, but why does it do that between each invocation? Rustdoc shouldn't generate fewer files than before unless someone passed --document-private-items/--document-hidden-items before and then removed it.

I'm not really sure how --show-coverage fits within Cargo's model, since it appears to not generate any output? Cargo has to make some assumptions about the behavior of the tools it runs, particularly that certain inputs result in certain outputs.

It's no different from rustc --print, which cargo also handles incorrectly. I don't really have suggestions for how to handle this - maybe instead of overwriting the files in place, cargo can use a temporary --out-dir, and only rename it to target/doc if it's non-empty? That should avoid caching issues without having to hard-code which options don't generate docs.

[jyn514]

It could also be possible that rustdoc could take on that responsibility?

I would rather not special case rustdoc - whatever solution to this we come up with should also work for rustc. If that means rustc needs to delete stale .rlib files instead of cargo doing it, that seems ok (although a lot of work), but I don't think rustdoc should be the only one in charge of its build outputs.

[ehuss]

Right, I understand, but why does it do that between each invocation? Rustdoc shouldn't generate fewer files than before unless someone passed --document-private-items/--document-hidden-items before and then removed it.

If one runs cargo doc, and then removes any item (a function, a module, whatever), and the runs cargo doc again, the removed items will still remain in the output directory.

[jyn514]

That will change the source mtime, won't it? Or is the issue you don't know what files rustdoc reads because it doesn't have an --emit=depinfo option?

[ehuss]

The mtime changes, and cargo runs rustdoc again, but the old output files would otherwise remain. For example, the first run creates target/doc/foo/fn.some_function.html. Remove that function and run cargo doc again, and target/doc/foo/fn.some_function.html remains in the output directory.

It's not a big deal, since it gets removed from the index and sidebar, so I'm fine with reverting the change. There are some obscure cases where it can cause confusion (like if the user has something loaded in their browser, they rebuild docs for different version or target or features that don't include that item, but they reload their browser and the item is still there), but I don't think it is a common problem.

One of the goals of a build system is to reliably produce the same output based on the same input. We generally prefer to avoid situations where providing correct output requires the user to essentially run rm -rf (or make clean or cargo clean) because the build directory is "messed up". The intent here was to clean up stale files that otherwise accumulate over time.