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

Migrate run-make/rustdoc-map-file to rmake #124837

Merged
merged 1 commit into from
May 9, 2024
Merged

Migrate run-make/rustdoc-map-file to rmake #124837

merged 1 commit into from
May 9, 2024

Conversation

GuillaumeGomez
Copy link
Member

Part of #121876.

r? @jieyouxu

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 7, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Fixed formatting.

jieyouxu
jieyouxu reviewed May 8, 2024
View reviewed changes
tests/run-make/rustdoc-map-file/rmake.rs Outdated
Comment on lines 12 to 16
let python = std::env::var("PYTHON").unwrap_or("python".into());
assert!(
Command::new(python).arg("validate_json.py").arg(&out_dir).status().unwrap().success()
);
Copy link
Member

Choose a reason for hiding this comment

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

This is very amusing to me. Let me go check what validate_json.py does.

Copy link
Member

@jieyouxu jieyouxu May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should this Python script be ported over? I'm okay with not blocking this PR and make it preserve the old behavior of calling out to a Python script initially. But now that we can write arbitrary checks in Rust, we probably should avoid relying on external dependencies (esp. Python with its various version funny business) if it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/rust-lang/rust/blob/e3029d220f6843f9b83d35927ff20a86137d7ed0/tests/run-make/rustdoc-map-file/validate_json.py

jieyouxu
jieyouxu approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

The ported test looks reasonable to me. The Python script feels a bit wonky as an external dependency but the ported test merely preserves the behavior of the existing test.

I'm okay with not porting the Python script over for now, but imo we should leave a FIXME in that case.

Feel free to r=me after either:

  • porting the validate_json.py script over, or
  • adding a FIXME to remind porting the Python script over as well.

@GuillaumeGomez
Copy link
Member Author

I added the FIXME for the time being. When I'm back from the rustNL conference, I'll port the python script as well.

@jieyouxu
Copy link
Member

jieyouxu commented May 8, 2024

Thanks, feel free to r=me after CI is green 💚.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

@bors r=jieyouxu rollup

@bors
Copy link
Contributor

bors commented May 8, 2024

📌 Commit c078a44 has been approved by jieyouxu

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 May 8, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request May 9, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2024
@bors
Auto merge of rust-lang#124916 - matthiaskrgr:rollup-vmpmt4u, r=matth…
5f8c17d 
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#124777 (Fix Error Messages for `break` Inside Coroutines)
 - rust-lang#124837 (Migrate `run-make/rustdoc-map-file` to rmake)
 - rust-lang#124875 (Fix more ICEs in `diagnostic::on_unimplemented`)
 - rust-lang#124908 (Handle field projections like slice indexing in invalid_reference_casting)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4510ee3 into rust-lang:master May 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2024
@rust-timer
Unrolled build for rust-lang#124837
50c310a 
Rollup merge of rust-lang#124837 - GuillaumeGomez:migrate-rustdoc-map-file, r=jieyouxu

Migrate `run-make/rustdoc-map-file` to rmake

Part of rust-lang#121876.

r? `@jieyouxu`
@GuillaumeGomez GuillaumeGomez deleted the migrate-rustdoc-map-file branch May 9, 2024 12:27
@jieyouxu jieyouxu mentioned this pull request Jun 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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
Oxidizing run-make tests
Status: Done
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.
5 participants
@GuillaumeGomez @rustbot @rust-log-analyzer @jieyouxu @bors