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 cleanups/fixes and improvements #43630

Merged
merged 11 commits into from
Aug 13, 2017

Conversation

Mark-Simulacrum
Copy link
Member

Each commit is a standalone change, and can/should be reviewed separately.

This adds two new functionalities:

  • --target and --host can be passed without changing config.toml, and we'll respect the users' wishes, instead of requiring that all possible targets are passed.
    • Note that this means that ./x.py clean won't be quite as wide-spread as before, since it limits itself to the configured hosts, not all hosts. This could be considered a feature as well.
  • ignore-git field in config.toml which tells Rustbuild to not attempt to load git hashes from .git.

This is a precursor to eventual further simplification of the configuration system, but I want to get this merged first so that later work can be made in individual PRs.

r? @alexcrichton

@aidanhs aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2017
@alexcrichton
Copy link
Member

@bors: r+

Nice!

@bors
Copy link
Contributor

bors commented Aug 5, 2017

📌 Commit 1121c75 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 5, 2017

⌛ Testing commit 1121c75c32c0d840bd0ef73d026628863e4bc50f with merge 1df9f9c7316c2942fa87cdf72a7293262be8f696...

@bors
Copy link
Contributor

bors commented Aug 5, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Aug 5, 2017

armhf-gnu failed, the UI run-pass tests cannot be executed. Cross-testing issue.

[00:42:05] ---- [ui] ui/deprecated-macro_escape-inner.rs stdout ----
[00:42:05] 	ui: /checkout/src/test/ui/deprecated-macro_escape-inner.rs
[00:42:05] thread '[ui] ui/deprecated-macro_escape-inner.rs' panicked at 'failed to exec `/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/remote-test-client`: Error { repr: Os { code: 2, message: "No such file or directory" } }', /checkout/src/libcore/result.rs:860:4
[00:42:05] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:42:05] 
...
[00:42:05] 
[00:42:05] failures:
[00:42:05]     [ui] ui/deprecated-macro_escape-inner.rs
[00:42:05]     [ui] ui/deprecated-macro_escape.rs
[00:42:05]     [ui] ui/deriving-meta-empty-trait-list.rs
[00:42:05]     [ui] ui/enum-size-variance.rs
[00:42:05]     [ui] ui/issue-19100.rs
[00:42:05]     [ui] ui/path-lookahead.rs
[00:42:05]     [ui] ui/test-should-panic-attr.rs
[00:42:05] 
[00:42:05] test result: FAILED. 348 passed; 7 failed; 1 ignored; 0 measured; 0 filtered out

@Mark-Simulacrum
Copy link
Member Author

Hm, odd. I can't tell why it wouldn't have been available, since it was built as far as I can tell, and for the right target and stage. @alexcrichton Do we copy the RemoteTestClient to the qemu emulator usually? In the meantime, as the queue is mostly empty, I'm going to retry this just in case it was a spurious failure, since I don't believe I changed anything in relevant code...

@bors retry

@bors
Copy link
Contributor

bors commented Aug 5, 2017

⌛ Testing commit 1121c75c32c0d840bd0ef73d026628863e4bc50f with merge 6ca50f7533e08bcf5ea39eaddb9f0f446ba55393...

@bors
Copy link
Contributor

bors commented Aug 5, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Aug 5, 2017

Same error @Mark-Simulacrum.

@alexcrichton
Copy link
Member

Hm yeah I dunno what's going on :(

Files like remote-test-client indeed never go to the emulator, although this error looks like it's coming from the host, not the emulator? The remote-test-client binary was compiled, though, so I'm not sure what's up?

@Mark-Simulacrum
Copy link
Member Author

I'm wondering if maybe some change I added makes it so that we clear out the stage0 tools directory at some point? I'll try to investigate soon, though not sure when I'll get the chance.

@bors
Copy link
Contributor

bors commented Aug 8, 2017

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

@Mark-Simulacrum Mark-Simulacrum 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 Aug 10, 2017
@Mark-Simulacrum
Copy link
Member Author

@bors r=alexcrichton

The latest commit (7d2e1a9) has the fix for the problem encountered in CI. I'm not sure why the problem doesn't occur on master, and the fix contained isn't perfect, but I hope it'll be sufficient. See the commit description for more details.

@bors
Copy link
Contributor

bors commented Aug 10, 2017

📌 Commit 7d2e1a9 has been approved by alexcrichton

@Mark-Simulacrum Mark-Simulacrum 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 Aug 10, 2017
@bors
Copy link
Contributor

bors commented Aug 10, 2017

⌛ Testing commit 7d2e1a9e848a1a303ae9be5f73f7418d9d57f83d with merge d20881a5d77c454a61b2b739b8251c0abdbe692c...

@bors
Copy link
Contributor

bors commented Aug 10, 2017

💔 Test failed - status-appveyor

@@ -198,6 +198,12 @@ impl Step for StdLink {
// for reason why the sanitizers are not built in stage0.
copy_apple_sanitizer_dylibs(&build.native_dir(target), "osx", &libdir);
}

builder.ensure(tool::CleanTools {
Copy link
Member

Choose a reason for hiding this comment

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

Failed to bootstrap. Missing use tool;?

   Compiling bootstrap v0.0.0 (file:///C:/projects/rust/src/bootstrap)
error[E0433]: failed to resolve. Use of undeclared type or module `tool`
   --> src\bootstrap\compile.rs:202:24
    |
202 |         builder.ensure(tool::CleanTools {
    |                        ^^^^^^^^^^^^^^^^ Use of undeclared type or module `tool`
error[E0433]: failed to resolve. Use of undeclared type or module `tool`
   --> src\bootstrap\compile.rs:398:24
    |
398 |         builder.ensure(tool::CleanTools {
    |                        ^^^^^^^^^^^^^^^^ Use of undeclared type or module `tool`
error[E0433]: failed to resolve. Use of undeclared type or module `tool`
   --> src\bootstrap\compile.rs:581:24
    |
581 |         builder.ensure(tool::CleanTools {
    |                        ^^^^^^^^^^^^^^^^ Use of undeclared type or module `tool`

This introduces a slight change in behavior, where we unilaterally
respect the --host and --target parameters passed for all sanity
checking and runtime configuration.
We no longer care about the source of this information, so there is no
reason to restrict users.
We first check the configuration, then passed parameters (--build), then
fall back to the auto-detection that bootstrap.py does.

Fixes rust-lang#39673.
Some users of the build system change the git sha on every build due to
utilizing git to push changes to a remote server. This allows them to
simply configure that away instead of depending on custom patches to
rustbuild.
This permits proc macro crates to correctly work with rustdoc.
This fixes the bug we previously had where we'd build a libtest tool
after building a libstd tool and clear out the libstd tool. Since we
clear out all tools for a given stage on invocations of CleanTools after
lib{std, test, rustc} change, we need to make sure that all tools built
with that stage will be built after the clearing is done.

The fix contained here technically isn't perfect; there is still an edge
case of compiling a libstd tool, then compiling libtest, which will
clear out the libstd tool and it won't ever get rebuilt within that
session of rustbuild. This is where the caching system used today shows
it's problems -- in effect, all tools depend on a global counter of the
stage being cleared out. We can implement such a counter in a future
patch to ensure that tools are rebuilt as needed, but it is deemed
unlikely that it will be required in practice, since most if not all
tools are built after the relevant stage's std/test/rustc are built,
though this is only an opinion and hasn't been verified.
@Mark-Simulacrum
Copy link
Member Author

Thanks for investigating! I've pushed a fix that just renames the "tool" rustdoc to rustdoc-tool-binary which should work if I understand correctly, and rebased.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2017

📌 Commit 82cdf10 has been approved by Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member Author

Ah,

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 13, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Aug 13, 2017

📌 Commit 82cdf10 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 13, 2017

⌛ Testing commit 82cdf10 with merge 7eb0bd28d5a88e2971159c8237c95c41c5e1b456...

@bors
Copy link
Contributor

bors commented Aug 13, 2017

💔 Test failed - status-travis

@Mark-Simulacrum
Copy link
Member Author

Compiler hosts aren't always capable of running on the current platform...

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 13, 2017

📌 Commit 01641c7 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 13, 2017

⌛ Testing commit 01641c7 with merge adbce60...

bors added a commit that referenced this pull request Aug 13, 2017
…hton

Rustbuild cleanups/fixes and improvements

Each commit is a standalone change, and can/should be reviewed separately.

This adds two new functionalities:

 - `--target` and `--host` can be passed without changing config.toml, and we'll respect the users' wishes, instead of requiring that all possible targets are passed.
   - Note that this means that `./x.py clean` won't be quite as wide-spread as before, since it limits itself to the configured hosts, not all hosts. This could be considered a feature as well.
 - `ignore-git` field in `config.toml` which tells Rustbuild to not attempt to load git hashes from `.git`.

This is a precursor to eventual further simplification of the configuration system, but I want to get this merged first so that later work can be made in individual PRs.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Aug 13, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants