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

rustbuild: Build jemalloc and libbacktrace only once (take 2) #39329

Merged
merged 4 commits into from
Feb 3, 2017

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jan 26, 2017

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

} else if !target.contains("windows") && !target.contains("musl") {
println!("cargo:rustc-link-lib=pthread");
}
if !cfg!(stage0) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I believe you're right in that this may break cross compiles. Perhaps !cfg!(stage0) && some_stamp_file.exists() could be checked here? That "stamp" file could be created at the end of the build.

@bors
Copy link
Contributor

bors commented Jan 27, 2017

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

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 28, 2017

!cfg!(stage0) && some_stamp_file.exists()

This is exactly what I want to avoid - redoing make's job poorly.
I've used !cfg!(stage0) && target == host - this shouldn't be a pessimization unless stage 2 is built (?).


The best solution is what the previous PR does - running make unconditionally (modulo rerun-if-changed) and configure if Makefile doesn't exist.
I haven't answered the argument from #38583 (comment), but it's misunderstanding. Running configure explicitly and implicitly through make are equivalent. What makes a difference (in case the build directory ended up in bad state, this certainly happens from my experience too) is purging the build directory and starting fresh build.

@alexcrichton
Copy link
Member

Yeah target == host sounds plausible. Thinking back though I wonder if this'll break --enable-local-rebuild (or whatever the name of the flag is)? That never ends up passing --cfg stage0 but instead passes --cfg stage1 during the first build.

@petrochenkov
Copy link
Contributor Author

I wonder if this'll break --enable-local-rebuild

Restored the solution from #38583, it's automatically correct regardless of rustbuild configuration or cross-compilation intricacies.

@alexcrichton
Copy link
Member

@petrochenkov I've mentioned before though that this approach is empirically incorrect? For a number of reasons:

  • If make regenerates the Makefile, then the build won't pick that up (even if it re-runs ./configure)
  • If we change ./configure arguments, existing build directories won't pick up that change

@petrochenkov
Copy link
Contributor Author

Ok, those are valid reasons.
I'll implement the suggestion with timestamps then.

I'll have to move libbacktrace/jemalloc builds from build scripts to bootstrap though.
Otherwise code dealing with timestamps (currently located in bootstrap/util.rs) will need to be moved to build_helper bloating it and introducing dependency on filetime crate. build_helper and its dependencies are built for all stages so it's not good.

@petrochenkov
Copy link
Contributor Author

build_helper bloating it and introducing dependency on filetime crate. build_helper and its dependencies are built for all stages

Actually, nevermind. This is still much faster than building libbacktrace and jemalloc.
Updated the PR with timestamps.

@alexcrichton
Copy link
Member

@bors: r+

Looks great, thanks!

@bors
Copy link
Contributor

bors commented Feb 1, 2017

📌 Commit 0e41f13 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 2, 2017

⌛ Testing commit 0e41f13 with merge f6ce61b...

@bors
Copy link
Contributor

bors commented Feb 2, 2017

💔 Test failed - status-travis

@petrochenkov
Copy link
Contributor Author

error: There are multiple `libc` packages in your project, and the specification `libc` is ambiguous.
Please re-run this command with `-p <spec>` where `<spec>` is one of the following:
  libc:0.0.0
  libc:0.2.17

Hm, is this spurious or not?

@alexcrichton
Copy link
Member

Nah I think that's a correct error, adding a new dependency here change the dependency graph.

You can fix this though by appending :0.0.0 here

@petrochenkov
Copy link
Contributor Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 3, 2017

📌 Commit b4abb72 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 3, 2017

⌛ Testing commit b4abb72 with merge 57ecd7a...

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

bors commented Feb 3, 2017

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

@bors bors merged commit b4abb72 into rust-lang:master Feb 3, 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