Migrate run-make/issue-53964 to rmake

[Oneirical]

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.

[rustbot]

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]

Some changes occurred in run-make tests.

cc @jieyouxu

[compiler-errors]

@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]

@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!

[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 :)

Yes, we should batch easier migrations

[jieyouxu]

This test can probably be rewritten as a ui test with an aux crate that's compiled with //@ compile-flags: -Cpanic=abort --emit=obj

[Oneirical]

I attempted to create this in my last commit. library_search_path did not come over to the UI test, I imagine the extern crate panic does the job on its own? EDIT: I found aux-build in the documentation, trying that instead.

[jieyouxu]

Is this lint actually even being checked in the original test (and thus this test)? There's no deny for this lint, either in source or as a -D unused_extern_crate cli flag, so I don't think it's actually being checked is it?

[Oneirical]

You're right, similarly to my last incorrect comment, it's not checking that the patch fixed something, but rather that it did not break something else. It's important to remember.

[Oneirical]

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...

[jieyouxu]

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 #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.

IMO they're different enough, because #125146 is checking panic_handlers can be provided by transitive dependencies, whereas this test is checking that the lint is not emitted.

[jieyouxu]

@rustbot author

[bors]

☔ The latest upstream changes (presumably #125379) made this pull request unmergeable. Please resolve the merge conflicts.

[Oneirical]

@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?

[jieyouxu]

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.

[jieyouxu]

Thanks, feel free to r=me after squashing commits into one.
@bors delegate+

[bors]

✌️ @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

[Oneirical]

Sorry I missed the #![deny(unused_extern_crates)] in app.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.

[Oneirical]

@bors r=@jieyouxu

[bors]

📌 Commit dd7e68b has been approved by jieyouxu

It is now in the queue for this repository.

[jieyouxu]

@bors rollup