rustdoc: Add option to persist doc test executables #56189
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Edit: figured it out I think, was just using it wrong lol |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Hm, I didn't noticed that somehow the |
|
Pinging from triage, looking for a reviewer from |
|
This looks nice and I think it's good to go. However, since it touches rustdoc options and @QuietMisdreavus reworked it recently, I'll let them review this PR. |
src/librustdoc/test.rs
Outdated
| let mut target_path: PathBuf = env::var("OUT_DIR") | ||
| .expect("Rustdoc must be run with Cargo to persist doctest executables.").into(); | ||
|
|
||
| while !target_path.ends_with("target") { |
There was a problem hiding this comment.
This doesn't seem safe. The target directory is not necessarily named "target". Also, cargo has a bit of an expectation of managing the directory layout. Normally tools use something like -o to control their output.
src/librustdoc/test.rs
Outdated
| } | ||
|
|
||
| let target_path = if persist_executable { | ||
| let mut target_path: PathBuf = env::var("OUT_DIR") |
There was a problem hiding this comment.
OUT_DIR is only set for build scripts. How is this supposed to be used?
There was a problem hiding this comment.
Is it? I don't believe the project I tested it on had one, but I could be remembering wrong. Regardless, I'll be taking @QuietMisdreavus's advice and taking a path in the cmd args :)
There was a problem hiding this comment.
Thanks so much for this! Improvements like this are something i've wanted to work on for doctests, or at least see happen, so it's exciting to see! I've left a few suggestions, to make the feature fit better with the way rustdoc works in general, and one or two personal style preferences.
src/librustdoc/test.rs
Outdated
|
|
||
| let target_path = if persist_executable { | ||
| let mut target_path: PathBuf = env::var("OUT_DIR") | ||
| .expect("Rustdoc must be run with Cargo to persist doctest executables.").into(); |
There was a problem hiding this comment.
I feel like Rustdoc shouldn't assume the presence of Cargo. If it's being run bare, we can probably get away with either just creating/using a rustdoctest subdirectory of the current working directory, or (my preferred choice) taking the directory as a CLI flag.
In fact, i think it would be better if the CLI flag that unlocked this functionality just took the directory itself, rather than trying to infer it like this. This way, tooling that wants to use this functionality can set the directory itself instead of working backwards from the target directory. Plus, if Cargo adds a flag to unlock this later on without forcing users to use RUSTDOCFLAGS, it can pass target/rustdoctest by itself.
There was a problem hiding this comment.
Huh, I never considered taking in the path, sounds like a much better idea! 👌
src/librustdoc/test.rs
Outdated
| @@ -346,6 +391,17 @@ fn run_test(test: &str, cratename: &str, filename: &FileName, line: usize, | |||
| }; | |||
| cmd.env(var, &newpath); | |||
|
|
|||
| if persist_executable { | |||
| let mut target_path = target_path.unwrap(); | |||
There was a problem hiding this comment.
This could probably be better written as if let Some(target_path) = target_path, to avoid the unwrap.
There was a problem hiding this comment.
The reason I didn't was because it needs cloned, but that's solvable with if let too and I like that better also.
src/librustdoc/test.rs
Outdated
|
|
||
| std::fs::write( | ||
| target_path, | ||
| newpath.as_os_str().to_str().unwrap().as_bytes(), |
There was a problem hiding this comment.
This seems a little awkward, because it's not always the same environment variable that's being set here. A better option would be to make this an environment file, and make the file output be the string "{var}={newpath}", to at least show that this should be setting something other than PATH (on non-Windows platforms, at least). (With these file contents, i'd probably also name it something like test.env, to make it more obvious what it's there for.)
One situation where this breaks down is if the doctests are being built for a different target than the host. It looks like the chosen environment variable will always be stuck to what the host expects, but if you're expecting to run the doctests on another target (say, some embedded platform), then that may be different. However, in this situation it's also possible that the dylib path extension may not be necessary. Still, i think the above suggestion should be good enough.
There was a problem hiding this comment.
Ah, okay, yeah fair enough :)
There was a problem hiding this comment.
I'm pretty sure we don't need any of this newpath stuff now that we're not compiling doctests with prefer-dynamic since #54939. I don't think we need a path.txt or test.env file at all.
There was a problem hiding this comment.
That would be fantastic! However I wasn't able to execute them without the paths generated in that file, so I'm not sure what the issue would be there.
src/librustdoc/lib.rs
Outdated
| @@ -338,6 +338,11 @@ fn opts() -> Vec<RustcOptGroup> { | |||
| "enable-index-page", | |||
| "To enable generation of the index page") | |||
| }), | |||
| unstable("persist-executable", |o| { | |||
There was a problem hiding this comment.
Bikeshed alert: I feel like --persist-doctests is a better name, since the executable here feels a little ambiguous if there's a binary crate somewhere in the user's mind that could get confused with this.
There was a problem hiding this comment.
Haha, no idea how I missed that. Definitely agree with you there.
Dylan-DPC-zz
added
S-waiting-on-author
|
Thanks for the comments @ehuss and @QuietMisdreavus! I'll get the changes in soon:tm: (maybe even tonight or tomorrow night!) |
src/librustdoc/test.rs
Outdated
|
|
||
| std::fs::write( | ||
| target_path, | ||
| newpath.as_os_str().to_str().unwrap().as_bytes(), |
There was a problem hiding this comment.
Path has a to_str method, you souldn't need to go through OsStr
https://doc.rust-lang.org/nightly/std/path/struct.Path.html#method.to_str
There was a problem hiding this comment.
newpath is an OsString
There was a problem hiding this comment.
Ah, well OsString coerces to an OsStr, so you should be able to call to_str() directly on the OsString, but not a big deal
|
☔ The latest upstream changes (presumably #54517) made this pull request unmergeable. Please resolve the merge conflicts. |
repnop
force-pushed
the
keep_doc_test_executable
branch
from
ab904f1
to
0c999ed
Compare
repnop
force-pushed
the
keep_doc_test_executable
branch
from
09434f5
to
f6a5fd6
Compare
|
Does anyone have any other suggestions for where to put the doctest executables themselves? The |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@QuietMisdreavus unless you or anyone else has final thoughts on where its outputting the test files, looks like we're good to go whenever 👍 |
One option is to use something like the You could make another function like this (to make something that was easier to use as a directory name) and hand that to |
src/librustdoc/lib.rs
Outdated
| unstable("persist-doctests", |o| { | ||
| o.optopt("", | ||
| "persist-doctests", | ||
| "Persists the rustdoc test executables", |
There was a problem hiding this comment.
| "Persists the rustdoc test executables", | |
| "Directory to persist doctest executables into", |
Makes it obvious what the path is for.
src/librustdoc/test.rs
Outdated
| .rsplit('/') | ||
| .next() | ||
| .unwrap() | ||
| .replace(".", "_"), line) |
There was a problem hiding this comment.
The , line hanging on the end of this call looks awkward. Can you break it onto its own line?
|
☔ The latest upstream changes (presumably #56818) made this pull request unmergeable. Please resolve the merge conflicts. |
repnop
force-pushed
the
keep_doc_test_executable
branch
from
1e82664
to
94e576b
Compare
|
@bors r+ Whoops, closed the tab and totally forgot about this one... |
|
📌 Commit 94e576b73d06be72b9701dec8aae22d723e85b64 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
|
☔ The latest upstream changes (presumably #57392) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
Bless test, remove submodule, and fix book entry. bless test again? maybe it'll work this time...
repnop
force-pushed
the
keep_doc_test_executable
branch
from
94e576b
to
f5413cd
Compare
|
We seriously need to find a way to strip that panic message (and the line numbers) from the doctest failure output. It's causing a lot of problems. @bors r+ |
|
📌 Commit f5413cd has been approved by |
bors
added
S-waiting-on-bors
…eavus rustdoc: Add option to persist doc test executables Fixes #37048. This is the initial version of the code so the doctest executables can be used for stuff like code coverage (specifically xd009642/tarpaulin#13) the folders it goes into were just a first idea, so any better ones are welcome. Right now it creates a directory structure like: ``` given_path/ |_____ <filename>_rs_<linenum>/ |_____ ... |_____ <filename>_rs_<linenum>/ |_____ rust_out ``` I couldn't figure out where it actually outputs the file w/ the name, I suspect its somewhere deeper in the compiler. It also adds the unstable `--persist-doctests` flag to `rustdoc` that enables this behavior.
|
☀️ Test successful - checks-travis, status-appveyor |
|
This is so awesome! Thank you for working on this feature! It's something people have been looking forward to for a while. 😄 This has really great implications for code coverage. I helped get Codecov.io to start supporting Rust and soon we will be able to remove the big note at the top of the example-rust repo that says that doctests can't be used for coverage. 🎉 |
|
Yep! I'm friends with the author of https://github.com/xd009642/tarpaulin and this is one of the issues he had opened on his repo, so decided to take a stab at it to help him out 👍 There's some more plans in works for better doctest ergonomics now that this is merged so things should get even better in the future :D |
Fixes #37048.
This is the initial version of the code so the doctest executables can be used for stuff like code coverage (specifically xd009642/tarpaulin#13) the folders it goes into were just a first idea, so any better ones are welcome.
Right now it creates a directory structure like:
I couldn't figure out where it actually outputs the file w/ the name, I suspect its somewhere deeper in the compiler.
It also adds the unstable
--persist-doctestsflag torustdocthat enables this behavior.