compiletest: Run revisions as independent tests. #50400

ehuss · 2018-05-02T23:34:41Z

The output of each test is now in its own directory.
"auxiliary" output is now under the respective test directory.
stage_id removed from filenames, and instead placed in the stamp file as a hash. This helps keep path lengths down for Windows.

In brief, the new layout looks like this:

<build_base>/<relative_dir>/<testname>.<revision>.<mode>/
    stamp
    <testname>.err
    <testname>.out
    a (binary)
    auxiliary/lib<auxname>.dylib
    auxiliary/<auxname>/<auxname>.err
    auxiliary/<auxname>/<auxname>.out

(revision and mode are optional)

rust-highfive · 2018-05-02T23:34:52Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

ehuss · 2018-05-02T23:35:48Z

Note: It might be easier to review if you look at the diff for the commit after the rustfmt commit.

Concerns:

Are there any tools (besides update-references) that depend on the directory layout of the test output?
How do I enable wasm32-unknown-unknown? How do I run the tests in test/run-make?
Why was canonicalize() used in stamp()? Why was it not used elsewhere?
Should the hasher be changed? I noticed Cargo uses SipHasher24, but that is deprecated?
I wanted to remove the duplicate logic for paths, but it all ended up in common.rs. I'd like to move it all into TestCx, but that would require TestCx being created earlier for the "stamp" check. This patch was already getting a little big, so I held off. That could be more elegant.
Changing the binary name to a might be surprising? An alternative is to rename some of the very long test names. Here is an example of a very long path that causes problems if you use the full test name: d:\Proj\rust\rust\build\x86_64-pc-windows-msvc\test\ui\nll\closure-requirements\region-lbr1-does-outlive-lbr2-because-implied-bound\region-lbr1-does-outlive-lbr2-because-implied-bound.region_lbr1_does_outlive_lbr2_because_implied_bound0-317d481089b8c8fe83113de504472633.rs.rcgu.o

bors · 2018-05-04T17:50:34Z

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

bors · 2018-05-05T01:33:38Z

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

bors · 2018-05-07T11:14:37Z

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

nikomatsakis

This looks really nice! It feels like the path handling is much cleaner than before, even if it's still a bit messy.

nikomatsakis · 2018-05-09T14:37:33Z

src/tools/compiletest/src/runtest.rs

-    fn output_base_name(&self) -> PathBuf {
-        let dir = self.config.build_base.join(&self.testpaths.relative_dir);
+    /// The revision, ignored for Incremental since it wants all revisions in
+    /// the same directory.


All these special cases definitely make me think that separating out the incremental phases from revisions would be a good idea -- but maybe in a follow-up PR.

nikomatsakis · 2018-05-09T14:40:13Z

@ehuss

Are there any tools (besides update-references) that depend on the directory layout of the test output?

Don't think so. @alexcrichton couldn't think of any either. Maybe someone from @rust-lang/infra has thoughts though.

How do I enable wasm32-unknown-unknown? How do I run the tests in test/run-make?

I don't really know about this.

Why was canonicalize() used in stamp()? Why was it not used elsewhere?

I don't know this either. There may not be a reason? Maybe check the git blame and see who put those stamps in...

Should the hasher be changed? I noticed Cargo uses SipHasher24, but that is deprecated?

What is hasher used for exactly?

I wanted to remove the duplicate logic for paths, but it all ended up in common.rs. I'd like to move it all into TestCx, but that would require TestCx being created earlier for the "stamp" check. This patch was already getting a little big, so I held off. That could be more elegant.

OK -- how it is still seems more centralized than before. I like the "one directory per test" etc.

Changing the binary name to a might be surprising?

Maybe, but I don't think it matters.

nikomatsakis · 2018-05-09T14:40:36Z

I'd be happy to land this as is -- does anyone from @rust-lang/infra want to take a look too?

ehuss · 2018-05-09T15:36:27Z

Maybe check the git blame and see who put those stamps in...

It was @alexcrichton in #39431 (c8e0d04).

Hmm, just noticed #40578 which changed the stamp name for ecryptfs which says it has a 145 character limit. We're way over that now, so maybe that isn't something that can be realistically handled now?

What is hasher used for exactly?

Right now it just hashes the stage ID (instead of placing it in a filename). If you run tests for a different stage, it will invalidate the cached results and re-run the tests. It can be used to fingerprint anything else we want in the future.

alexcrichton · 2018-05-09T16:14:57Z

Oh as long as we're within limits on Windows I think we're good, and it looks like this is already taking that into account and optimizing for it!

nikomatsakis · 2018-05-10T16:07:31Z

@bors r+ -- let's do this!

bors · 2018-05-10T16:07:32Z

📌 Commit e0a6acd has been approved by nikomatsakis

ehuss · 2018-05-10T16:11:18Z

@nikomatsakis Just a warning, this conflicts with #50447. If this can get through the queue in a reasonable amount of time, I suggest just closing #50447.

kennytm · 2018-05-10T16:15:13Z

#50447 has been rolled up into #50608, so it will be merged first.

ehuss · 2018-05-10T16:17:15Z

@kennytm Ah, I'll just rebase as soon as it passes. Thanks!

bors · 2018-05-11T02:07:03Z

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

ehuss · 2018-05-11T02:45:04Z

Rebased.

alexcrichton · 2018-05-11T14:14:53Z

@bors: r=nikomatsakis

bors · 2018-05-11T14:14:54Z

📌 Commit 2d7096f has been approved by nikomatsakis

ehuss · 2018-05-13T15:15:28Z

I've been running various docker images locally, and haven't found any more issues, yet. I figured out how to get the wasm target going, too, and it seems ok.

alexcrichton · 2018-05-14T13:58:39Z

@bors: r+

bors · 2018-05-14T13:58:40Z

📌 Commit 598cc07 has been approved by alexcrichton

bors · 2018-05-17T04:54:40Z

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

- The output of each test is now in its own directory. - "auxiliary" output is now under the respective test directory. - `stage_id` removed from filenames, and instead placed in the stamp file as a hash. This helps keep path lengths down for Windows. In brief, the new layout looks like this: ``` <build_base>/<relative_dir>/<testname>.<revision>.<mode>/ stamp <testname>.err <testname>.out a (binary) auxiliary/lib<auxname>.dylib auxiliary/<auxname>/<auxname>.err auxiliary/<auxname>/<auxname>.out ``` (revision and mode are optional)

The aux dir, which previously had the `stage_id` embedded in it, was picking up remnants from previous runs.

ehuss · 2018-05-17T05:29:36Z

rebased

kennytm · 2018-05-17T07:48:36Z

@bors r=alexcrichton

bors · 2018-05-17T07:48:36Z

📌 Commit b8473de has been approved by alexcrichton

bors · 2018-05-17T08:29:20Z

⌛ Testing commit b8473de with merge e315056...

compiletest: Run revisions as independent tests. Fixes #47604. - The output of each test is now in its own directory. - "auxiliary" output is now under the respective test directory. - `stage_id` removed from filenames, and instead placed in the stamp file as a hash. This helps keep path lengths down for Windows. In brief, the new layout looks like this: ``` <build_base>/<relative_dir>/<testname>.<revision>.<mode>/ stamp <testname>.err <testname>.out a (binary) auxiliary/lib<auxname>.dylib auxiliary/<auxname>/<auxname>.err auxiliary/<auxname>/<auxname>.out ``` (revision and mode are optional)

bors · 2018-05-17T10:49:23Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e315056 to master...

rust-highfive assigned nikomatsakis May 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2018

ehuss force-pushed the compiletest-revisions branch from 2566edb to 5b9bee8 Compare May 4, 2018 21:31

ehuss force-pushed the compiletest-revisions branch from 5b9bee8 to eb6d575 Compare May 5, 2018 05:59

ehuss force-pushed the compiletest-revisions branch from eb6d575 to e0a6acd Compare May 7, 2018 17:19

nikomatsakis approved these changes May 9, 2018

View reviewed changes

ehuss mentioned this pull request May 10, 2018

Fix update-references for tests within subdirectories. #50447

Merged

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 10, 2018

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 May 11, 2018

ehuss force-pushed the compiletest-revisions branch from e0a6acd to 2d7096f Compare May 11, 2018 02:44

rust-lang deleted a comment from rust-highfive May 13, 2018

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 14, 2018

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 May 17, 2018

ehuss added 4 commits May 16, 2018 22:00

compiletest: rustfmt

2438434

Fix update-references for tests within subdirectories.

6f414b1

Fix running multiple targets.

b8473de

The aux dir, which previously had the `stage_id` embedded in it, was picking up remnants from previous runs.

ehuss force-pushed the compiletest-revisions branch from 598cc07 to b8473de Compare May 17, 2018 05:25

kennytm 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 17, 2018

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 17, 2018

bors merged commit b8473de into rust-lang:master May 17, 2018

bors mentioned this pull request May 17, 2018

Add bless x.py subcommand for easy ui test replacement #50806

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiletest: Run revisions as independent tests. #50400

compiletest: Run revisions as independent tests. #50400

ehuss commented May 2, 2018

rust-highfive commented May 2, 2018

ehuss commented May 2, 2018

bors commented May 4, 2018

bors commented May 5, 2018

bors commented May 7, 2018

nikomatsakis left a comment

nikomatsakis May 9, 2018

nikomatsakis commented May 9, 2018

nikomatsakis commented May 9, 2018

ehuss commented May 9, 2018

alexcrichton commented May 9, 2018

nikomatsakis commented May 10, 2018

bors commented May 10, 2018

ehuss commented May 10, 2018

kennytm commented May 10, 2018

ehuss commented May 10, 2018

bors commented May 11, 2018

ehuss commented May 11, 2018

alexcrichton commented May 11, 2018

bors commented May 11, 2018

ehuss commented May 13, 2018

alexcrichton commented May 14, 2018

bors commented May 14, 2018

bors commented May 17, 2018

ehuss commented May 17, 2018

kennytm commented May 17, 2018

bors commented May 17, 2018

bors commented May 17, 2018

bors commented May 17, 2018

compiletest: Run revisions as independent tests. #50400

compiletest: Run revisions as independent tests. #50400

Conversation

ehuss commented May 2, 2018

rust-highfive commented May 2, 2018

ehuss commented May 2, 2018

bors commented May 4, 2018

bors commented May 5, 2018

bors commented May 7, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis May 9, 2018

Choose a reason for hiding this comment

nikomatsakis commented May 9, 2018

nikomatsakis commented May 9, 2018

ehuss commented May 9, 2018

alexcrichton commented May 9, 2018

nikomatsakis commented May 10, 2018

bors commented May 10, 2018

ehuss commented May 10, 2018

kennytm commented May 10, 2018

ehuss commented May 10, 2018

bors commented May 11, 2018

ehuss commented May 11, 2018

alexcrichton commented May 11, 2018

bors commented May 11, 2018

ehuss commented May 13, 2018

alexcrichton commented May 14, 2018

bors commented May 14, 2018

bors commented May 17, 2018

ehuss commented May 17, 2018

kennytm commented May 17, 2018

bors commented May 17, 2018

bors commented May 17, 2018

bors commented May 17, 2018