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

travis: Gate on some minimal support for incremental compilation. #38847

Merged
merged 2 commits into from
Jan 31, 2017

Conversation

michaelwoerister
Copy link
Member

This commit adds a travis job that

  1. builds a stage2 compiler in incremental mode (but with empty incremental compilation cache), and
  2. builds and runs the run-pass test suite also in incremental mode.

Building incrementally with an empty cache makes sure that the compiler doesn't crash in dependency tracking during bootstrapping. Executing the incrementally built test suite gives some measure of confidence that we generate valid code.

Note, however, that the above does not give strong guarantees about the validity of incremental compilation, it just provides a basis for being able to rely on from-scratch incr. comp. builds as reference values in further tests (which then do actual incremental compilation).

r? @alexcrichton

@michaelwoerister
Copy link
Member Author

cc @nikomatsakis

@alexcrichton
Copy link
Member

@michaelwoerister have you confirmed that RUSTFLAGS does indeed work here? I could imagine that it's picked up by Cargo to build crates, but I'm not sure if we have logic to move those flags into compiletest test suites. Just want to make sure we're actually doing that all incrementally!

Additionally, perhaps we could just run the whole test suite here? Presumably stdtest/coretest/collectionstest may weed out other bugs that may otherwise crop up?

@michaelwoerister
Copy link
Member Author

I did test that the compiler is invoked with -Zincremental during bootstrap, but you're right, the test suites are a different matter. I'll look into that.

Additionally, perhaps we could just run the whole test suite here? Presumably stdtest/coretest/collectionstest may weed out other bugs that may otherwise crop up?

Good idea! Though I suspect that at least some of the tests would not want -Zincremental passed to them. Is there a way to filter out some tests/test suites? Or does x.py take multiple arguments?

@alexcrichton
Copy link
Member

You can't say "don't run these suites" but you can indeed say "run all of these suites" with something like:

./x.py test src/test/run-pass src/test/compile-fail src/libstd ...

(etc)

@michaelwoerister
Copy link
Member Author

Yeah, that should do the trick.

@alexcrichton
Copy link
Member

ping @michaelwoerister, any update on this?

@michaelwoerister
Copy link
Member Author

Somewhat: I can confirm that RUSTFLAGS are not passed down to tests with rustbuild. So in the current form, this would only test that bootstrapping works with incremental compilation and that a compiler generated this way would generate valid code. It would not confirm that it could also incrementally compile all tests and they'd still be valid.

The question is whether that's enough testing for now or whether I should extend compiletest with an "incremental mode" ...

@alexcrichton
Copy link
Member

@michaelwoerister I'd be fine either way. We could always add this and then perhaps expand later if need be?

This commit adds a travis job that builds a stage2 compiler in
incremental mode (but with empty incremental compilation cache).
Building incrementally with an empty cache makes sure that the
compiler doesn't crash in dependency tracking during bootstrapping.
@michaelwoerister
Copy link
Member Author

@alexcrichton I've updated this to just bootstrapping in incremental mode and then running make check.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 18, 2017

📌 Commit 5f118b9 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jan 20, 2017

⌛ Testing commit 5f118b9 with merge d2803ce...

@bors
Copy link
Contributor

bors commented Jan 20, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

So that error is just a spurious network error, but @michaelwoerister I think that this error may be caused by this PR? (found from the rollup)

@michaelwoerister
Copy link
Member Author

I'll try to reproduce that locally.

bors added a commit that referenced this pull request Jan 20, 2017
bors added a commit that referenced this pull request Jan 21, 2017
@michaelwoerister
Copy link
Member Author

OK, I was able to reproduce this locally. Haven't looked further into to yet...

@michaelwoerister
Copy link
Member Author

So the problem here is that the run-make tests inherit the RUSTFLAGS env-var, which messes with the codegen-unit related tests there. I tried to fix this by setting CARGO_INCREMENTAL instead of RUSTFLAGS, but the Cargo version used from bootstrapping doesn't seem to pick that up yet.

@alexcrichton
Copy link
Member

@michaelwoerister perhaps the compiletest support for run-make could delete the env var from spawned processes?

@michaelwoerister
Copy link
Member Author

perhaps the compiletest support for run-make could delete the env var from spawned processes?

I'll look into that. It would only be consistent with the rest of compiletest.

@michaelwoerister
Copy link
Member Author

Yeah, that seems to be sufficient to fix the run-make tests. I just pushed a commit to that effect.
r? @alexcrichton

@alexcrichton
Copy link
Member

Oh I think we've got .env_remove to completely remove an env var, and could you also add a comment to the effect of why it's there?

@michaelwoerister
Copy link
Member Author

Updated.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 30, 2017

📌 Commit d292e12 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jan 30, 2017

⌛ Testing commit d292e12 with merge 9364f6f...

@bors
Copy link
Contributor

bors commented Jan 31, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Jan 31, 2017 via email

@bors
Copy link
Contributor

bors commented Jan 31, 2017

⌛ Testing commit d292e12 with merge 36e0616...

@bors
Copy link
Contributor

bors commented Jan 31, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Jan 31, 2017 via email

@bors
Copy link
Contributor

bors commented Jan 31, 2017

⌛ Testing commit d292e12 with merge 0c85f2a...

bors added a commit that referenced this pull request Jan 31, 2017
…hton

travis: Gate on some minimal support for incremental compilation.

This commit adds a travis job that

1. builds a stage2 compiler in incremental mode (but with empty incremental compilation cache), and
2. builds and runs the run-pass test suite also in incremental mode.

Building incrementally with an empty cache makes sure that the compiler doesn't crash in dependency tracking during bootstrapping. Executing the incrementally built test suite gives some measure of confidence that we generate valid code.

Note, however, that the above does not give strong guarantees about the validity of incremental compilation, it just provides a basis for being able to rely on from-scratch incr. comp. builds as reference values in further tests (which then do actual incremental compilation).

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Jan 31, 2017

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

@bors bors merged commit d292e12 into rust-lang:master Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants