Skip to content

rustbuild: A few tweaks#40236

Merged
bors merged 8 commits intorust-lang:masterfrom
petrochenkov:btweak
Mar 4, 2017
Merged

rustbuild: A few tweaks#40236
bors merged 8 commits intorust-lang:masterfrom
petrochenkov:btweak

Conversation

@petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Mar 3, 2017

Fixes #40016
Fixes #39507

r? @alexcrichton

alexcrichton
alexcrichton reviewed Mar 3, 2017
View reviewed changes
src/build_helper/lib.rs
pub timestamp: PathBuf,
}

pub fn native_lib_boilerplate(src_name: &str,
Copy link
Member

@alexcrichton alexcrichton Mar 3, 2017

Choose a reason for hiding this comment

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

Could you add some comments here as well as to how this function is expected to be called? It looks like there's a standard rigamarole of checking skip_build and then creating the timestamp at the end, and it'd be good to enusre that's all documented.

@alexcrichton
Copy link
Member

alexcrichton commented Mar 3, 2017

Just one minor request for a comment but otherwise looks great to me, thanks!

r=me

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 3, 2017

@alexcrichton
Updated with comments for native_lib_boilerplate.
I also automated timestamp creation and partially build skipping, it should be simpler this way.

@alexcrichton
Copy link
Member

alexcrichton commented Mar 3, 2017

@bors: r+

@bors
Copy link
Collaborator

bors commented Mar 3, 2017

📌 Commit 384f64a has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 3, 2017
@frewsxcv
Rollup merge of rust-lang#40236 - petrochenkov:btweak, r=alexcrichton
6efb3b2 
rustbuild: A few tweaks

Fixes rust-lang#40016
Fixes rust-lang#39507

r? @alexcrichton
@frewsxcv frewsxcv mentioned this pull request Mar 3, 2017
Closed
@bors
Copy link
Collaborator

bors commented Mar 4, 2017

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

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 4, 2017

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Mar 4, 2017

📌 Commit a1c6471 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Mar 4, 2017

⌛ Testing commit a1c6471 with merge d7d1dd0...

@bors
Copy link
Collaborator

bors commented Mar 4, 2017

💔 Test failed - status-travis

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 4, 2017

Spurious (?) failure in the middle of LLVM build due to sccache (?).
Or it may be some interaction between LLVM build caching used on CI and de2af63.

@bors retry

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 4, 2017 •
edited
Loading

@alexcrichton

What are reuse requirements for building LLVM on Travis/Appveyor with caching, by the way?

What I want with local builds is:

  • if llvm-finished-building does not exist, do not delete the build directory, reuse it (REUSE)
  • if llvm-finished-building exists and differs from llvm-auto-clean-trigger, do not delete the build directory, reuse it (REUSE)
  • if llvm-finished-building exists and is equal to llvm-auto-clean-trigger, then do not try to build anything at all (SKIP)
  • fresh build is done only if something really bad happens and I delete the build directory manually, so this PR doesn't quite do what I'd like to see ideally.

As I understand, what CI wants is:

  • if llvm-finished-building does not exist, delete the build directory, do a fresh build accelerated by (s)ccache (FRESH)
  • if llvm-finished-building exists and differs from llvm-auto-clean-trigger, delete the build directory, do a fresh build accelerated by (s)ccache (FRESH)
  • if llvm-finished-building exists and is equal to llvm-auto-clean-trigger, then do not try to build anything at all (SKIP)
  • i.e. CI always does either full reuse or, failing that, accelerated fresh build, because reuse provided by caching tools is considered more reliable than one provided by CMake.

Is my understanding correct?
If it's correct, then a new option in config.toml switching between these two use cases would be a better solution than what de2af63 does.

@bors
Copy link
Collaborator

bors commented Mar 4, 2017

🔒 Merge conflict

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 4, 2017

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Mar 4, 2017

📌 Commit 428f063 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Mar 4, 2017

⌛ Testing commit 428f063 with merge ba07bd5...

bors added a commit that referenced this pull request Mar 4, 2017
@bors
Auto merge of #40236 - petrochenkov:btweak, r=alexcrichton
ba07bd5 
rustbuild: A few tweaks

Fixes #40016
Fixes #39507

r? @alexcrichton
@bors
Copy link
Collaborator

bors commented Mar 4, 2017

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

@bors bors merged commit 428f063 into rust-lang:master Mar 4, 2017
@petrochenkov petrochenkov mentioned this pull request Mar 7, 2017
Merged
@alexcrichton alexcrichton mentioned this pull request Mar 8, 2017
Closed
bors added a commit that referenced this pull request Mar 13, 2017
@bors
Auto merge of #40329 - petrochenkov:llreuse, r=alexcrichton
fd182c4 
rustbuild: Add option for enabling partial LLVM rebuilds

@alexcrichton , you probably didn't notice my [late comment](#40236 (comment)) on #40236, but here's an implementation of that suggestion, it supersedes c652a4f.

r? @alexcrichton
@petrochenkov petrochenkov deleted the btweak branch March 16, 2017 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@alexcrichton alexcrichton

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@petrochenkov @alexcrichton @bors