Improve strategy for selecting targets to be scraped for examples #11430
Conversation
rustbot
added
the
S-waiting-on-review
| @@ -597,6 +591,7 @@ fn generate_targets( | |||
| package_set: &PackageSet<'_>, | |||
| profiles: &Profiles, | |||
| interner: &UnitInterner, | |||
| has_dev_units: HasDevUnits, | |||
There was a problem hiding this comment.
On no. The thirteenth parameter 😂
| @@ -828,6 +823,52 @@ fn generate_targets( | |||
| } | |||
| } | |||
|
|
|||
| if mode.is_doc_scrape() { | |||
There was a problem hiding this comment.
Not a blocker. Do you feel it better if we extract this piece of code to a separate function doing post-process after calling generate_targets? Doing so avoids adding more parameters on generate_targets.
Don't blindly agree with me, though 😆.
There was a problem hiding this comment.
The issue is that this code needs to be in the middle of generate_targets --- it post-processes the proposals but not the units.
In my proposed refactor, I'm going to separate out generate_targets into TargetGenerator::make_proposals and TargetGenerator::proposals_to_units. Then I can cleanly insert the doc-scrape-specific code after make_proposals as post-processing.
|
@weihanglo I share your concern that If it's ok, I would like to merge this PR as-is to get the functionality into Cargo. After this is merged, I will file a second PR that refactors |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
…anglo Refactor generate_targets into separate module ### What does this PR try to resolve? The `generate_targets` function is fairly complicated with an absurd number of parameters. This PR refactors the function into a `TargetGenerator` struct that represents the state of the generator, and reduces the amount of parameter-passing between the relevant functions. Additionally, the `generate_targets` function has been refactored into two smaller functions `create_proposals` and `proposals_to_units`. The docscrape-specific functionality from #11430 has been pulled out into a separate function, as promised. ### How should we test and review this PR? This PR does not change any functionality, so no new tests are added. It should be reviewed for code style. r? `@weihanglo`
9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306 2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000 - Refactor generate_targets into separate module (rust-lang/cargo#11445) - Improve file found in multiple build targets warning (rust-lang/cargo#11299) - Error when precise without -p flag (rust-lang/cargo#11349) - Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430) - Aware of compression ratio for unpack size limit (rust-lang/cargo#11337) - Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403) - Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400) - Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387) - Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
Update cargo 9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306 2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000 - Refactor generate_targets into separate module (rust-lang/cargo#11445) - Improve file found in multiple build targets warning (rust-lang/cargo#11299) - Error when precise without -p flag (rust-lang/cargo#11349) - Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430) - Aware of compression ratio for unpack size limit (rust-lang/cargo#11337) - Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403) - Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400) - Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387) - Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
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), I realized that the current approach of modifying the
CompileFilterdid not correctly implement this new specification. For example, a target withdoc = falseshould 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:
generate_targetsis called with the same parameters except themodewhich becomesCompileMode::Docscrapeinstead ofCompileMode::Doc.filter_default_targetsgenerates the same targets forDocscrapeas forDoc.generate_targets, an initial set ofProposals are created. This set of proposals is extended with further proposals based on targets identified asdoc-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_targetsto verify that only documented or explicitly-enabled targets are included for scraping.r? @weihanglo