Make doctest compile / run cwd consistent with other test kinds #8993
Comments
|
I think @jan-auer and me were both very eager to create this, this is basically a duplicate of: #8992 Since one workaround for this is to use |
|
Can you explain why |
I don’t know exactly why workspace relative paths are needed, just that the llvm-based tools need them to have proper coverage mappings. For example when I run my testcases from https://github.com/Swatinem/fucov, I get the following lcov output: Note however that the test on line 3 comes from When I switch the llvm tools to html output, I get the following: Especially the Other test types are covered properly in the workspace setting, just not doctests. |
|
Ok, so basically rustdoc has always behaved differently from rustc and people depended on that fact? That's really unfortunate :/ I think doing this only when running the test would still run into the same issue: people could be opening relative file system paths at runtime, and that would break if you changed the working directory. I still think the best path forward is to figure out why LLVM depends on the current working directory and maybe tell it to use paths relative to the workspace instead. |
|
I think the crucial detail is the difference between compiling and then running the doctest. For coverage, it is apparently required to have the doc tests compiled from the workspace root. I'm not that familiar with the LLVM tooling myself, but I'm assuming they are using DWARF's relative paths, which are always based on the "compilation directory", which is more-or-less the CWD of the compiler invocation. This is all defined at compile time. Contrary to this, cargo already runs all other tests in the respective crate root, which may be a workspace member. That means, for unit and integration tests, there is already an expectation that by default relative file paths are always evaluated from Cargo.toml, and I think the same should hold for doc tests. @Swatinem please correct me if I'm wrong in any of the above. |
Sure, but that would be a breaking change. At the very least it would need an edition, but I'm still not convinced there's not a way to pass this information to LLVM in another way. |
|
I would propose to maybe add a new CLI flag to rustdoc that controls the runtime cwd, while keeping the current compile-time cwd behavior. Does that sound good? @jyn514 Also, I think @jan-auer is correct that the llvm tools probably use DWARF and similar debuginfo for all this. I haven’t looked at how this all works in detail, but that would at least explain why you need to provide all your compiled objects (executables) to the coverage tools. |
If there's a way to do this so that changing the directory is opt-in and only happens when collecting coverage information, I would be ok with this. Otherwise, it's still a breaking change IMO. |
Explain please. Also, is all of this specified/documented somewhere how these things should behave? |
Previously, rustdoc would run tests in the crate directory, and the behavior of that was observable in doc tests (with This is probably more on the T-cargo side than the T-rustdoc side, and if the cargo team is willing to make this change I'll defer to them, but I'm still concerned about the breaking change.
Probably not, one of the issues of rustdoc is that a lot of people depend on the implementation rather than the documented behavior; this is one of the reasons intra-doc links had to be stabilized, because even though they were unstable half the ecosystem was using them: rust-lang/rust#63305. |
Thats why my original PR was reverted. My proposal would be to keep the current runtime behavior, while changing the compile-time cwd, specifically to not break this. |
|
btw, changing the compile-time cwd would probably be observable when using the unstable |
|
@Swatinem is there a way to get the relative path from the workspace root to the doctest root via API, during the Unless I'm mistaken, nothing in the coverage map implementation really cares what the actual filename is during compile time. What's important is that You could force absolute paths for doctests, but I think it's cleaner to just prepend the doctest root path with the path from workspace root to doctest root, the same way you offset the line numbers, for example: let start_line = source_map.doctest_offset_line(&source_file.name, start_line);
let end_line = source_map.doctest_offset_line(&source_file.name, end_line);
let file_name = source_map.doctest_offset_file_name(&source_file.name, file_name);
CodeRegion {
file_name,
start_line: start_line as u32,
start_col: start_col.to_u32() + 1,
end_line: end_line as u32,
end_col: end_col.to_u32() + 1,
}I don't think anything else needs to change. |
To avoid regressions to the test runtime directory, this asserts that all test types (unit, integration, doctest) are executed in the crate (manifest) directory, no matter where that crate is in relation to the workspace root. See rust-lang#8992 / rust-lang#8993
|
So after thinking very hard about the comment above and a discussion on zulip that we had, I still disagree with the proposed solutions here. Let me explain. What issues / implications do we have:
Proposal 1Introduce a new
Also this proposal is forward compatible if rustdoc better seperates the compile from the run step, and maybe hands the responsibility off to cargo. Proposal 2If I understand correctly, the proposal here is to pass a
ConclusionI really do think that proposal 1 would result in a simpler, more straight forward implementation and implicitly fix a few issues at once, while being more forward compatible. |
|
@Swatinem for proposal one, what happens if a doctest has |
|
@Swatinem - Thank you for the well thought out proposals. I'll defer to the more experienced compiler team members on what the best approach is. One correction: With You should remove item 4 from your issues list. |
so the profile counters use a different mechanism? Either way, it is something thats embedded in the produced executable, I will rename accordingly |
I will add a test for this. I think these are relative to the current file, but I will double check. |
|
Ok, this seems good to me then. |
This option will allow splitting the compile-time from the run-time directory of doctest invocations and is one step to solve rust-lang/cargo#8993 (comment)
Run rustdoc doctests relative to the workspace By doing so, rustdoc will also emit workspace-relative filenames for the doctests. This was first landed in #8954 but later backed out in #8996 because it changed the CWD of rustdoc test invocations. The second try relies on the new `--test-run-directory` rustdoc option which was added in rust-lang/rust#81264 to explicitly control the rustdoc test cwd. fixes #8993
In #8954 I changed the cwd that
rustdocitself is invoked to be similar to howrustcis invoked.This is needed to get correct workspace-relative file paths, at least for
-Z instrument-coveragepurposes (rust-lang/rust#79417) (maybe also debuginfo, but I haven’t checked).The problem is that this also changes the cwd in which the doctests themselves are executed, which is sadly a regression from stable :-(
CC @jan-auer @alexcrichton @richkadel maybe @jyn514
I have a small example here with unit, integration and doctests, running it with:
stable:
rustc --crate-name child --edition=2018 child/src/lib.rs […]rustdoc --edition=2018 --crate-type lib --test /Users/swatinem/Coding/cargo-test-cwd/child/src/lib.rs […]nightly:
rustc --crate-name child --edition=2018 child/src/lib.rs […]rustdoc --edition=2018 --crate-type lib --crate-name child --test child/src/lib.rs […]Full output below, but it shows that unit and integration tests are run with the crate directory as CWD, not the workspace, but the compiler itself is invoked from the workspace.
I think we need a way to tell rustdoc to use a specific cwd when running the doctests vs when compiling them.
`cargo +stable test --workspace --verbose -- --nocapture > stable.txt 2>&1`
`cargo +nightly test --workspace --verbose -- --nocapture > nightly.txt 2>&1`
The text was updated successfully, but these errors were encountered: