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

[WIP] rustbuild: Build jemalloc and libbacktrace only once #38583

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 23, 2016

Part of #38284
Build common libbacktrace and jemalloc for all stages.
Run configure for libbacktrace and jemalloc only when makefiles don't exist yet.
The location for building C/C++ code is build/<target>/native (I also moved rust-test-helpers there because it was trivial).

├── build
│   └── x86_64-pc-windows-gnu
│       ├── native
│       │   ├── jemalloc
│       │   ├── libbacktrace
│       │   └── rust-test-helpers

I haven't touched the build system before and haven't worked with Cargo build scripts, so I'm submitting this early to get some feedback.

Questions:
Any problems with the whole setup, cross-compilation or something?
Does native/jemalloc and native/libbacktrace need some kind of filesystem lock during the build? Probably not, different stages are not built in parallel.
What to do with errors in build.rs, e.g. "can't create directory", panic? What cargo does with panicking build scripts?
I had to pass the native directory location to Cargo build scripts through a new env variable, is there a better way?
Why does Cargo hide non-special stderr/stdout from build scripts? How can I enable it?

Plans (maybe in this PR, maybe not):
Look at other C/C++ code in the repo. It looks like most of small things are built directly with CC without any dependency tracking build system, so we can't reuse the build results between stages unless we check their up-to-date-ness manually in Rust code, but this is not a big deal since they are small and built quickly. compiler-rt is relatively large and is built directly as well, maybe there's a way to share its build results from Rust code as well, but it's unfortunate to do build systems' job manually.
Fill in rerun-if-changed for all build scripts.
Move LLVM build into native directory as well, make LLVM a dependency of the Rust crate that needs it instead of building it immediately and unconditionally.
Do not purge LLVM build directory after any changes in LLVM, reuse the existing build instead.

r? @alexcrichton

@petrochenkov
Copy link
Contributor Author

Whitespace ignoring diff: https://github.com/rust-lang/rust/pull/38583/files?w=1

@alexcrichton
Copy link
Member

Thanks for the PR! I think this is definitely along the right lines and what we'll want to end up at. To answer some of your questions:

Any problems with the whole setup, cross-compilation or something?

Due to your definition of Build::native_dir, I believe this is correct for cross compilation. I don't think we'd run into problems otherwise.

Does native/jemalloc and native/libbacktrace need some kind of filesystem lock during the build? Probably not, different stages are not built in parallel.

Yeah stages are not parallel, nor do we even run same-stage builds in parallel. Regardless we'd only ever compile libbacktrace/jemalloc once for any one target, so I think it's ok to elide locks.

What to do with errors in build.rs, e.g. "can't create directory", panic? What cargo does with panicking build scripts?

If a build script panics, Cargo just fails the build and prints out the stdout/stderr of the build script.

I had to pass the native directory location to Cargo build scripts through a new env variable, is there a better way?

For now this is ok by me. Could you make the env var optional, though? Ideally all crates work with cargo build outside of rustbuild itself, and rustbuild just configures pieces slightly differently. I think you could accomplish this with a .unwrap_or(env::var("OUT_DIR")) or something like that.

Why does Cargo hide non-special stderr/stdout from build scripts? How can I enable it?

You can pass -vv to cargo to stream all output. Otherwise Cargo suppresses successful build scripts by default to avoid unnecessary noise on a build.

Look at other C/C++ code in the repo. It looks like most of small things are built directly with CC without any dependency tracking build system, so we can't reuse the build results between stages unless we check their up-to-date-ness manually in Rust code, but this is not a big deal since they are small and built quickly. compiler-rt is relatively large and is built directly as well, maybe there's a way to share its build results from Rust code as well, but it's unfortunate to do build systems' job manually.

AFAIK the only C++ we have are the helper files we use to communicate with LLVM, which shouldn't take too long to build, so it may not be critical we handle the librustc_llvm build script at least.

Otherwise yeah compiler-rt is probably the biggest candidate to cache.

Fill in rerun-if-changed for all build scripts.

Sounds good to me! Some already have it, but not all may have been modified.

Move LLVM build into native directory as well, make LLVM a dependency of the Rust crate that needs it instead of building it immediately and unconditionally.

This I think will need to be a bit more long term. LLVM takes so long to build we'll probably run into issues with output on Travis, but overall I'd love to move the logic into the build script and not rustbuild!

Do not purge LLVM build directory after any changes in LLVM, reuse the existing build instead.

Sounds good to me!


My only worry for this is that for dirty build directories we'll want to rerun ./configure, for example, if something about the configure script changes. That is, if a Makefile exists, we may want to still actually run the configure script to pick up changes.

I wonder if we could solve this slightly different, though? Perhaps we could actually run the build only in stage0, and then further stages just assume the output is at the stage0 location. That way stage 1+ builds would just short circuit everything but stage0 builds would retain the same logic as today as always-configure-then-build.

@petrochenkov
Copy link
Contributor Author

@alexcrichton
Thanks for the answers, I'll update the PR soon, I hope.

if something about the configure script changes. That is, if a Makefile exists, we may want to still actually run the configure script to pick up changes

Makefiles generated by configure contain dependencies back to configure, so the configuration is rerun automatically by make if necessary (cmake does the same thing).

Perhaps we could actually run the build only in stage0, and then further stages just assume the output is at the stage0 location

The condition should probably be stage0 && !test, because x.py test --stage 0 seems to rerun the build script again. I'll look more carefully.
IMO, delegating all this to make is simpler and more robust than doing make's job manually.

@alexcrichton
Copy link
Member

Makefiles generated by configure contain dependencies back to configure, so the configuration is rerun automatically by make if necessary (cmake does the same thing).

Ah I wasn't aware of that! Empirically though I've found that this rarely works in practice. Even with CMake I find myself having to blow away the build directory and especially with makefiles I have to reconfigure then make very often. Our own build system used to detect changes like this but it wouldn't pick up changes to the makefiles themselves because the build system wouldn't re-make, it'd only re-configure.

In that sense I think we'll still need to handle this by periodically reconfiguring.

The condition should probably be stage0 && !test, because x.py test --stage 0 seems to rerun the build script again.

Ah build scripts aren't actually compiled or run specially for tests, so test --stage 0 would use the same build script from build --stage 0, so I don't think we'll need to worry about tests here.

@bors
Copy link
Contributor

bors commented Jan 1, 2017

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

@alexcrichton
Copy link
Member

ping @petrochenkov, any update on this?

@petrochenkov
Copy link
Contributor Author

@alexcrichton
I still plan to finish this, but not in the next couple of weeks.
(If someone wants this earlier, feel free to reuse the patch.)
Closing for now.

bors added a commit that referenced this pull request Feb 3, 2017
rustbuild: Build jemalloc and libbacktrace only once (take 2)

This is a rebase of #38583 without any additions, but with implemented @alexcrichton's suggestions.
~~This includes `exists(Makefile)` => `cfg(stage0)` suggestion... but it will break cross-compilation, no? Are `libstd/liballoc_jemalloc` cross-compiled for `target != host` built during `stage0`?~~

r? @alexcrichton
@petrochenkov petrochenkov deleted the rb branch March 16, 2017 19:45
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