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

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

Merged
merged 2 commits into from
Dec 19, 2016

Conversation

nikomatsakis
Copy link
Contributor

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
Copy link
Contributor Author

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.

> ../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
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a copy-and-paste artifact.

@nikomatsakis
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@nikomatsakis nikomatsakis Nov 30, 2016

Choose a reason for hiding this comment

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

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


# 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 \
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I guess there is optflagmulti

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

impl<'a> Step<'a> {
fn noop() -> Step<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

👍

much nicer

@alexcrichton
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@nikomatsakis
Copy link
Contributor Author

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

  1. 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.
  2. 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
Copy link
Contributor Author

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
Copy link
Contributor

bors commented Nov 30, 2016

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

@alexcrichton
Copy link
Member

@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
Copy link
Contributor Author

@alexcrichton rebased and adjusted

# 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@alexcrichton
Copy link
Member

r=me with a squash

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

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Dec 1, 2016

📌 Commit 3bfdd48 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Dec 3, 2016

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

@bors
Copy link
Contributor

bors commented Dec 3, 2016

💔 Test failed - auto-linux-64-cargotest

@bors
Copy link
Contributor

bors commented Dec 7, 2016

☔ 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
Copy link
Contributor Author

@bors r=acrichto

@bors
Copy link
Contributor

bors commented Dec 19, 2016

📌 Commit 83453bc has been approved by acrichto

@bors
Copy link
Contributor

bors commented Dec 19, 2016

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

bors added a commit that referenced this pull request Dec 19, 2016
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 bors merged commit 83453bc into rust-lang:master Dec 19, 2016
@est31 est31 mentioned this pull request Dec 20, 2016
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Dec 24, 2016
@nikomatsakis nikomatsakis deleted the bootstrap-incremental branch April 14, 2017 10:12
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jun 2, 2017
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.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 3, 2017
…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.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Jun 3, 2017
…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.
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.

4 participants