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

Migrate run-make/issue-53964 to rmake #125224

Merged
merged 1 commit into from
May 23, 2024
Merged

Migrate run-make/issue-53964 to rmake #125224

merged 1 commit into from
May 23, 2024

Conversation

Oneirical
Copy link
Contributor

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
Copy link
Collaborator

rustbot commented May 17, 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-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 17, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

@compiler-errors
Copy link
Member

compiler-errors commented May 17, 2024

@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
Copy link
Contributor Author

@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
Copy link
Member

@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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Oneirical Oneirical May 21, 2024

Choose a reason for hiding this comment

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

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.

@@ -0,0 +1,16 @@
// Defining a crate that provides panic handling as an external crate
// could uselessly trigger the "unused external crate" lint. This test
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@Oneirical Oneirical May 21, 2024

Choose a reason for hiding this comment

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

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.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Oneirical
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2024
@bors
Copy link
Contributor

bors commented May 21, 2024

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

@rust-log-analyzer

This comment has been minimized.

@Oneirical
Copy link
Contributor Author

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2024
@jieyouxu
Copy link
Member

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
Copy link
Member

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

@bors
Copy link
Contributor

bors commented May 22, 2024

✌️ @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
Copy link
Contributor Author

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
Copy link
Contributor Author

@bors r=@jieyouxu

@bors
Copy link
Contributor

bors commented May 22, 2024

📌 Commit dd7e68b 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 22, 2024
@jieyouxu
Copy link
Member

@bors rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2024
Migrate `run-make/issue-53964` to `rmake`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

This is extremely similar to rust-lang#125146. Could it be interesting to merge the two in some way? This one seems to do the same thing as the rust-lang#125146, but with an added check that a useless lint is not shown.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2024
Migrate `run-make/issue-30063` to `rmake`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

(Sorry about the [inconvenience](rust-lang#125224 (comment)) of all these PRs, this is the last one batched for today. I will discuss how we can cut these down a bit.)

The last check was previously commented out in the Makefile, and I have readded it. If it fails the CI, this can be reconsidered.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#125165 (Migrate `run-make/pgo-branch-weights` to `rmake`)
 - rust-lang#125210 (Cleanup: Fix up some diagnostics)
 - rust-lang#125224 (Migrate `run-make/issue-53964` to `rmake`)
 - rust-lang#125227 (Migrate `run-make/issue-30063` to `rmake`)
 - rust-lang#125383 (Rewrite `emit`, `mixing-formats` and `bare-outfile` `run-make` tests in `rmake.rs` format)
 - rust-lang#125401 (Migrate `run-make/rustdoc-scrape-examples-macros` to `rmake.rs`)
 - rust-lang#125409 (Rename `FrameworkOnlyWindows` to `RawDylibOnlyWindows`)
 - rust-lang#125416 (Use correct param-env in `MissingCopyImplementations`)
 - rust-lang#125421 (Rewrite `core-no-oom-handling`, `issue-24445` and `issue-38237` `run-make` tests to new `rmake.rs` format)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2024
Migrate `run-make/issue-53964` to `rmake`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

This is extremely similar to rust-lang#125146. Could it be interesting to merge the two in some way? This one seems to do the same thing as the rust-lang#125146, but with an added check that a useless lint is not shown.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2024
Migrate `run-make/issue-30063` to `rmake`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

(Sorry about the [inconvenience](rust-lang#125224 (comment)) of all these PRs, this is the last one batched for today. I will discuss how we can cut these down a bit.)

The last check was previously commented out in the Makefile, and I have readded it. If it fails the CI, this can be reconsidered.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#125210 (Cleanup: Fix up some diagnostics)
 - rust-lang#125224 (Migrate `run-make/issue-53964` to `rmake`)
 - rust-lang#125227 (Migrate `run-make/issue-30063` to `rmake`)
 - rust-lang#125383 (Rewrite `emit`, `mixing-formats` and `bare-outfile` `run-make` tests in `rmake.rs` format)
 - rust-lang#125401 (Migrate `run-make/rustdoc-scrape-examples-macros` to `rmake.rs`)
 - rust-lang#125409 (Rename `FrameworkOnlyWindows` to `RawDylibOnlyWindows`)
 - rust-lang#125416 (Use correct param-env in `MissingCopyImplementations`)
 - rust-lang#125421 (Rewrite `core-no-oom-handling`, `issue-24445` and `issue-38237` `run-make` tests to new `rmake.rs` format)
 - rust-lang#125438 (Remove unneeded string conversion)

r? `@ghost`
`@rustbot` modify labels: rollup
fmease added a commit to fmease/rust that referenced this pull request May 23, 2024
Migrate `run-make/issue-30063` to `rmake`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

(Sorry about the [inconvenience](rust-lang#125224 (comment)) of all these PRs, this is the last one batched for today. I will discuss how we can cut these down a bit.)

The last check was previously commented out in the Makefile, and I have readded it. If it fails the CI, this can be reconsidered.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#122382 (Detect unused structs which implement private traits)
 - rust-lang#124389 (Add a warning to proc_macro::Delimiter::None that rustc currently does not respect it.)
 - rust-lang#125224 (Migrate `run-make/issue-53964` to `rmake`)
 - rust-lang#125227 (Migrate `run-make/issue-30063` to `rmake`)
 - rust-lang#125336 (Add dedicated definition for intrinsics)
 - rust-lang#125401 (Migrate `run-make/rustdoc-scrape-examples-macros` to `rmake.rs`)
 - rust-lang#125454 (Improve the doc of query associated_item)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e282b1f into rust-lang:master May 23, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
Rollup merge of rust-lang#125227 - Oneirical:seventh, r=jieyouxu

Migrate `run-make/issue-30063` to `rmake`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

(Sorry about the [inconvenience](rust-lang#125224 (comment)) of all these PRs, this is the last one batched for today. I will discuss how we can cut these down a bit.)

The last check was previously commented out in the Makefile, and I have readded it. If it fails the CI, this can be reconsidered.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
Rollup merge of rust-lang#125224 - Oneirical:sixth, r=jieyouxu

Migrate `run-make/issue-53964` to `rmake`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

This is extremely similar to rust-lang#125146. Could it be interesting to merge the two in some way? This one seems to do the same thing as the rust-lang#125146, but with an added check that a useless lint is not shown.
@Oneirical Oneirical deleted the sixth branch May 30, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants