-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Migrate run-make/rustdoc-map-file
to rmake
#124837
Conversation
Some changes occurred in run-make tests. cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
913f83f
to
bbe7f05
Compare
This comment has been minimized.
This comment has been minimized.
bbe7f05
to
1a06635
Compare
This comment has been minimized.
This comment has been minimized.
1a06635
to
1128453
Compare
Fixed formatting. |
let python = std::env::var("PYTHON").unwrap_or("python".into()); | ||
assert!( | ||
Command::new(python).arg("validate_json.py").arg(&out_dir).status().unwrap().success() | ||
); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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.
1128453
to
4128e7d
Compare
I added the FIXME for the time being. When I'm back from the rustNL conference, I'll port the python script as well. |
Thanks, feel free to r=me after CI is green 💚. |
This comment has been minimized.
This comment has been minimized.
4128e7d
to
c078a44
Compare
@bors r=jieyouxu rollup |
…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
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`
Part of #121876.
r? @jieyouxu