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

Change rustdoc-scrape-examples to be a target-level configuration #10343

Merged
merged 5 commits into from Nov 25, 2022

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented Jan 30, 2022

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:

[lib]
doc-scrape-examples = false

[[test]]
name = "my_test"
doc-scrape-examples = true

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2022
@camelid
Copy link
Member

camelid commented Jan 31, 2022

Somehow I feel like it'd be better to structure the configuration as something like:

[doc]
scrape-examples-targets = ["lib", "test"]

rather than putting doc-scrape-examples in each target section. Just a gut reaction though.

@willcrichton
Copy link
Contributor Author

Somehow I feel like it'd be better to structure the configuration as something like:

[doc]
scrape-examples-targets = ["lib", "test"]

rather than putting doc-scrape-examples in each target section. Just a gut reaction though.

I think the issue with this is (a) it's not consistent with existing ways to configure targets, and (b) it doesn't allow target configuration at a higher granularity. For instance, the proposed method allows you to individually enable/disable specific examples and tests, while a list of target strings can't do so.

@alexcrichton
Copy link
Member

This all looks pretty reasonable to me, but I'd like to confirm my understanding. The intention is that once stabilized this is enabled for all projects by default? So all projects with libraries and examples, when using cargo doc, will see examples for methods and such drawn from libraries examples. Packages have fine-grained control at the per-target level what's used as candidates for examples and what isn't (with the default being only the lib targets and examples)

If that's the case I think it might be good to flesh out the failing tests for now. I think there may need to be some trickery around not running doc-scrape with stable Rust as-is today since Cargo tests its CI with all three channels of Rust. Otherwise though that seems pretty reasonable to me and the PR otherwise looks pretty good.

What is the right set of default targets to search for examples?

Personally the choice made here I think is fine. This seems like something we can alter over time if necessary as well.

Is there a different granularity at which example scraping should also be configurable?

At least from Cargo's perspective this probably won't have much impact, but I don't have much of an opinion on this otherwise.


As a random bikeshedding point seeing doc-scrape-examples = false in isolation feels a little surprising, so I might suggest something like doc-example-candidate = false or something like that.

@GuillaumeGomez
Copy link
Member

I thought this feature would be disabled by default?

@willcrichton
Copy link
Contributor Author

willcrichton commented Feb 1, 2022

I thought this feature would be disabled by default?

My understanding is that it would be disabled by default while unstable, and then enabled by default once stabilized. The vast majority of people will not use this feature if it is hidden behind a flag.

@GuillaumeGomez
Copy link
Member

My understanding is that it would be disabled by default while unstable, and then enabled by default once stabilized. The vast majority of people will not use this feature if it is hidden behind a flag.

Sorry, I definitely didn't understand it like this. In this case, we'll need to update the display quite a lot because it takes a lot of space by default. What about collapsing the section by default with a title like "see usage in examples" or something among the line? If you prefer we can discuss it on zulip.

@willcrichton
Copy link
Contributor Author

The discussion about scraping-by-default-or-not has been pulled onto Zulip: https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Enabling.20scrape.20examples.20by.20default

@alexcrichton alexcrichton added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2022
@bors
Copy link
Collaborator

bors commented Feb 22, 2022

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

@lqd
Copy link
Member

lqd commented Mar 28, 2022

@willcrichton The .rustfmt.toml change and new log file, seem unintentional.

@willcrichton
Copy link
Contributor Author

Ah thanks @lqd , got caught up in the rebase. I'll squash everything in a bit.

@bors
Copy link
Collaborator

bors commented Apr 4, 2022

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

@willcrichton willcrichton force-pushed the example-analyzer branch 2 times, most recently from 836e9f1 to 1daaf68 Compare April 14, 2022 00:46
@willcrichton
Copy link
Contributor Author

willcrichton commented Apr 14, 2022

This PR is ready for review, although it needs a new Cargo reviewer since Alex has stepped back (is there a way to nominate a new reviewer?)

As a reminder, this PR will enable the scrape examples feature by default, although only when run with a nightly rustc. Now that rust-lang/rust#93217 has landed, I think this feature is ready to be rolled out at least on docs.rs. Rustdoc maintainers, please let me know if you have any remaining concerns about this feature.

cc @jsha @camelid @GuillaumeGomez

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Apr 14, 2022
@willcrichton
Copy link
Contributor Author

