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

Enable doctests in compiler/ crates #99324

Merged
merged 13 commits into from
Oct 6, 2022
Merged

Conversation

reez12g
Copy link
Contributor

@reez12g reez12g commented Jul 16, 2022

Helps with #99144

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jul 16, 2022
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2022
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2022
@Mark-Simulacrum Mark-Simulacrum assigned jyn514 and unassigned kennytm Aug 1, 2022
@Mark-Simulacrum Mark-Simulacrum self-assigned this Aug 21, 2022
@Mark-Simulacrum
Copy link
Member

I would like to verify that these aren't already running on other builders - at a quick look, CrateLibrustc is default enabled, so I would expect them to be...

@Mark-Simulacrum
Copy link
Member

Yeah, it looks like we are?

If I look at the llvm-12 builder (after a rebase/rerun llvm-13) builder in this PR, or from a recent master merge:

2022-07-16T12:23:18.2198781Z �[0m�[0m�[1m�[32m Running�[0m unittests src/lib.rs (build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps/rustc_parse_format-d175d01f72a50abf)
2022-07-16T12:23:23.8389736Z �[0m�[0m�[1m�[32m Doc-tests�[0m rustc_parse_format

So I think we should try to understand what we're intending to add here -- @jyn514, marking as waiting on reviewer for you to confirm whether #99144 actually needs fixing.

@Mark-Simulacrum Mark-Simulacrum removed their assignment Aug 21, 2022
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2022
@jyn514
Copy link
Member

jyn514 commented Sep 7, 2022

Something strange is going on. https://pipelines.actions.githubusercontent.com/serviceHosts/d3b42637-680e-47c4-b396-04d4ba514549/_apis/pipelines/1/runs/64123/signedlogcontent/8?urlExpires=2022-09-07T03%3A02%3A56.8274271Z&urlSigningMethod=HMACV1&urlSignature=XpkHU6Ja%2BoIzOyGtj83HPrWJa1K0ULVXrUQCmh3Na40%3D shows doc-tests for rustc crates, but only some of them. Notably, at least rustc_borrowck is missing doctests.

Oh, it turns out all these crates set doctest = false 🤦

@reez12g can you remove this new job, and instead remove doctest = false from all compiler/ crates, then check that the llvm-12 PR job includes "Doc-tests rustc_borrowck"?

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2022
@reez12g
Copy link
Contributor Author

reez12g commented Sep 8, 2022

@jyn514
Thank you for your kind guide!
Do I just remove the doctest = false in line below?
OR change it to doctest = true? I'm not sure I can delete it.

% grep -r 'doctest = false' compiler
compiler/rustc_errors/Cargo.toml:doctest = false
compiler/rustc_resolve/Cargo.toml:doctest = false
compiler/rustc_symbol_mangling/Cargo.toml:doctest = false
compiler/rustc_query_system/Cargo.toml:doctest = false
compiler/rustc_index/Cargo.toml:doctest = false
compiler/rustc_infer/Cargo.toml:doctest = false
compiler/rustc_query_impl/Cargo.toml:doctest = false
compiler/rustc_hir/Cargo.toml:doctest = false
compiler/rustc_trait_selection/Cargo.toml:doctest = false
compiler/rustc_mir_transform/Cargo.toml:doctest = false
compiler/rustc_mir_transform/src/coverage/test_macros/Cargo.toml:doctest = false
compiler/rustc_error_messages/Cargo.toml:doctest = false
compiler/rustc_typeck/Cargo.toml:doctest = false
compiler/rustc_plugin_impl/Cargo.toml:doctest = false
compiler/rustc_data_structures/Cargo.toml:doctest = false
compiler/rustc_ast_pretty/Cargo.toml:doctest = false
compiler/rustc_attr/Cargo.toml:doctest = false
compiler/rustc_span/Cargo.toml:doctest = false
compiler/rustc_hir_pretty/Cargo.toml:doctest = false
compiler/rustc_incremental/Cargo.toml:doctest = false
compiler/rustc_mir_build/Cargo.toml:doctest = false
compiler/rustc_codegen_llvm/Cargo.toml:doctest = false
compiler/rustc_type_ir/Cargo.toml:doctest = false
compiler/rustc_const_eval/Cargo.toml:doctest = false
compiler/rustc_ast/Cargo.toml:doctest = false
compiler/rustc_metadata/Cargo.toml:doctest = false
compiler/rustc_middle/Cargo.toml:doctest = false
compiler/rustc_expand/Cargo.toml:doctest = false
compiler/rustc_monomorphize/Cargo.toml:doctest = false
compiler/rustc_parse/Cargo.toml:doctest = false
compiler/rustc_borrowck/Cargo.toml:doctest = false
compiler/rustc_mir_dataflow/Cargo.toml:doctest = false
compiler/rustc_lexer/Cargo.toml:doctest = false
compiler/rustc_ast_lowering/Cargo.toml:doctest = false
compiler/rustc_interface/Cargo.toml:doctest = false
compiler/rustc_builtin_macros/Cargo.toml:doctest = false
compiler/rustc_feature/Cargo.toml:doctest = false

@jyn514
Copy link
Member

jyn514 commented Sep 8, 2022

@reez12g yes, you can just delete it :) the default is true.

@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic labels Sep 11, 2022
@rust-log-analyzer

This comment has been minimized.

@reez12g reez12g changed the title Add test execution step to x86_64-gnu-aux Enable doctest in compiler/ crates Sep 11, 2022
@reez12g reez12g changed the title Enable doctest in compiler/ crates Enable doctests in compiler/ crates Sep 11, 2022
@jyn514
Copy link
Member

jyn514 commented Sep 11, 2022

Looks like the test failure is because the doctests are now running 🎉 @reez12g can you please fix those failures? You can run them locally with the command in #99324 (comment) (replacing --stage 2 with --stage 0 for better iteration times).

Once you do that, I think is ready to merge :)

@jyn514
Copy link
Member

jyn514 commented Sep 11, 2022

Oh, looks like this is still disabled for bootstrap - if it's easy to fix it would be nice to remove these too. But I won't block on that.

; git grep 'doctest = ' FETCH_HEAD
FETCH_HEAD:src/bootstrap/Cargo.toml:doctest = false
FETCH_HEAD:src/tools/rust-demangler/Cargo.toml:doctest = false

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

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.

This looks great so far :) just one question.

By the way, if this is more work than you were expecting, I'm happy to merge this for a subset of crates at first and we can fix the rest later.

compiler/rustc_borrowck/src/diagnostics/region_errors.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@reez12g reez12g left a comment

Choose a reason for hiding this comment

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

I would appreciate it if you merge this!
It looks like there are still some doctests that need to be fixed, so I agree to merge it for now and move on.

compiler/rustc_borrowck/src/diagnostics/region_errors.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Oct 1, 2022

I would appreciate it if you merge this! It looks like there are still some doctests that need to be fixed, so I agree to merge it for now and move on.

For us to be able to merge this, you'll need to re-enable doctest = false for the crates you haven't fixed. Can you please do that? As a bonus, it would be extremely helpful if you could post the list of crates with doctest=false on the tracking issue.

@rust-log-analyzer

This comment has been minimized.

@reez12g
Copy link
Contributor Author

reez12g commented Oct 5, 2022

@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: This is awaiting some action (such as code changes or more information) from the author. labels Oct 5, 2022
@reez12g
Copy link
Contributor Author

reez12g commented Oct 5, 2022

@jyn514

you'll need to re-enable doctest = false for the crates you haven't fixed.

I did it.
After this PR is merged, I will create a tracking issue to clean up the rest!

@jyn514
Copy link
Member

jyn514 commented Oct 6, 2022

Thanks! Please reuse #99144 for the tracking issue :)

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Oct 6, 2022

📌 Commit 488eb42 has been approved by jyn514

It is now in the queue for this repository.

@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 6, 2022
@bors
Copy link
Contributor

bors commented Oct 6, 2022

⌛ Testing commit 488eb42 with merge 0152393...

@bors
Copy link
Contributor

bors commented Oct 6, 2022

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 0152393 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 6, 2022
@bors bors merged commit 0152393 into rust-lang:master Oct 6, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 6, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0152393): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.4% [2.3%, 4.5%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-3.0%, -1.2%] 6
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

@reez12g reez12g deleted the issue-99144 branch October 6, 2022 08:29
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Enable doctests in compiler/ crates

Helps with rust-lang#99144
@reez12g reez12g mentioned this pull request Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants