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

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

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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

5 participants