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

Add config option and cli arg for skip-llvm-rebuild #65848

Closed
wants to merge 2 commits into from
Closed

Add config option and cli arg for skip-llvm-rebuild #65848

wants to merge 2 commits into from

Conversation

Walther
Copy link

@Walther Walther commented Oct 26, 2019

Fixes #65612.

First time contributing, please let me know if there's anything I need to tweak 🙂 ❤️

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 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-10-26T15:31:29.8394278Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-26T15:31:29.8589462Z ##[command]git config gc.auto 0
2019-10-26T15:31:29.8651565Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-26T15:31:29.8706291Z ##[command]git config --get-all http.proxy
2019-10-26T15:31:29.8826978Z ##[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/65848/merge:refs/remotes/pull/65848/merge
---
2019-10-26T15:37:48.9541542Z    Compiling serde_json v1.0.40
2019-10-26T15:37:50.5816216Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-10-26T15:38:01.3701437Z     Finished release [optimized] target(s) in 1m 23s
2019-10-26T15:38:01.3772259Z tidy check
2019-10-26T15:38:01.7770035Z tidy error: /checkout/src/bootstrap/bootstrap.py:827: line longer than 100 chars
2019-10-26T15:38:03.5427586Z Found 484 error codes
2019-10-26T15:38:03.5427656Z Found 0 error codes with no tests
2019-10-26T15:38:03.5427697Z Done!
2019-10-26T15:38:03.5427756Z some tidy checks failed
2019-10-26T15:38:03.5427756Z some tidy checks failed
2019-10-26T15:38:03.5427779Z 
2019-10-26T15:38:03.5427799Z 
2019-10-26T15:38:03.5428496Z command did not execute successfully: "/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-10-26T15:38:03.5428596Z 
2019-10-26T15:38:03.5428617Z 
2019-10-26T15:38:03.5428669Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-10-26T15:38:03.5428707Z Build completed unsuccessfully in 0:01:27
2019-10-26T15:38:03.5428707Z Build completed unsuccessfully in 0:01:27
2019-10-26T15:38:03.5473041Z == clock drift check ==
2019-10-26T15:38:03.5485857Z   local time: Sat Oct 26 15:38:03 UTC 2019
2019-10-26T15:38:03.6959967Z   network time: Sat, 26 Oct 2019 15:38:03 GMT
2019-10-26T15:38:03.6968758Z == end clock drift check ==
2019-10-26T15:38:05.0970795Z 
2019-10-26T15:38:05.1043064Z ##[error]Bash exited with code '1'.
2019-10-26T15:38:05.1074805Z ##[section]Starting: Checkout
2019-10-26T15:38:05.1076799Z ==============================================================================
2019-10-26T15:38:05.1076852Z Task         : Get sources
2019-10-26T15:38:05.1076912Z 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)

@Walther
Copy link
Author

Walther commented Oct 26, 2019

I'm not sure if adding the pub skip_llvm_rebuild: bool into the config is a great thing, as the Rust side of the build will not use that config option anywhere. On the other hand, tidy check will fail without it, as it will fail to parse the unknown property in the config.toml file.

@alexcrichton
Copy link
Member

Thanks for the PR! I think I may be missing something though? It looks like this adds configuration for skipping an LLVM rebuild but doesn't actually end up reading it when building LLVM?

Also cc @varkor who requested these changes

@Walther
Copy link
Author

Walther commented Oct 28, 2019

Ah, it's not enough to skip the submodule in bootstrap.py https://github.com/rust-lang/rust/pull/65848/files#diff-3e203f016947a52e3f1fad3967027e7eR734-R740 - sorry, I'll take a new look at where else it needs to be skipped 👍

@JohnCSimon JohnCSimon 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
@Walther can you please post your status on this pr?
cc: @alexcrichton @varkor

Thank you!

@Walther
Copy link
Author

Walther commented Nov 3, 2019

I have a simple question - at src/bootstrap/config.rs line 388 ish, i'd love to add config.skip_llvm_rebuild to be set from the bootstrap.py too like some other settings are, as i did the handling of CLI args & TOML setting already there. Should I set a new capitalized env var for it, or should i plop it within RUSTFLAGS or some such in build_bootstrap(), or something completely different? I just need the already-resolved self.skip_llvm_rebuild in RustBuild() in Python/bootstrap side to be visible in Rust side too.

In addition to what's visible in the PR, I already locally added the lines necessary to actually skip the LLVM build in native.rs

if builder.config.skip_llvm_rebuild {
            builder.info("Skipping LLVM rebuild");
            return build_llvm_config
        }

@alexcrichton
Copy link
Member

I think in general it's fine to only update config.rs, I suspect that bootstrap.py doesn't really need any changes for this. Inside of native.rs yeah I think you'd basically have that block which would skip any amount of LLVM rebuilding.

@JohnCSimon
Copy link
Member

Ping from triage:
@Walther - can you address the comments from @alexcrichton so this can be moved forward?

Thank you!

@Walther
Copy link
Author

Walther commented Nov 12, 2019

Sorry, I've been fairly busy! I hope I can get back at this next weekend or so. If not, and if someone wants to quickly fix this instead, that's okay too. There's always more issues to help with :)

@joelpalmer
Copy link

Ping from Triage: Any update @Walther?

@JohnCSimon
Copy link
Member

Ping from triage:
@Walther - can you please post your status for this PR?

Thank you.

@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 30, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
@Walther - Unfortunately this PR has sat idle for far too long, and I am going to close it as inactive. Please reopen it when you have the time to make the necessary changes or find help to move this forward.
Please do not push to the PR while it is closed as that prevents it from being reopened.
Thank you for your efforts!

@JohnCSimon JohnCSimon closed this Nov 30, 2019
aidanhs added a commit to aidanhs/rust that referenced this pull request Dec 24, 2019
…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... 😬
bors added a commit that referenced this pull request Dec 27, 2019
…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... 😬
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add x.py option to skip rebuilding LLVM
5 participants