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

Advise against removing the remaining Python scripts from tests/run-make #129387

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

Zalathar
Copy link
Contributor

After some recent PRs (e.g. #129185), there are only two Python scripts left in tests/run-make.

Having come so far, it's tempting to try to get rid of the remaining ones. But after trying that myself, I've come to the conclusion that it's not worth the extra hassle, especially if it means pulling in an XML-parsing crate just for one test.

This PR therefore leaves behind a few signpost comments to explain why getting rid of these particular scripts has low value.

This makes it easier for maintainers to see what the Python script is for.
Trying to get rid of this Python script looks tempting, because it's currently
the only Python script in the whole `run-make` suite that we actually run.

But getting rid of it would require pulling in a Rust crate to parse XML
instead, and that's probably not worth the extra hassle for a relatively-minor
test.
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

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.

Thanks!

@jieyouxu
Copy link
Member

Feel free to r=me once PR CI is green.
@bors delegate+ rollup

@bors
Copy link
Contributor

bors commented Aug 22, 2024

✌️ @Zalathar, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@Zalathar
Copy link
Contributor Author

PR CI is green.

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Aug 22, 2024

📌 Commit 34cdfc9 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 Aug 22, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 22, 2024
Advise against removing the remaining Python scripts from `tests/run-make`

After some recent PRs (e.g. rust-lang#129185), there are only two Python scripts left in `tests/run-make`.

Having come so far, it's tempting to try to get rid of the remaining ones. But after trying that myself, I've come to the conclusion that it's not worth the extra hassle, especially if it means pulling in an XML-parsing crate just for one test.

This PR therefore leaves behind a few signpost comments to explain why getting rid of these particular scripts has low value.
@Zalathar Zalathar mentioned this pull request Aug 22, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128432 (WASI: forbid `unsafe_op_in_unsafe_fn` for `std::{os, sys}`)
 - rust-lang#129373 (Add missing module flags for CFI and KCFI sanitizers)
 - rust-lang#129374 (Use `assert_unsafe_precondition!` in `AsciiChar::digit_unchecked`)
 - rust-lang#129376 (Change `assert_unsafe_precondition` docs to refer to `check_language_ub`)
 - rust-lang#129382 (Add `const_cell_into_inner` to `OnceCell`)
 - rust-lang#129387 (Advise against removing the remaining Python scripts from `tests/run-make`)
 - rust-lang#129388 (Do not rely on names to find lifetimes.)
 - rust-lang#129395 (Pretty-print own args of existential projections (dyn-Trait w/ GAT constraints))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d24e6b7 into rust-lang:master Aug 22, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2024
Rollup merge of rust-lang#129387 - Zalathar:python-apologia, r=jieyouxu

Advise against removing the remaining Python scripts from `tests/run-make`

After some recent PRs (e.g. rust-lang#129185), there are only two Python scripts left in `tests/run-make`.

Having come so far, it's tempting to try to get rid of the remaining ones. But after trying that myself, I've come to the conclusion that it's not worth the extra hassle, especially if it means pulling in an XML-parsing crate just for one test.

This PR therefore leaves behind a few signpost comments to explain why getting rid of these particular scripts has low value.
@Zalathar Zalathar deleted the python-apologia branch August 22, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants