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.

Sign up for GitHub

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

Jump to bottom

Fix json flag in bootstrap doc #103851

Merged
merged 5 commits into from
Nov 6, 2022
Merged

Fix json flag in bootstrap doc #103851

merged 5 commits into from
Nov 6, 2022

Conversation

viandoxdev
Copy link
Contributor

@viandoxdev viandoxdev commented Nov 1, 2022 •
edited by jyn514
Loading

Fix the --json flag not working with x.py (Closes #103816)

While this works I'm not sure about the should_run of JsonStd, had to change it because

rust/src/bootstrap/builder.rs

Line 334 in ab5a2bc

let pathsets = should_run.pathset_for_paths_removing_matches(&mut paths, desc.kind);
would match with JsonStd and remove the paths that Std matched. So I did this but that looks more like a hack/workaround than anything. I'm guessing there's something to do with the default condition thing but idk how it works

@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2022

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

Please see the contribution instructions for more information.

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2022
@jyn514
Copy link
Member

jyn514 commented Nov 1, 2022

@viandoxdev I think 9791b9f is a more principled fix than your second commit, I would prefer to use that.

@viandoxdev
Copy link
Contributor Author

viandoxdev commented Nov 2, 2022 •
edited
Loading

9791b9f9826adea1078775afbaf66545b319b78d doesn't really fix the issue, I still need a way to not run the JsonStd steps when the flag isn't set. Currently just applying the changes of that commit makes it do both html and json regardless.

Looking at the code, from the get go if JsonStd is in Builder::get_step_descriptions, because it has the same paths as Std, it can't really be separated from Std, the ShouldRun::never works by simply removing the paths if the flag isn't set

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2022

Hmm, ok. In that case maybe we should make it the same Step as Std and call doc_std with the appropriate mode based on the flag, instead of having a separate step.

@viandoxdev
Copy link
Contributor Author

Hmm, ok. In that case maybe we should make it the same Step as Std and call doc_std with the appropriate mode based on the flag, instead of having a separate step.

did that

@viandoxdev
Copy link
Contributor Author

@jyn514 thoughts?

@jyn514
Copy link
Member

jyn514 commented Nov 2, 2022

@viandoxdev I don't have time to look at this right now, I'll make sure I let you know after I have a change to look.

jyn514
jyn514 approved these changes Nov 5, 2022
View reviewed changes
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 is very well done :) I hadn't thought to make format part of the Std struct itself, that's absolutely the right thing to do since it avoids breaking RustdocJSStd tests.

@jyn514
Copy link
Member

jyn514 commented Nov 5, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 5, 2022

📌 Commit 1bc2bfa 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 Nov 5, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 5, 2022
@matthiaskrgr
Rollup merge of rust-lang#103851 - viandoxdev:103816_bootstrap_fix_js…
8fd875d 
…on_doc, r=jyn514

Fix json flag in bootstrap doc

Fix the `--json` flag not working with x.py (Closes rust-lang#103816)

While this works I'm not sure about the `should_run` of `JsonStd`, had to change it because https://github.com/rust-lang/rust/blob/ab5a2bc7316012ee9b2a4a4f3821673f2677f3d5/src/bootstrap/builder.rs#L334 would match with JsonStd and remove the paths that Std matched. So I did [this](https://github.com/viandoxdev/rust/blob/ffd4078264c4892b5098d6191e0adfe3564d62ca/src/bootstrap/doc.rs#L526-L534) but that looks more like a hack/workaround than anything. I'm guessing there's something to do with the default condition thing but idk how it works
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 5, 2022
Closed
@matthiaskrgr
Copy link
Member

probably failed in #104010 (comment) but I'm not entierly sure
@bors rollup=iffy

@viandoxdev
Copy link
Contributor Author

Should be fixed by 900af41, but not sure how to test @matthiaskrgr

@jyn514
Copy link
Member

jyn514 commented Nov 5, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Nov 5, 2022

📌 Commit 900af41 has been approved by jyn514

It is now in the queue for this repository.

@jyn514
Copy link
Member

jyn514 commented Nov 5, 2022

@bors rollup=maybe

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 6, 2022
@matthiaskrgr
Rollup merge of rust-lang#103851 - viandoxdev:103816_bootstrap_fix_js…
2e92f66 
…on_doc, r=jyn514

Fix json flag in bootstrap doc

Fix the `--json` flag not working with x.py (Closes rust-lang#103816)

While this works I'm not sure about the `should_run` of `JsonStd`, had to change it because https://github.com/rust-lang/rust/blob/ab5a2bc7316012ee9b2a4a4f3821673f2677f3d5/src/bootstrap/builder.rs#L334 would match with JsonStd and remove the paths that Std matched. So I did [this](https://github.com/viandoxdev/rust/blob/ffd4078264c4892b5098d6191e0adfe3564d62ca/src/bootstrap/doc.rs#L526-L534) but that looks more like a hack/workaround than anything. I'm guessing there's something to do with the default condition thing but idk how it works
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 6, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 6, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 6, 2022
@bors
Auto merge of rust-lang#104043 - matthiaskrgr:rollup-sttf9e8, r=matth…
88935e0 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#103012 (Suggest use .. to fill in the rest of the fields of Struct)
 - rust-lang#103851 (Fix json flag in bootstrap doc)
 - rust-lang#103990 (rustdoc: clean up `.logo-container` layout CSS)
 - rust-lang#104002 (fix a comment in UnsafeCell::new)
 - rust-lang#104014 (Migrate test-arrow to CSS variables)
 - rust-lang#104016 (Add internal descriptions to a few queries)
 - rust-lang#104035 (Add 'closure match' test to weird-exprs.rs.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 131ef95 into rust-lang:master Nov 6, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.

x doc --json flag not working
6 participants
@viandoxdev @rustbot @jyn514 @bors @matthiaskrgr @Mark-Simulacrum