add preliminary support for incremental compilation to rustbuild.py #38072

nikomatsakis · 2016-11-29T15:00:34Z

This implements the integration described in #37929. It requires the use of a local nightly as your bootstrap compiler. The setup is described in src/bootstrap/README.md.

This does NOT implement the "copy stage0 libs to stage1" optimization described in #37929, just because that seems orthogonal to me.

In local testing, I do not yet see any incremental re-use when building rustc. I'm not sure why that is, more investigation needed.

(For these reasons, this is not marked as fixing the relevant issue.)

r? @alexcrichton -- I included one random cleanup (Step::noop()) that turned out to not be especially relevant. Feel free to tell me you liked it better the old way.

nikomatsakis · 2016-11-29T15:40:47Z

Probably I should remove the code that dumps the rustc command line -- well, more specifically, I should make it a separate option, distinct from --verbose. This is because unless you use -j1 you get some pretty gibberish output.

michaelwoerister · 2016-11-29T15:54:16Z

src/bootstrap/README.md

+> ../configure --enable-rustbuild --local-rust-root=~/.cargo/ --enable-local-rust \
+               --enable-local-rebuild --enable-debug --enable-optimize
+
+# you don't have to use a build directory, I just think it's nicer


This seems like a copy-and-paste artifact.

nikomatsakis · 2016-11-29T19:21:59Z

So, I take it back, I actually am getting reuse locally. I'm experimenting more now.

I also have some local fixes to make before this lands:

I am moving the incremental dir to out/<host>/stageN-incremental
- this also means that x.py clean deletes it, as (I imagine) would be expected
I permit --stage 0 or --stage 1 when using --incremental

nikomatsakis · 2016-11-29T20:05:48Z

OK, this is... good to go I guess, though maybe it would be better to avoid printing the full verbose output unless the verbosity is at "level 2" or something like that. Anyway, using it locally, and running with --verbose, I do observe that we get a decent amount of reuse. Not as much as I would like: just touching some random file in libstd, for example, gets 100% reuse in libstd, but we gradually get less and less for other crates, due to us thinking that metadata has changed.

nikomatsakis · 2016-11-29T20:22:25Z

Fixed the verbosity level concern in the latest commit by adding --very-verbose

alexcrichton · 2016-11-29T21:58:09Z

src/bootstrap/README.md

+```
+# you don't have to use a build directory, I just think it's nicer --nmatsakis
+> mkdir build
+> cd build


Technically rustbuild already places all of it's output in a build directory, so this'll create build/build. Maybe we could leave the separate directory bits to a different section of the readme?

Yeah, I'll remove it. I just still find that it's better to have a build directory, since it winds up with quite a lot of files in it:

> ls build config.mk config.stamp Makefile tmp

alexcrichton · 2016-11-29T22:00:00Z

src/bootstrap/README.md

+
+# ok, the --enable-debug and --enable-optimize are not strictly needed,
+# but I recommend them personally. --nmatsakis
+> ../configure --enable-rustbuild --local-rust-root=~/.cargo/ --enable-local-rust \


I think that --local-rust-root and --enable-local-rust can both be elided here. At least rustbuild only takes a look at --enable-local-rust, so the other two configuration options are ignored by rustbuild.

--local-rust-root is certainly used, since that's how we pick which binary -- but I think if you don't supply it, we will search your path. I can add some comments about that.

alexcrichton · 2016-11-29T22:00:57Z

src/bootstrap/flags.rs

@@ -63,6 +74,8 @@ impl Flags {
    pub fn parse(args: &[String]) -> Flags {
        let mut opts = Options::new();
        opts.optflag("v", "verbose", "use verbose output");
+        opts.optflag("", "very-verbose", "use very verbose output");


Normally this is interpreted as -vv, but I'm not sure if getopts has the ability to tell us about that?

Yeah, I think it doesn't? I should dig a bit more I guess.

oh I guess there is optflagmulti

alexcrichton · 2016-11-29T22:01:30Z

src/bootstrap/step.rs

@@ -29,6 +29,10 @@ struct Step<'a> {
 }

 impl<'a> Step<'a> {
+    fn noop() -> Step<'a> {


👍

much nicer

alexcrichton · 2016-11-29T22:02:54Z

Hm I didn't quite see all the pieces I expected to see here. So right now if you pass --incremental it'll incremental build the entire compiler. But I think a ./x.py build will continue to use that compiler to build a whole new compiler, right? That second compilation isn't incremental from what it looks like.

nikomatsakis · 2016-11-30T17:51:57Z

@alexcrichton

But I think a ./x.py build will continue to use that compiler to build a whole new compiler, right? That second compilation isn't incremental from what it looks like.

Right. This is what I meant by this from the PR summary: 'This does NOT implement the "copy stage0 libs to stage1" optimization described in #37929, just because that seems orthogonal to me.'

Note that --incremental also implies --stage 1, so you're not going to rebuild all of rustc.

nikomatsakis · 2016-11-30T18:12:58Z

@alexcrichton pushed one bug fix and tweaked README. I didn't address the problem of copying libraries over yet.

nikomatsakis · 2016-11-30T22:48:28Z

So @alexcrichton and I had some chats over IRC and we came to some conclusions:

If we are not copying stage0 libs into the stage1 directory, but rather recompiling, then there is no need to use a local rust. I mean, you should just because the beta support for incremental compilation is weak. But it's not required.
The only reason to rebuild all the libs for stage1 is because the metadata format may have changed (well, that and testing etc). So we should perhaps say "if you use --incremental and --enable-local-rebuild (which is only set if your local rust has same version number), then we copy the libs by default".

This would mean that building with --incremental and a recent nightly as your base compiler is by default fast:

It would only build to stage1 (because --incremental)
It would not rebuild the libs 2x (because --incremental and --enable-local-rebuild)
It would build incrementally up to stage1 (because --incremental)

@alexcrichton do you want to hold off on landing until I do that? doesn't seem so hard.

nikomatsakis · 2016-11-30T22:48:59Z

On the other hand, I could just remove the check that forces --incremental to use a local Rust and land this now, and then add the lib copying as a separate thing. That would be my preference.

bors · 2016-11-30T23:40:53Z

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

alexcrichton · 2016-12-01T01:18:17Z

@nikomatsakis I'd be fine landing this ahead of time, seems like a fine optimization no matter what to unconditionally incrementally build stage0 with --incremental (and it's always possible). I'd also be fine adding an orthogonal feature to copy libs instead of build them.

So in that sense, likely r=me with a rebase, but I'll take one more look.

nikomatsakis · 2016-12-01T18:46:44Z

@alexcrichton rebased and adjusted

alexcrichton · 2016-12-01T19:18:47Z

src/bootstrap/README.md

+# configure to use local rust instead of downloding a beta.
+# `--local-rust-root` is optional here. If elided, we will
+# use whatever rustc we find on your PATH.
+> configure --enable-rustbuild --local-rust-root=~/.cargo/ --enable-local-rebuild


Aha! I found out where we actually read this. Ideally we wouldn't need --local-rust-root here because --enable-local-rebuild should say "get from PATH by default".

Let's fix that bug later.

@alexcrichton actually, I think the configure script does fetch from path by default -- I just used --local-rust-root because I have a different rustc in my path that takes precedence over rustup

oh, but I see that bootstrap doesn't. ok.

alexcrichton · 2016-12-01T19:20:04Z

r=me with a squash

nikomatsakis · 2016-12-01T19:57:20Z

@bors r=alexcrichton

bors · 2016-12-01T19:57:22Z

📌 Commit 3bfdd48 has been approved by alexcrichton

bors · 2016-12-03T17:30:27Z

⌛ Testing commit 3bfdd48 with merge 5daf4b6...

bors · 2016-12-03T17:36:11Z

💔 Test failed - auto-linux-64-cargotest

bors · 2016-12-07T16:30:46Z

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

For example: - we now support `-vv` to get very verbose output. - RUSTFLAGS is respected by `x.py` - better error messages for some cases

nikomatsakis · 2016-12-19T16:53:55Z

@bors r=acrichto

bors · 2016-12-19T16:53:56Z

📌 Commit 83453bc has been approved by acrichto

bors · 2016-12-19T20:07:48Z

⌛ Testing commit 83453bc with merge 94ae2a2...

@alexcrichton

add preliminary support for incremental compilation to rustbuild.py This implements the integration described in #37929. It requires the use of a local nightly as your bootstrap compiler. The setup is described in `src/bootstrap/README.md`. This does NOT implement the "copy stage0 libs to stage1" optimization described in #37929, just because that seems orthogonal to me. In local testing, I do not yet see any incremental re-use when building rustc. I'm not sure why that is, more investigation needed. (For these reasons, this is not marked as fixing the relevant issue.) r? @alexcrichton -- I included one random cleanup (`Step::noop()`) that turned out to not be especially relevant. Feel free to tell me you liked it better the old way.

bors · 2016-12-19T23:45:59Z

@nikomatsakis

…e, r=nikomatsakis Correct path of incremental artifacts Per rust-lang#38072 (comment) r? @nikomatsakis

This was added in rust-lang#38072 but I can't recall why and AFAIK Cargo already handles this. This was discovered through rust-lang#42146 where passing duplicate flags was causing problems.

…rk-Simulacrum rustbuild: Remove RUSTFLAGS logic in rustc shim This was added in rust-lang#38072 but I can't recall why and AFAIK Cargo already handles this. This was discovered through rust-lang#42146 where passing duplicate flags was causing problems.

rust-highfive assigned alexcrichton Nov 29, 2016

michaelwoerister reviewed Nov 29, 2016

View reviewed changes

alexcrichton reviewed Nov 29, 2016

View reviewed changes

src/bootstrap/step.rs

@@ -29,6 +29,10 @@ struct Step<'a> {

}

impl<'a> Step<'a> {

fn noop() -> Step<'a> {

Copy link

Member

alexcrichton Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

much nicer

nikomatsakis force-pushed the bootstrap-incremental branch from 73eaec0 to 41a677b Compare December 1, 2016 18:46

nikomatsakis force-pushed the bootstrap-incremental branch from 41a677b to d09050f Compare December 1, 2016 18:54

alexcrichton reviewed Dec 1, 2016

View reviewed changes

nikomatsakis force-pushed the bootstrap-incremental branch 2 times, most recently from f4601db to 3bfdd48 Compare December 1, 2016 19:57

nikomatsakis added 2 commits December 19, 2016 11:46

add concept of Step::noop()

ef8921a

add and document --incremental flag along with misc other changes

83453bc

For example: - we now support `-vv` to get very verbose output. - RUSTFLAGS is respected by `x.py` - better error messages for some cases

nikomatsakis force-pushed the bootstrap-incremental branch from 3bfdd48 to 83453bc Compare December 19, 2016 16:48

bors merged commit 83453bc into rust-lang:master Dec 19, 2016

est31 mentioned this pull request Dec 20, 2016

rustbuild warning #38484

Closed

aidanhs mentioned this pull request Dec 22, 2016

Correct path of incremental artifacts #38549

Merged

nikomatsakis mentioned this pull request Dec 23, 2016

integrate incremental into rustbuild.py #37929

Closed

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Dec 24, 2016

Rollup merge of rust-lang#38549 - aidanhs:aphs-incremental-readme-not…

8669b5d

…e, r=nikomatsakis Correct path of incremental artifacts Per rust-lang#38072 (comment) r? @nikomatsakis

nikomatsakis deleted the bootstrap-incremental branch April 14, 2017 10:12

alexcrichton mentioned this pull request Jun 2, 2017

rustbuild: Remove RUSTFLAGS logic in rustc shim #42382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add preliminary support for incremental compilation to rustbuild.py #38072

add preliminary support for incremental compilation to rustbuild.py #38072

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

michaelwoerister Nov 29, 2016

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

alexcrichton Nov 29, 2016

nikomatsakis Nov 30, 2016 •

edited

alexcrichton Nov 29, 2016

nikomatsakis Nov 30, 2016

alexcrichton Nov 29, 2016

nikomatsakis Nov 30, 2016

nikomatsakis Dec 1, 2016

alexcrichton Nov 29, 2016

alexcrichton commented Nov 29, 2016

nikomatsakis commented Nov 30, 2016

nikomatsakis commented Nov 30, 2016

nikomatsakis commented Nov 30, 2016

nikomatsakis commented Nov 30, 2016

bors commented Nov 30, 2016

alexcrichton commented Dec 1, 2016

nikomatsakis commented Dec 1, 2016

alexcrichton Dec 1, 2016

nikomatsakis Dec 1, 2016

nikomatsakis Dec 1, 2016

alexcrichton commented Dec 1, 2016

nikomatsakis commented Dec 1, 2016

bors commented Dec 1, 2016

bors commented Dec 3, 2016

bors commented Dec 3, 2016

bors commented Dec 7, 2016

nikomatsakis commented Dec 19, 2016

bors commented Dec 19, 2016

bors commented Dec 19, 2016

bors commented Dec 19, 2016

add preliminary support for incremental compilation to rustbuild.py #38072

add preliminary support for incremental compilation to rustbuild.py #38072

Conversation

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

Choose a reason for hiding this comment

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

Choose a reason for hiding this comment

nikomatsakis Nov 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Nov 29, 2016

nikomatsakis commented Nov 30, 2016

nikomatsakis commented Nov 30, 2016

nikomatsakis commented Nov 30, 2016

nikomatsakis commented Nov 30, 2016

bors commented Nov 30, 2016

alexcrichton commented Dec 1, 2016

nikomatsakis commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Dec 1, 2016

nikomatsakis commented Dec 1, 2016

bors commented Dec 1, 2016

bors commented Dec 3, 2016

bors commented Dec 3, 2016

bors commented Dec 7, 2016

nikomatsakis commented Dec 19, 2016

bors commented Dec 19, 2016

bors commented Dec 19, 2016

bors commented Dec 19, 2016

nikomatsakis Nov 30, 2016 •

edited