It seems like only @ehuss is on reviewer rotation for Cargo right now, so I will add him. Sorry for all the extra work Eric :(

r? @ehuss

@bors
Copy link
Collaborator

bors commented Nov 24, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 24, 2022
@weihanglo
Copy link
Member

@bors retry

@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 Nov 25, 2022
@bors
Copy link
Collaborator

bors commented Nov 25, 2022

⌛ Testing commit 183425f with merge de56c12...

@bors
Copy link
Collaborator

bors commented Nov 25, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing de56c12 to master...

@bors bors merged commit de56c12 into rust-lang:master Nov 25, 2022
weihanglo added a commit to weihanglo/rust that referenced this pull request Nov 25, 2022
5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744
2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000
- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 26, 2022
Update cargo

5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744 2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000

- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)

r+ `@ghost`
weihanglo added a commit to weihanglo/rust that referenced this pull request Nov 27, 2022
5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744
2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000
- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 28, 2022
Update cargo

5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744 2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000

- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)

r+ `@ghost`
epage added a commit to clap-rs/clap that referenced this pull request Nov 29, 2022
bors added a commit that referenced this pull request Nov 29, 2022
Improve strategy for selecting targets to be scraped for examples

### What does this PR try to resolve?

After #10343, we have identified a clear set of conditions for whether a particular target should be considered by `-Zrustdoc-scrape-examples`. These conditions are described more clearly in #11425.

However, after some testing with complex Cargo workspaces (e.g. [wasmtime](https://github.com/bytecodealliance/wasmtime/)), I realized that the current approach of modifying the `CompileFilter` did not correctly implement this new specification. For example, a target with `doc = false` should not be scraped by default since it is not a documented unit, but the current approach would potentially include such a target for scraping.

This PR provides a new approach which I believe correctly implements the specification:
1. `generate_targets` is called with the same parameters except the `mode` which becomes `CompileMode::Docscrape` instead of `CompileMode::Doc`. `filter_default_targets` generates the same targets for `Docscrape` as for `Doc`.
2. Inside `generate_targets`, an initial set of `Proposal`s are created. This set of proposals is extended with further proposals based on targets identified as `doc-scrape-examples = true`, or Example targets where possible.

This PR subsumes #11423, and also fixes #10571.

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

I have added another test `docscrape::only_scrape_documented_targets` to verify that only documented or explicitly-enabled targets are included for scraping.

r? `@weihanglo`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 3, 2022
Update cargo

5 commits in ba607b23db8398723d659249d9abf5536bc322e5..e027c4b5d25af2119b1956fac42863b9b3242744 2022-11-22 20:52:39 +0000 to 2022-11-25 19:44:46 +0000

- fix: Move off atty to resolve soundness issue (rust-lang/cargo#11420)
- add newline char to `cargo install .` error message for easier reading. (rust-lang/cargo#11401)
- chore: Upgrade to env_logger (rust-lang/cargo#11417)
- Change rustdoc-scrape-examples to be a target-level configuration (rust-lang/cargo#10343)
- temporarily disable test `lto::test_profile` (rust-lang/cargo#11419)

r+ `@ghost`
@ehuss ehuss added this to the 1.67.0 milestone Dec 14, 2022
bors added a commit that referenced this pull request Dec 18, 2022
Enable triagebot's relabel functionality

### What does this PR try to resolve?

This fixes the following failure that rustbot currently posts whenever someone tries to use "<b>`@</b><b>rustbot</b>` label" in this repository.

> **Error**: The feature `relabel` is not enabled in this repository.
> To enable it add its section in the `triagebot.toml` in the root of the repository.

Unauthenticated relabel has been enabled in rust-lang/rust for nearly 4 years. People overwhelmingly use it in good faith.

<br>

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

Compare against https://github.com/rust-lang/rust/blob/1.66.0/triagebot.toml.

Also skim through the 7 pages of labels on https://github.com/rust-lang/cargo/labels, whether it makes sense the ones I decided to allow arbitrary GitHub users to apply.

<br>

### Additional information

Attempted uses of "<b>`@</b><b>rustbot</b>` label", that failed, but this PR would allow:

- #10343 (comment)
- #10243 (comment)
- #9982 (comment)
- #9128 (comment)
- #9067 (comment)
- #8441 (comment)
- #11432 (comment)
- #8841 (comment)
- #10820 (comment)
- #10572 (comment)
- #9114 (comment)
- #8980 (comment)
- #9064 (comment)
- #8726 (comment)
- #8089 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet