Skip to content

Allow to pass a full path for run-make tests #128100

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

GuillaumeGomez
Copy link
Member

It's common (at least for me) to pass a full path to a run-make test (including the rmake.rs file) and to see that it isn't found, which is a bit frustrating.

With these changes, we can now optionally pass the rmake.rs (or even Makefile) at the end of the path.

cc @jieyouxu
r? @Kobzol

@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 Jul 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 23, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@jieyouxu
Copy link
Member

Oh yeah I've been annoyed by this as well, nice!

@Kobzol
Copy link
Member

Kobzol commented Jul 23, 2024

I like the idea, but I'm a bit scared of making compiletest changes unnecessarily. Could this be implemented via the inverse logic, i.e. if we get a rmake.rs/Makefile file path, then take its directory, and then just use the original Makefile logic going further?

@GuillaumeGomez
Copy link
Member Author

I went for that originally. That means updating the filters, iterate over them and for each of them, check if the path starts with run-make then tests and ends with rmake.rs or Makefile. I'm fine with this approach too so gonna implement it.

@Kobzol
Copy link
Member

Kobzol commented Jul 23, 2024

Oh, if you specify a filepath, it won't automatically go here? In that case it's not as simple as I thought 😆 But still I probably prefer the directory handling, in my view the test is the directory, and rmake.rs/Makefile is an implementation detail.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jul 23, 2024

I think it doesn't do what you think it does. 😆

@GuillaumeGomez
Copy link
Member Author

I went for the filter update instead. Much simpler, should have gone for this one instead... What do you think of this approach?

@Kobzol
Copy link
Member

Kobzol commented Jul 23, 2024

I don't know a lot about compiletest, but this looks better than before, and it works for me.

I wonder how UI tests do this though. If I do x test tests/ui/associated-item, it runs all tests within that directory, if I do x test tests/ui/associated-item/associated-item-enum.rs, it runs just that one test. I suppose that this happens on a different place than here (?).

Anyway, feel free to r=me, unless @jieyouxu has any objections.

@GuillaumeGomez
Copy link
Member Author

It's actually run-make that is different here. We check if the folder has a Makefile or a rmake.rs then add it to the path.

@jieyouxu
Copy link
Member

jieyouxu commented Jul 23, 2024

Yeah, rmake.rs not accepting the exact path was just a quirk in my original implementation of it, this part of the test filtering has not been changed since I added it initially. It was more of a "initial working version".

@GuillaumeGomez
Copy link
Member Author

Prototype -> prod, oh well, we all know the drill. 😆

Then let's go!

@bors r=Kobzol,jieyouxu rollup

@bors
Copy link
Collaborator

bors commented Jul 23, 2024

📌 Commit 0728c15 has been approved by Kobzol,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 Jul 23, 2024
@jieyouxu
Copy link
Member

Prototype -> prod, oh well, we all know the drill. 😆

The original rmake.rs infra PR failed bors full build like 20 times, im just glad it ever passed lol

@GuillaumeGomez
Copy link
Member Author

You should give a talk or write a blog post about "Migrating run-make, the ugly and terrible" haha

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 23, 2024
…bzol,jieyouxu

Allow to pass a full path for `run-make` tests

It's common (at least for me) to pass a full path to a `run-make` test (including the `rmake.rs` file) and to see that it isn't found, which is a bit frustrating.

With these changes, we can now optionally pass the `rmake.rs` (or even `Makefile`) at the end of the path.

cc `@jieyouxu`
r? `@Kobzol`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 23, 2024
…bzol,jieyouxu

Allow to pass a full path for `run-make` tests

It's common (at least for me) to pass a full path to a `run-make` test (including the `rmake.rs` file) and to see that it isn't found, which is a bit frustrating.

With these changes, we can now optionally pass the `rmake.rs` (or even `Makefile`) at the end of the path.

cc ``@jieyouxu``
r? ``@Kobzol``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#124895 (Disallow hidden references to mutable static)
 - rust-lang#128043 (Docs for core::primitive: mention that "core" can be shadowed, too, so we should write "::core")
 - rust-lang#128092 (Remove wrapper functions from c.rs)
 - rust-lang#128100 (Allow to pass a full path for `run-make` tests)
 - rust-lang#128106 (Fix return type of FileAttr methods on AIX target)
 - rust-lang#128108 (ensure std step before preparing sysroot)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125962 (Update tracking issue for `const_binary_heap_new_in`)
 - rust-lang#126770 (Add elem_offset and related methods)
 - rust-lang#127481 (Remove generic lifetime parameter of trait `Pattern`)
 - rust-lang#128043 (Docs for core::primitive: mention that "core" can be shadowed, too, so we should write "::core")
 - rust-lang#128092 (Remove wrapper functions from c.rs)
 - rust-lang#128100 (Allow to pass a full path for `run-make` tests)
 - rust-lang#128106 (Fix return type of FileAttr methods on AIX target)
 - rust-lang#128108 (ensure std step before preparing sysroot)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 20e86c9 into rust-lang:master Jul 24, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024
Rollup merge of rust-lang#128100 - GuillaumeGomez:run-make-path, r=Kobzol,jieyouxu

Allow to pass a full path for `run-make` tests

It's common (at least for me) to pass a full path to a `run-make` test (including the `rmake.rs` file) and to see that it isn't found, which is a bit frustrating.

With these changes, we can now optionally pass the `rmake.rs` (or even `Makefile`) at the end of the path.

cc ```@jieyouxu```
r? ```@Kobzol```
@GuillaumeGomez GuillaumeGomez deleted the run-make-path branch July 24, 2024 08:24
# for free to join this conversation on GitHub. Already have an account? # 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.

5 participants