Add LLVM `skip-rebuild` option to `x.py` #67437

matthew-healy · 2019-12-19T20:27:55Z

This PR reimplements parts of @Walther's work from #65848, and closes #65612.

I decided not to implement the arguments to override this setting in this PR. If there's strong feeling that this change shouldn't be merged without the overrides then I'm happy to close this until I've had a chance to add them in. Otherwise I'll aim to submit a second PR with those this weekend.

I'd have liked to have tested the change in native.rs, but there didn't seem to be any existing test infrastructure. I ran this a few times manually and it worked on my machine though... 😬

rust-highfive · 2019-12-19T20:28:06Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2019-12-19T21:59:15Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-19T20:28:37.3282672Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-19T20:28:37.3526796Z ##[command]git config gc.auto 0
2019-12-19T20:28:37.3589344Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-19T20:28:37.3653706Z ##[command]git config --get-all http.proxy
2019-12-19T20:28:37.3805209Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67437/merge:refs/remotes/pull/67437/merge
---
2019-12-19T20:38:44.8874423Z configure: build.locked-deps    := True
2019-12-19T20:38:44.8874470Z configure: llvm.ccache          := sccache
2019-12-19T20:38:44.8874692Z configure: build.cargo-native-static := True
2019-12-19T20:38:44.8874892Z configure: dist.missing-tools   := True
2019-12-19T20:38:44.8875138Z configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
2019-12-19T20:38:44.8875296Z configure: writing `config.toml` in current directory
2019-12-19T20:38:44.8875341Z configure: 
2019-12-19T20:38:44.8875614Z configure: run `python /checkout/x.py --help`
2019-12-19T20:38:44.8875661Z configure: 
---
2019-12-19T20:46:56.7915137Z Build completed successfully in 0:01:36
2019-12-19T20:46:56.7924923Z + /scripts/validate-toolstate.sh
2019-12-19T20:46:56.7994761Z Cloning into 'rust-toolstate'...
2019-12-19T20:46:57.3263505Z Traceback (most recent call last):
2019-12-19T20:46:57.3264416Z   File "../../src/tools/publish_toolstate.py", line 303, in <module>
2019-12-19T20:46:57.3264706Z     cur_datetime
2019-12-19T20:46:57.3264889Z   File "../../src/tools/publish_toolstate.py", line 182, in update_latest
2019-12-19T20:46:57.3268162Z     for os in ['windows', 'linux']
2019-12-19T20:46:57.3268564Z   File "../../src/tools/publish_toolstate.py", line 182, in <dictcomp>
2019-12-19T20:46:57.3269024Z     for os in ['windows', 'linux']
2019-12-19T20:46:57.3269279Z   File "../../src/tools/publish_toolstate.py", line 111, in read_current_status
2019-12-19T20:46:57.3270213Z     (commit, status) = line.split('\t', 1)
2019-12-19T20:46:57.3270672Z ValueError: need more than 1 value to unpack
2019-12-19T20:46:57.3320862Z   local time: Thu Dec 19 20:46:57 UTC 2019
2019-12-19T20:46:57.5959915Z   network time: Thu, 19 Dec 2019 20:46:57 GMT
2019-12-19T20:46:57.5963776Z == end clock drift check ==
2019-12-19T20:46:59.2820469Z 
2019-12-19T20:46:59.2820469Z 
2019-12-19T20:46:59.2941954Z ##[error]Bash exited with code '1'.
2019-12-19T20:46:59.2989246Z ##[section]Starting: Checkout
2019-12-19T20:46:59.2991286Z ==============================================================================
2019-12-19T20:46:59.2991374Z Task         : Get sources
2019-12-19T20:46:59.2991428Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

matthew-healy · 2019-12-20T21:51:41Z

I think the build failed for reasons unrelated to the changes. Could someone with powers retry it for me?

Mark-Simulacrum · 2019-12-20T21:56:37Z

Shouldn't matter too much, it's just the PR build -- the main PR builder (llvm-7) passed anyway. I will however do r? @Mark-Simulacrum as it's likely I'll be the reviewer here not Niko; I'll try to get to this soon.

Mark-Simulacrum

r=me with nits fixed

I don't personally agree with having this option as it seems strictly worse to get spurious LLVM bugs due to not rebuilding it, but since people seem to want it, I'm not opposed either.

Mark-Simulacrum · 2019-12-21T17:41:08Z

config.toml.example

+# this is `false` then the compiler's LLVM will be rebuilt whenever the built
+# version doesn't have the correct hash. If it is `true` then LLVM will never
+# be rebuilt. The default value is `false`.
+#skip-rebuild = true


Suggested change

#skip-rebuild = true

#skip-rebuild = false

This should indicate the default value.

Mark-Simulacrum · 2019-12-21T17:42:29Z

src/bootstrap/config.rs

@@ -617,6 +621,9 @@ impl Config {
        set(&mut config.initial_rustc, build.rustc.map(PathBuf::from));
        set(&mut config.initial_cargo, build.cargo.map(PathBuf::from));

+        let default = false;
+        config.llvm_skip_rebuild = llvm_skip_rebuild.unwrap_or(default);


Please inline default here. I'm not sure why LLVM asserts below have it pulled out (no need to change that in this PR).

Yeah, I also thought this was a bit odd, but decided to stick to the pattern that was there. Will fix this now. Thanks! 👍

Mark-Simulacrum · 2019-12-21T17:43:33Z

src/bootstrap/native.rs

@@ -74,6 +74,11 @@ impl Step for Llvm {
        let done_stamp = out_dir.join("llvm-finished-building");

        if done_stamp.exists() {
+            if builder.config.llvm_skip_rebuild {
+                builder.info("Skipping LLVM rebuild");


We should use language similar to --keep-stage here.

Suggested change

builder.info("Skipping LLVM rebuild");

builder.info("Warning: Using a potentially old LLVM; this may not behave well.");

matthew-healy · 2019-12-22T16:06:07Z

@Mark-Simulacrum I think I've addressed your feedback. Let me know if you need anything else.

rust-highfive · 2019-12-22T16:20:37Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-22T16:00:42.8036286Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-22T16:00:42.8238519Z ##[command]git config gc.auto 0
2019-12-22T16:00:42.8318530Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-22T16:00:42.8383145Z ##[command]git config --get-all http.proxy
2019-12-22T16:00:42.8531192Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67437/merge:refs/remotes/pull/67437/merge
---
2019-12-22T16:06:26.8583886Z    Compiling serde_json v1.0.40
2019-12-22T16:06:28.4531984Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-12-22T16:06:39.2427165Z     Finished release [optimized] target(s) in 1m 25s
2019-12-22T16:06:39.2527880Z tidy check
2019-12-22T16:06:39.9636159Z tidy error: /checkout/src/bootstrap/native.rs:78: line longer than 100 chars
2019-12-22T16:06:41.9550051Z some tidy checks failed
2019-12-22T16:06:41.9550876Z Found 485 error codes
2019-12-22T16:06:41.9551173Z Found 0 error codes with no tests
2019-12-22T16:06:41.9551479Z Done!
2019-12-22T16:06:41.9551479Z Done!
2019-12-22T16:06:41.9551799Z 
2019-12-22T16:06:41.9552132Z 
2019-12-22T16:06:41.9553367Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-12-22T16:06:41.9554048Z 
2019-12-22T16:06:41.9554273Z 
2019-12-22T16:06:41.9557654Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-12-22T16:06:41.9558014Z Build completed unsuccessfully in 0:01:28
2019-12-22T16:06:41.9558014Z Build completed unsuccessfully in 0:01:28
2019-12-22T16:06:41.9604235Z == clock drift check ==
2019-12-22T16:06:41.9615242Z   local time: Sun Dec 22 16:06:41 UTC 2019
2019-12-22T16:06:42.0472508Z   network time: Sun, 22 Dec 2019 16:06:42 GMT
2019-12-22T16:06:42.0475715Z == end clock drift check ==
2019-12-22T16:06:43.4507818Z 
2019-12-22T16:06:43.4617350Z ##[error]Bash exited with code '1'.
2019-12-22T16:06:43.4645910Z ##[section]Starting: Checkout
2019-12-22T16:06:43.4651890Z ==============================================================================
2019-12-22T16:06:43.4651969Z Task         : Get sources
2019-12-22T16:06:43.4652016Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Mark-Simulacrum · 2019-12-22T16:24:10Z

r=me with the tidy failure fixed

Mark-Simulacrum · 2019-12-23T16:02:19Z

@bors r+

bors · 2019-12-23T16:02:21Z

📌 Commit 522f977 has been approved by Mark-Simulacrum

bors · 2019-12-23T16:02:22Z

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@Walther

…Mark-Simulacrum Add LLVM `skip-rebuild` option to `x.py` This PR reimplements parts of @Walther's work from rust-lang#65848, and closes rust-lang#65612. I decided not to implement the [arguments to override this setting](rust-lang#65612 (comment)) in this PR. If there's strong feeling that this change shouldn't be merged without the overrides then I'm happy to close this until I've had a chance to add them in. Otherwise I'll aim to submit a second PR with those this weekend. I'd have liked to have tested the change in `native.rs`, but there didn't seem to be any existing test infrastructure. I ran this a few times manually and it _worked on my machine_ though... 😬

aidanhs · 2019-12-24T11:46:09Z

@bors r-

         if done_stamp.exists() {
             if builder.config.llvm_skip_rebuild {
-                builder.info("Warning: \
+                builder.info(
+                    "Warning: \
                               Using a potentially stale build of LLVM; \
-                              This may not behave well.");
+                              This may not behave well.",
+                );
                 return build_llvm_config;
             }

Tidy failure in the rollup

matthew-healy · 2019-12-25T09:50:43Z

I don't know enough about how rollups work to know why there'd be a tidy issue there and not when I run it locally, but I've reformatted it to match @aidanhs' comment.

rust-highfive · 2019-12-25T10:10:40Z

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-25T09:50:15.1629300Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-25T09:50:15.1842117Z ##[command]git config gc.auto 0
2019-12-25T09:50:15.1917857Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-25T09:50:15.1989881Z ##[command]git config --get-all http.proxy
2019-12-25T09:50:15.2131571Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67437/merge:refs/remotes/pull/67437/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Mark-Simulacrum · 2019-12-25T16:12:11Z

Looks like rustfmt wants a comma at the end. You might be able to observe the failure locally by rebasing atop master before running tidy.

matthew-healy · 2019-12-26T22:20:50Z

@Mark-Simulacrum Thank you! Should be fixed now.

Mark-Simulacrum · 2019-12-27T00:10:23Z

@bors r+

bors · 2019-12-27T00:10:24Z

📌 Commit e44fc45 has been approved by Mark-Simulacrum

bors · 2019-12-27T10:59:02Z

⌛ Testing commit e44fc45 with merge 41501a6...

@Walther

…crum Add LLVM `skip-rebuild` option to `x.py` This PR reimplements parts of @Walther's work from #65848, and closes #65612. I decided not to implement the [arguments to override this setting](#65612 (comment)) in this PR. If there's strong feeling that this change shouldn't be merged without the overrides then I'm happy to close this until I've had a chance to add them in. Otherwise I'll aim to submit a second PR with those this weekend. I'd have liked to have tested the change in `native.rs`, but there didn't seem to be any existing test infrastructure. I ran this a few times manually and it _worked on my machine_ though... 😬

bors · 2019-12-27T14:03:57Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 41501a6 to master...

…ion, r=Centril Add `llvm-skip-rebuild` flag to `x.py` This PR follows on from rust-lang#67437 to complete the feature request from rust-lang#65612. Specifically it adds a new command-line flag, `--llvm-skip-rebuild`, which overrides both any value set in `config.toml` and the default value (`false`). I'm not 100% confident that I've implemented the override in the "best" way, but I've checked it locally and it seems to work at least. This option isn't currently mentioned in the Guide to Rustc Development. I'd be happy to write something on it if folk think that's worthwhile.

rust-highfive assigned nikomatsakis Dec 19, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2019

rust-highfive assigned Mark-Simulacrum and unassigned nikomatsakis Dec 20, 2019

Mark-Simulacrum reviewed Dec 21, 2019

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 23, 2019

aidanhs mentioned this pull request Dec 24, 2019

Rollup of 3 pull requests #67581

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 24, 2019

Matthew Healy added 3 commits December 26, 2019 22:16

Document LLVM skip-rebuild config.toml option

1a01f97

Parse llvm_skip_rebuild into Config

2297a8b

Skip LLVM rebuild when skip-rebuild is true

e44fc45

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 27, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 27, 2019

bors merged commit e44fc45 into rust-lang:master Dec 27, 2019

matthew-healy deleted the skip-llvm-rebuild branch January 9, 2020 19:12

matthew-healy mentioned this pull request Jan 9, 2020

Add llvm-skip-rebuild flag to x.py #68074

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LLVM `skip-rebuild` option to `x.py` #67437

Add LLVM `skip-rebuild` option to `x.py` #67437

matthew-healy commented Dec 19, 2019

rust-highfive commented Dec 19, 2019

rust-highfive commented Dec 19, 2019

matthew-healy commented Dec 20, 2019

Mark-Simulacrum commented Dec 20, 2019

Mark-Simulacrum left a comment

Mark-Simulacrum Dec 21, 2019

Mark-Simulacrum Dec 21, 2019

matthew-healy Dec 22, 2019

Mark-Simulacrum Dec 21, 2019

matthew-healy commented Dec 22, 2019

rust-highfive commented Dec 22, 2019

Mark-Simulacrum commented Dec 22, 2019

Mark-Simulacrum commented Dec 23, 2019

bors commented Dec 23, 2019

bors commented Dec 23, 2019

aidanhs commented Dec 24, 2019

matthew-healy commented Dec 25, 2019

rust-highfive commented Dec 25, 2019

Mark-Simulacrum commented Dec 25, 2019

matthew-healy commented Dec 26, 2019

Mark-Simulacrum commented Dec 27, 2019

bors commented Dec 27, 2019

bors commented Dec 27, 2019

bors commented Dec 27, 2019

	builder.info("Skipping LLVM rebuild");
	builder.info("Warning: Using a potentially old LLVM; this may not behave well.");

Add LLVM skip-rebuild option to x.py #67437

Add LLVM skip-rebuild option to x.py #67437

Conversation

matthew-healy commented Dec 19, 2019

rust-highfive commented Dec 19, 2019

rust-highfive commented Dec 19, 2019

matthew-healy commented Dec 20, 2019

Mark-Simulacrum commented Dec 20, 2019

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Mark-Simulacrum Dec 21, 2019

Choose a reason for hiding this comment

Mark-Simulacrum Dec 21, 2019

Choose a reason for hiding this comment

matthew-healy Dec 22, 2019

Choose a reason for hiding this comment

Mark-Simulacrum Dec 21, 2019

Choose a reason for hiding this comment

matthew-healy commented Dec 22, 2019

rust-highfive commented Dec 22, 2019

Mark-Simulacrum commented Dec 22, 2019

Mark-Simulacrum commented Dec 23, 2019

bors commented Dec 23, 2019

bors commented Dec 23, 2019

aidanhs commented Dec 24, 2019

matthew-healy commented Dec 25, 2019

rust-highfive commented Dec 25, 2019

Mark-Simulacrum commented Dec 25, 2019

matthew-healy commented Dec 26, 2019

Mark-Simulacrum commented Dec 27, 2019

bors commented Dec 27, 2019

bors commented Dec 27, 2019

bors commented Dec 27, 2019

Add LLVM `skip-rebuild` option to `x.py` #67437

Add LLVM `skip-rebuild` option to `x.py` #67437