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.

Sign up for GitHub

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

Jump to bottom

rustbuild: A few tweaks #40236

Merged
merged 8 commits into from
Mar 4, 2017
Merged

rustbuild: A few tweaks #40236

merged 8 commits into from
Mar 4, 2017

Conversation

petrochenkov
Copy link
Contributor

Fixes #40016
Fixes #39507

r? @alexcrichton

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

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

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

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

r=me

@petrochenkov
Copy link
Contributor Author

@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

@bors: r+

@bors
Copy link
Contributor

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
Contributor

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

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 4, 2017

📌 Commit a1c6471 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 4, 2017

⌛ Testing commit a1c6471 with merge d7d1dd0...

@bors
Copy link
Contributor

bors commented Mar 4, 2017

💔 Test failed - status-travis

@petrochenkov
Copy link
Contributor Author

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
Contributor

bors commented Mar 4, 2017

🔒 Merge conflict

@petrochenkov
Copy link
Contributor Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 4, 2017

📌 Commit 428f063 has been approved by alexcrichton

@bors
Copy link
Contributor

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
Contributor

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