Closed Oneirical closed 1 week ago
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
Some changes occurred in run-make tests.
cc @jieyouxu
@Oneirical @jieyouxu: Is it possible for these run-make migrations to be batched into PRs that migrate a handful at once, instead of migrating one at a time? I believe it would be easier for those who read the PRs page often (e.g. me), and I'm not sure if I see much value in having each one be a separate PR :)
@Oneirical @jieyouxu: Is it possible for these run-make migrations to be batched into PRs that migrate a handful at once, instead of migrating one at a time? I believe it would be easier for those who read the PRs page often (e.g. me), and I'm not sure if I see much value in having each one be a separate PR :)
I asked the same question, because it's true that these get a bit spammy... Nilstrieb answered me in Zulip's #gsoc:
making the PRS individually means that they can be reviewed, approved, rebased and kept track of individually, which is very valuable as it means one won't block another, while the overhead of reviewing two PRs instead of one is minimal
Kobzol:
I think that one PR per test is fine. Maybe if there is a bunch of very similar tests that could be ported at once, you could group these.
Even though a test may look really simple, sometimes, there are complications, like platform-specific discrepancies, tests not deserving run-make
status and needing to be sent to be UI tests, or ties with the run-make-support
library.
But I do agree that these are covering the PR frontpage rather intensely. Maybe it could be fine to reserve the single-PRs for tests that have some complexity (like #125165) or those that add new helper functions in the support library.
Sorry for the inconvenience, and I'll talk about this with the GSoC organizers to find a compromise that makes everyone happy!
@Oneirical @jieyouxu: Is it possible for these run-make migrations to be batched into PRs that migrate a handful at once, instead of migrating one at a time? I believe it would be easier for those who read the PRs page often (e.g. me), and I'm not sure if I see much value in having each one be a separate PR :)
Yes, we should batch easier migrations
The job mingw-check-tidy
failed! Check out the build log: (web) (plain)
The job x86_64-gnu-llvm-17
failed! Check out the build log: (web) (plain)
The job x86_64-gnu-llvm-17
failed! Check out the build log: (web) (plain)
Interesting error message on the CI fail:
--- stderr -------------------------------
error: unwinding panics are not supported without std
|
= help: using nightly cargo, use -Zbuild-std with panic="abort" to avoid unwinding
= note: since the core library is usually precompiled with panic="unwind", rebuilding your crate with panic="abort" may not be enough to fix the problem
The suggestion seems to be "you should build the standard library". But, this test is checking for successful panic unwinding when there is no standard library.
The test was made a long time ago, so is it possible support for panic unwinding without std was dropped all-together? That would make the test obsolete...
The test was made a long time ago, so is it possible support for panic unwinding without std was dropped all-together? That would make the test obsolete...
The reason is that by default UI tests and their auxilliaries when compiled by compiletest
sets a bunch of additional flags. I think it's okay if we don't try to force this to be a UI test and instead keep it as a run-make
test.
This is extremely similar to https://github.com/rust-lang/rust/pull/125146. Could it be interesting to merge the two in some way? This one seems to do the same thing as the https://github.com/rust-lang/rust/pull/125146, but with an added check that a useless lint is not shown.
IMO they're different enough, because #125146 is checking panic_handler
s can be provided by transitive dependencies, whereas this test is checking that the lint is not emitted.
@rustbot author
:umbrella: The latest upstream changes (presumably #125379) made this pull request unmergeable. Please resolve the merge conflicts.
The job mingw-check-tidy
failed! Check out the build log: (web) (plain)
@rustbot review
run-make
test restored.
this test is checking that the lint is not emitted.
Your review feedback about the explanatory test comment said that this test was not checking anything related to the lint. (otherwise there would be a CGREP looking for the lint text, and throwing a failure if it was detected). Did I misunderstand?
this test is checking that the lint is not emitted.
Your review feedback about the explanatory test comment said that this test was not checking anything related to the lint. (otherwise there would be a CGREP looking for the lint text, and throwing a failure if it was detected). Did I misunderstand?
Sorry I missed the #![deny(unused_extern_crates)]
in app.rs
, so compilation will fail if the lint is in fact fired.
Thanks, feel free to r=me after squashing commits into one. @bors delegate+
:v: @Oneirical, 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
Sorry I missed the
#![deny(unused_extern_crates)]
inapp.rs
, so compilation will fail if the lint is in fact fired.
Alright, I have re-edited the test's comment to highlight the fact that the test does check if the useless lint gets printed.
@bors r=@jieyouxu
:pushpin: Commit dd7e68b3566c9417eb13ba8b09c74481727ff977 has been approved by jieyouxu
It is now in the queue for this repository.
@bors rollup
Part of #121876 and the associated Google Summer of Code project.
This is extremely similar to #125146. Could it be interesting to merge the two in some way? This one seems to do the same thing as the #125146, but with an added check that a useless lint is not shown.