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

Rewrite suspicious-library, resolve-rename and incr-prev-body-beyond-eof run-make tests in rmake.rs format #125683

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

Oneirical
Copy link
Contributor

@Oneirical Oneirical commented May 28, 2024

Part of #121876 and the associated Google Summer of Code project.

Some oddly specific ignore flags in incr-prev-body-beyond-eof:

// ignore-none
// ignore-nvptx64-nvidia-cuda

it could be interesting to run a try job, but it seems there is no nvidia-cuda in the CI settings (jobs.yml).

try-job: armhf-gnu

@rustbot
Copy link
Collaborator

rustbot commented May 28, 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 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 28, 2024

The run-make-support library was changed

cc @jieyouxu

Some changes occurred in run-make tests.

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Contributor

@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 28, 2024
.run();
fs::copy("b.rs", tmp_dir().join("main.rs"));
rustc()
.incremental(tmp_dir().join("incr"))
Copy link
Contributor

Choose a reason for hiding this comment

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

The tmp_dir().join(...) pattern seems to be repeating quite often. It could be worth it to introduce something like fn tmp_path(path: &str) to simplify it, it's a bit hard to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a helper, but in a separate PR (unrelated changes), I think it's fine for this PR to keep the join for now, I'll send another PR, so this PR can either rebase on top of that or I'll update the other PR to cover join in this PR too.

tests/run-make/incr-prev-body-beyond-eof/rmake.rs Outdated Show resolved Hide resolved
@Oneirical Oneirical force-pushed the patience-testing-test branch 2 times, most recently from cd21022 to a24ac15 Compare May 29, 2024 15:46
@Oneirical
Copy link
Contributor Author

Oneirical commented May 29, 2024

Changes made, keeping things as minimal as possible (unwrap() and just adding the missing run()). However, the points brought up (#[must_use] tags and tmp_path functions) would be very nice. I often have to check that I put my run() everywhere and it's so easy to forget.
@rustbot review

@Kobzol
Copy link
Contributor

Kobzol commented May 29, 2024

@bors review

I think that you might have meant @rustbot ready 😅

@jieyouxu
Copy link
Contributor

@bors review

I think that you might have meant @rustbot ready 😅

I derailed this PR by writing @bors author and being confused for 2 minutes why nothing is happening... I wish bors produced a warning message on unrecognized command :3

@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 29, 2024
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

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

I have some questions for the beyond-eof test

@rust-log-analyzer

This comment has been minimized.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 1, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Jun 1, 2024

(Added armhf-gnu to try jobs on this PR, so that we can test a potential fix in a try job.)

@Oneirical
Copy link
Contributor Author

Oneirical commented Jun 4, 2024

Looked up some past instances of this error, and found #28924.

The issue seems to be related to using the incorrect linker for the armhf-gnu platform, caused by --target $(TARGET).

As an experiment, I removed the --target flags. Why were they there in the first place? They don't seem linked to what the test does, and the purpose of --target is to compile for a platform that isn't the $(TARGET), is it not?

If that's not acceptable, it might be interesting to add a conditional check to set the appropriate linker for each failing architecture.

Running a try job once CI is green.

@rust-log-analyzer

This comment has been minimized.

@Oneirical
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Jun 4, 2024

⌛ Trying commit 59e2074 with merge ba0d297...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
…try>

Rewrite `suspicious-library`, `resolve-rename` and `incr-prev-body-beyond-eof` `run-make` tests in `rmake.rs` format

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

Some oddly specific ignore flags in `incr-prev-body-beyond-eof`:

```rs
// ignore-none
// ignore-nvptx64-nvidia-cuda
```

it could be interesting to run a try job, but it seems there is no nvidia-cuda in the CI settings (`jobs.yml`).

try-job: armhf-gnu
@bors
Copy link
Contributor

bors commented Jun 4, 2024

☀️ Try build successful - checks-actions
Build commit: ba0d297 (ba0d29759bc7ed6a809a44d0df52d68077160c0b)

@Oneirical
Copy link
Contributor Author

@rustbot review

Seems to be working, let me know if removing --target was unwise.

@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 Jun 5, 2024
@jieyouxu
Copy link
Contributor

jieyouxu commented Jun 5, 2024

As an experiment, I removed the --target flags. Why were they there in the first place? They don't seem linked to what the test does, and the purpose of --target is to compile for a platform that isn't the $(TARGET), is it not?

It would matter if the test artifacts are being cross-compiled and ran on a different target. For the purposes of this test, I think we could add a //@ ignore-cross-compile if it causes any issues because I feel like we're not really testing cross-compilation here.

@jieyouxu
Copy link
Contributor

jieyouxu commented Jun 5, 2024

Good work! @bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 5, 2024

📌 Commit 59e2074 has been approved by jieyouxu

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 5, 2024

🌲 The tree is currently closed for pull requests below priority 101. This pull request will be tested once the tree is reopened.

@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 Jun 5, 2024
@workingjubilee
Copy link
Contributor

mmmm gonna have to disagree here
@bors rollup=iffy

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#123168 (Add `size_of` and `size_of_val` and `align_of` and `align_of_val` to the prelude)
 - rust-lang#125273 (bootstrap: implement new feature `bootstrap-self-test`)
 - rust-lang#125683 (Rewrite `suspicious-library`, `resolve-rename` and `incr-prev-body-beyond-eof` `run-make` tests in `rmake.rs` format)
 - rust-lang#125815 (`rustc_parse` top-level cleanups)
 - rust-lang#125903 (rustc_span: Inline some hot functions)
 - rust-lang#125906 (Remove a bunch of redundant args from `report_method_error`)
 - rust-lang#125920 (Allow static mut definitions with #[linkage])
 - rust-lang#125982 (Make deleting on LinkedList aware of the allocator)
 - rust-lang#125995 (Use inline const blocks to create arrays of `MaybeUninit`.)
 - rust-lang#125996 (Closures are recursively reachable)
 - rust-lang#126003 (Add a co-maintainer for the two ARMv4T targets)
 - rust-lang#126004 (Add another test for hidden types capturing lifetimes that outlive but arent mentioned in substs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0f2b34a into rust-lang:master Jun 5, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup merge of rust-lang#125683 - Oneirical:patience-testing-test, r=jieyouxu

Rewrite `suspicious-library`, `resolve-rename` and `incr-prev-body-beyond-eof` `run-make` tests in `rmake.rs` format

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

Some oddly specific ignore flags in `incr-prev-body-beyond-eof`:

```rs
// ignore-none
// ignore-nvptx64-nvidia-cuda
```

it could be interesting to run a try job, but it seems there is no nvidia-cuda in the CI settings (`jobs.yml`).

try-job: armhf-gnu
@Oneirical Oneirical deleted the patience-testing-test branch June 5, 2024 14:20
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants