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

Scrape code examples from examples/ directory for Rustdoc #9525

Merged
merged 34 commits into from Oct 28, 2021

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented May 30, 2021

Adds support for the functionality described in rust-lang/rfcs#3123

Matching changes to rustdoc are here: rust-lang/rust#85833

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/bin/cargo/commands/doc.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_compile.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_compile.rs Outdated Show resolved Hide resolved
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest LGTM but this should have a reviewer from t-cargo.

src/cargo/ops/cargo_compile.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Jul 27, 2021

☔ The latest upstream changes (presumably #9730) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Collaborator

bors commented Oct 7, 2021

☔ The latest upstream changes (presumably #9943) made this pull request unmergeable. Please resolve the merge conflicts.

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 26, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton
Copy link
Member

Ok everything looks green and we've got good sign-off so I'm gonna r+, thanks @willcrichton for being patient with this!

@alexcrichton
Copy link
Member

Oh actually I recall that I wanted to test this out before merging. I'm testing this out on the wasmtime repository but currently what I get is:

$ rustup run nightly $HOME/code/cargo/target/release/cargo doc --scrape-examples examples -Z unstable-options
error: package ID specification `file:///home/acrichto/code/wasmtime/cranelift#cranelift-tools:0.73.0` did not match any packages

I think that's coming from the Cargo integration here? This project is also slightly wonky where the crate that I'm interested in is crates/wasmtime but all of the examples are located in a top-level directory of examples which are technically examples Cargo-wise for the wasmtime-cli crate at the top level.

* Docscrape unit not having dev-dependencies included
* Sources for reverse-dependencies generated to the wrong directory
* Incorrect features being selected for Docscrape units
* Panics from Docscrape-dependent packages not being available
@willcrichton
Copy link
Contributor Author

willcrichton commented Oct 27, 2021

Good call @alexcrichton!! I fixed no fewer than 5 different bugs when testing on the wasmtime repo, including an upstream fix to rustc at rust-lang/rust#90349.

See my latest commit for what changed + a summary of changes. This extension now works correctly on the wasmtime repo, e.g.

Screen Shot 2021-10-27 at 11 52 17 AM

@alexcrichton
Copy link
Member

Nice! Could you add some tests to Cargo for the bugs you found here as well? I think it's fine to add one "interesting" test instead of lots of little tests as well if you'd prefer.

Also wow this is really nice. The generated docs I think look really good and having everything automatically slurped up is very appealing.

FWIW I ran cargo doc at the root of the repository and then browsed wasmtime's own docs and was disappointed to not see any examples. I then ran cargo doc -p wasmtime, however, and saw a bunch of examples. I believe that this would be fixed with changing the interface from a CLI flag to a configured-in-the-manifest-thing, however, so I'm not too worried about that.

The only other issue I noticed was that there was a precipitious "cliff" in the execution of rustdoc subprocesses where after all the rustc processes (or presumably rustdoc --scrape-examples processes?) finished there was a stampede of new rustdoc processes to execute.

Screen Shot 2021-10-27 at 2 05 15 PM

I think we can probably do better here in the long run because crates like memoffset aren't going to scrape examples from wasmtime-based examples, right? This isn't an issue for now but I do think we'll want to carefully evaluate the impact on time-to-cargo doc because this could run the risk of significantly increasing those costs. (this is assuming a world in which we turn this on-by-default which I still think we'll want to do because of how nice this feature is)

root units. Only attach Docscrape unit dependencies to workspace Doc
units. Add test for scraping examples with complex reverse dependencies.
@willcrichton
Copy link
Contributor Author

Ok I added a test. Also re: your concerns, I changed it so:

  • All doc units for workspace packages will include scraped examples, so cargo doc --scrape-examples just works.
  • Docscrape units are only added as dependencies for workspace packages, so the cliff is avoided.

@alexcrichton
Copy link
Member

I think that's pretty reasonable, yeah, a workspace sounds like a good unit for "scrape examples for all crates within here". It looks like there's some more failing tests though?

@willcrichton
Copy link
Contributor Author

Ah missed those, just some changed stderr output. Fixed the issue and we should be good now!

@alexcrichton
Copy link
Member

@bors: r+

👍

@bors
Copy link
Collaborator

bors commented Oct 28, 2021

📌 Commit 33718c7 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2021
@bors
Copy link
Collaborator

bors commented Oct 28, 2021

⌛ Testing commit 33718c7 with merge 0a98b1d...

@bors
Copy link
Collaborator

bors commented Oct 28, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 0a98b1d to master...

@bors bors merged commit 0a98b1d into rust-lang:master Oct 28, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2021
Update cargo

3 commits in 6c1bc24b8b49d4bc965f67d7037906dc199c72b7..94ca096afbf25f670e76e07dca754fcfe27134be
2021-10-24 17:51:41 +0000 to 2021-10-29 14:45:06 +0000
- Chore: prefer `HashMap::from` rather than collecting `Vec` of tuples (rust-lang/cargo#10018)
- Change --scrape-examples flag to -Z rustdoc-scrape-examples (rust-lang/cargo#10017)
- Scrape code examples from examples/ directory for Rustdoc (rust-lang/cargo#9525)
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Nov 5, 2021
@ehuss ehuss added this to the 1.58.0 milestone Feb 6, 2022
bors added a commit that referenced this pull request Nov 24, 2022
Change rustdoc-scrape-examples to be a target-level configuration

This PR addresses issues raised in #9525. Specifically:
1. It enables examples to be scraped from `#[test]` functions, by passing additional flags to Rustdoc to ensure that these functions aren't ignored by rustc.
2. It moves the `arg` from `-Z rustdoc-scrape-examples={arg}` into a target-level configuration that can be added to Cargo.toml.

The added test `scrape_examples_configure_target` shows a concrete example. In short, examples will be default scraped from Example and Lib targets. Then the user can enable or disable scraping like so:

```toml
[lib]
doc-scrape-examples = false

[[test]]
name = "my_test"
doc-scrape-examples = true
```
bors added a commit that referenced this pull request Nov 24, 2022
Change rustdoc-scrape-examples to be a target-level configuration

This PR addresses issues raised in #9525. Specifically:
1. It enables examples to be scraped from `#[test]` functions, by passing additional flags to Rustdoc to ensure that these functions aren't ignored by rustc.
2. It moves the `arg` from `-Z rustdoc-scrape-examples={arg}` into a target-level configuration that can be added to Cargo.toml.

The added test `scrape_examples_configure_target` shows a concrete example. In short, examples will be default scraped from Example and Lib targets. Then the user can enable or disable scraping like so:

```toml
[lib]
doc-scrape-examples = false

[[test]]
name = "my_test"
doc-scrape-examples = true
```
bors added a commit that referenced this pull request Nov 25, 2022
Change rustdoc-scrape-examples to be a target-level configuration

This PR addresses issues raised in #9525. Specifically:
1. It enables examples to be scraped from `#[test]` functions, by passing additional flags to Rustdoc to ensure that these functions aren't ignored by rustc.
2. It moves the `arg` from `-Z rustdoc-scrape-examples={arg}` into a target-level configuration that can be added to Cargo.toml.

The added test `scrape_examples_configure_target` shows a concrete example. In short, examples will be default scraped from Example and Lib targets. Then the user can enable or disable scraping like so:

```toml
[lib]
doc-scrape-examples = false

[[test]]
name = "my_test"
doc-scrape-examples = true
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants