Skip to content

Add CI check to ensure that rustdoc JSON FORMAT_VERSION is correctly updated #142677

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add CI check to ensure that rustdoc JSON FORMAT_VERSION is correctly updated #142677

wants to merge 3 commits into from
+95 −1

Conversation

GuillaumeGomez
Copy link
Member

Follow-up of #142601.

Tested it locally with: BASE_COMMIT=1bb335244c311a07cee165c28c553c869e6f64a9 src/ci/docker/host-x86_64/mingw-check-1/validate-rustdoc-json-format-version-update.sh (where BASE_COMMIT value was the commit before I made a wrong change with the FORMAT_VERSION update).

This is a first version. I plan to send a follow-up to also ensure that FORMAT_VERSION is updated if any code change is done in rustdoc-json-types. For that I just need to check that a line not starting with / and not an empty line was updated. Fun times with grep ahead. :')

cc @aDotInTheVoid
r? @nnethercote

@rustbot
Copy link
Collaborator

rustbot commented Jun 18, 2025

nnethercote is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 18, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Jun 18, 2025

Could this be a tidy check instead? It's better to have these things in one centralized place in Rust, rather than scattered around in random bash scripts.

@GuillaumeGomez
Copy link
Member Author

I guess? Didn't even realize that I could do the same in rust much more easily. ^^'

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez force-pushed the check-format-version-update branch from 23e5243 to 5d9e058 Compare June 18, 2025 15:29
@rustbot rustbot added A-tidy Area: The tidy tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 18, 2025
@GuillaumeGomez
Copy link
Member Author

Moved it to tidy. I'll do the same with the other bash tidy check I added.

@GuillaumeGomez
Copy link
Member Author

Oh also:

r? @Kobzol

@rustbot rustbot assigned Kobzol and unassigned nnethercote Jun 18, 2025
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jun 18, 2025
Merged
Kobzol
Kobzol reviewed Jun 19, 2025
View reviewed changes
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

I guess it could also work locally, but it's always a PITA to figure out a diff vs master, so this is fine. Could you please try to break the CI job in this PR to see if it works on PR CI?

@GuillaumeGomez GuillaumeGomez force-pushed the check-format-version-update branch from 5d9e058 to 43d18cf Compare June 19, 2025 11:22
@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Jun 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2025

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

@Kobzol
Copy link
Contributor

Kobzol commented Jun 19, 2025 

2025-06-19T11:27:57.5354234Z [TIMING] core::build_steps::tool::Tidy { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu } -- 0.000
2025-06-19T11:27:57.5354975Z fmt check
2025-06-19T11:28:04.5397272Z fmt: checked 6074 files
2025-06-19T11:28:05.0397217Z tidy check
2025-06-19T11:28:24.2535092Z Running eslint on rustdoc JS files
2025-06-19T11:28:29.4054908Z tidy: Skipping binary file check, read-only filesystem
2025-06-19T11:28:36.0337263Z removing old virtual environment
2025-06-19T11:28:36.0359346Z creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'venv'
2025-06-19T11:28:36.0843019Z creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'virtualenv'
2025-06-19T11:28:37.6050421Z Requirement already satisfied: pip in ./build/venv/lib/python3.10/site-packages (25.1.1)
2025-06-19T11:28:38.8879878Z linting python files
2025-06-19T11:28:39.0477863Z All checks passed!
2025-06-19T11:28:39.0488259Z checking python file formatting
2025-06-19T11:28:39.1812151Z 28 files already formatted
2025-06-19T11:28:39.1822499Z checking C++ file formatting
2025-06-19T11:28:41.0313008Z x.py completions check

Looks like the check didn't run, hmm. Probably adding some logging before the return; statements would be useful, to figure out why. And also printing the output of git diff.

@GuillaumeGomez
Copy link
Member Author

It's because BASE_COMMIT is not set in tidy. Changing that.

@GuillaumeGomez GuillaumeGomez force-pushed the check-format-version-update branch from 76cea70 to 122cf96 Compare June 19, 2025 15:04
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez force-pushed the check-format-version-update branch from 122cf96 to c352ab5 Compare June 19, 2025 15:12
aDotInTheVoid
aDotInTheVoid reviewed Jun 19, 2025
View reviewed changes
Copy link
Member

@aDotInTheVoid aDotInTheVoid left a comment

Choose a reason for hiding this comment

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

Conceptually this change makes sense and is good.

This is a first version. I plan to send a follow-up to also ensure that FORMAT_VERSION is updated if any code change is done in rustdoc-json-types. For that I just need to check that a line not starting with / and not an empty line was updated.

I worry that this could still have false positives. E.g. #142642 touches non-comments in src/rustdoc-json-types, but doesn't need to bump the FORMAT_VERSION

src/rustdoc-json-types/lib.rs
@@ -38,7 +38,7 @@ pub type FxHashMap<K, V> = HashMap<K, V>; // re-export for use in src/librustdoc
// are deliberately not in a doc comment, because they need not be in public docs.)
//
// Latest feature: Pretty printing of inline attributes changed
pub const FORMAT_VERSION: u32 = 48;
pub const FORMAT_VERSION: u32 = 49;
Copy link
Member

Choose a reason for hiding this comment

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

This 48->49 change should be reverted before merging.

Kobzol
Kobzol reviewed Jun 19, 2025
View reviewed changes
src/tools/tidy/src/main.rs
}
// If first commit is from `[email protected]`, then we need to skip it as it means we're in
// the CI and it's the merge commit generated to test the PR.
let output = Command::new("git")
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is much more complicated than it might seem. Could you instead reuse

rust/src/build_helper/src/git.rs

Line 201 in a4b9a1b

fn get_closest_upstream_commit(
? This is more complicated than it might seem 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

@aDotInTheVoid aDotInTheVoid aDotInTheVoid left review comments

Assignees

@Kobzol Kobzol

Labels
A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GuillaumeGomez @rustbot @Kobzol @rust-log-analyzer @aDotInTheVoid @nnethercote