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

Support incremental in compiletest for non-incremental modes. #89101

Merged
merged 1 commit into from
Sep 26, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Sep 19, 2021

This adds first-class support for using incremental builds in non-incremental-mode tests. These tests previously manually passed -C incremental=tmp/foo which resulted in reusing the same tmp folder between runs. This means that these tests could fail whenever the on-disk incremental format changed (such as when updating one's local source tree). This changes it so that these tests can pass a // incremental-build header which instructs compiletest to create a set aside a dedicated incremental directory which will be cleared before the test starts to ensure it has a clean slate.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2021
@ehuss
Copy link
Contributor Author

ehuss commented Sep 19, 2021

There was prior discussion about this at https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Incremental.20ICE.20running.20rustc.20tests.20on.20origin.2Fmaster/near/251211217

It was suggested that these tests could be moved to the incremental directory, but I don't think that is possible because these are codegen tests which are tested differently. I'm not familiar with these tests, so maybe they can be moved, so let me know if an alternate solution would be preferred.

cc @michaelwoerister

@Mark-Simulacrum
Copy link
Member

I think we should also consider renaming the flag to "incremental" or "incremental-mode" -- I want to avoid accidentally reading it as similar to 'build-pass' or otherwise tying it specifically to build in that sense.

r=me with the docs and a possible rename, happy to leave it up to you which to choose -- or if you feel strongly about -build, that's also OK.

@Mark-Simulacrum Mark-Simulacrum 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 Sep 23, 2021
@ehuss ehuss force-pushed the compiletest-incremental-build branch from 6a1342a to 2da6e66 Compare September 23, 2021 19:17
@ehuss
Copy link
Contributor Author

ehuss commented Sep 23, 2021

Yea, I can see how that can be confusing with -build. I originally used just incremental, but then I noticed that a single English word can be accidentally picked up by a comment like this.

I went ahead and renamed it back to just incremental, since I think incremental-mode would be confusing with true incremental mode tests that support passes. I think the risk of accidentally making a test incremental is low (the existing examples all use -C incremental), and the consequences are low (making a test incremental usually shouldn't interfere with it).

I kinda wish the header parsing was a little more strict so that risk of accidental matches wasn't there.

Feel free to r- if that doesn't sound good, or if the comment I added isn't correct or clear.

@ehuss
Copy link
Contributor Author

ehuss commented Sep 23, 2021

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Sep 23, 2021

📌 Commit 2da6e66 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 23, 2021
@Mark-Simulacrum
Copy link
Member

Sounds great, thanks! Yeah, our header parsing story is not great :)

@bors
Copy link
Contributor

bors commented Sep 26, 2021

⌛ Testing commit 2da6e66 with merge ac8dd1b...

@bors
Copy link
Contributor

bors commented Sep 26, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing ac8dd1b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 26, 2021
@bors bors merged commit ac8dd1b into rust-lang:master Sep 26, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 26, 2021
@bors bors mentioned this pull request Sep 26, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ac8dd1b): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

jyn514 added a commit to jyn514/rust that referenced this pull request Jun 27, 2022
These used to be used by codegen-units tests, but were switched from manually specifying directories
to just using `// incremental` in rust-lang#89101.
Remove the old references.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2022
…acrum

Remove references to `./tmp` in-tree

These used to be used by codegen-units tests, but were switched from manually specifying directories
to just using `// incremental` in rust-lang#89101.
Remove the old references.

Fixes rust-lang#34586.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants