Skip to content

Enable "full tools" option on ARM dist builders #71486

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

Merged
merged 2 commits into from
Apr 29, 2020

Conversation

alexcrichton
Copy link
Member

This commit switches the --enable-extended option on the arm-related
dist builders to --enable-full-tools. This alias in config.py
corresponds to enabling a few more options:

  • rust.lld = true - this is the main purpose of this PR, to enable LLD
    on ARM-related platforms. This means it will effectively unlock
    compilation of wasm programs from an arm host.

  • rust.llvm-tools = true - it turns out that this option is largely
    ignored in rustbuild today. This is only read in one location to set
    some flags for the llvm-tools package, but the llvm-tools package
    is already produced on all of these builders. It's predicted that this
    will have no effect on build times.

  • rust.codegen-backends = ['llvm'] - historically this also enabled
    the emscripten backend, but that has long since been removed.

This brings the ARM dist builders in line with the x86_64 dist builders
using this flag. The hope is that the extra time spent on CI building
LLD will acceptable because it's cached by sccache, LLD is a
relatively small C++ project, and the dist builders are all clocking
well under 3 hours (the slowest of all builders) around 2 hours.

There's likely some possible cleanup that can happen with these
configure options since it doesn't look like they've aged too too well,
but I'm hopeful that possible refactorings, if necessary, could be
deferred to future PRs.

This commit switches the `--enable-extended` option on the arm-related
dist builders to `--enable-full-tools`. This alias in `config.py`
corresponds to enabling a few more options:

* `rust.lld = true` - this is the main purpose of this PR, to enable LLD
  on ARM-related platforms. This means it will effectively unlock
  compilation of wasm programs from an arm host.

* `rust.llvm-tools = true` - it turns out that this option is largely
  ignored in rustbuild today. This is only read in one location to set
  some flags for the `llvm-tools` package, but the `llvm-tools` package
  is already produced on all of these builders. It's predicted that this
  will have no effect on build times.

* `rust.codegen-backends = ['llvm']` - historically this also enabled
  the emscripten backend, but that has long since been removed.

This brings the ARM dist builders in line with the x86_64 dist builders
using this flag. The hope is that the extra time spent on CI building
LLD will acceptable because it's cached by `sccache`, LLD is a
relatively small C++ project, and the dist builders are all clocking
well under 3 hours (the slowest of all builders) around 2 hours.

There's likely some possible cleanup that can happen with these
configure options since it doesn't look like they've aged too too well,
but I'm hopeful that possible refactorings, if necessary, could be
deferred to future PRs.
@rust-highfive
Copy link
Contributor

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2020
@Mark-Simulacrum
Copy link
Member

These changes look reasonable to me. I'm unsure if we want some form of approval (e.g. FCP) though -- seems like given this amounts to adding lld I don't think it's necessary.

r=me if you feel the same way.

r? @Mark-Simulacrum

@alexcrichton
Copy link
Member Author

I would agree yeah that this probably doesn't need a full sign-off. How about the following though to account for that:

  • Merge now w/o FCP
  • I'll watch the builds after this lands
  • If the build time increase is >10m, I'll revert and we can figure out something else, otherwise this'll slide in.

@Mark-Simulacrum
Copy link
Member

Seems reasonable to me. We're also migrating to GHA over the next month or so so slight losses are likely to be patched over by the 2 (or more) times faster builds there so I'm not too worried.

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 23, 2020

📌 Commit c7a7658 has been approved by Mark-Simulacrum

@bors 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 Apr 23, 2020
@alexcrichton
Copy link
Member Author

@bors: r-

I've got some build logic to work through, this won't work as-is.

@bors 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 Apr 24, 2020
Looks like the native build system isn't great a coping with this, so
try to work around that with a few workarounds.
@alexcrichton
Copy link
Member Author

Ok I pushed up another commit after getting a successful build of LLD for aarch64. @Mark-Simulacrum wanna take a look at this again?

@Mark-Simulacrum
Copy link
Member

That indeed looks like a pretty horrible hack, but if it works seems fine. If an LLVM upgrade breaks this we may need to drop it temporarily, but given how long those usually take, it's probably not too likely.

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 24, 2020

📌 Commit 0546d11 has been approved by Mark-Simulacrum

@bors 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 Apr 24, 2020
@bors
Copy link
Collaborator

bors commented Apr 28, 2020

⌛ Testing commit 0546d11 with merge 4307e4ad6681e80e1fd0b447c62fed384cf7a57f...

@bors
Copy link
Collaborator

bors commented Apr 28, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 28, 2020
@kennytm
Copy link
Member

kennytm commented Apr 28, 2020

@bors retry

https://dev.azure.com/rust-lang/rust/_build/results?buildId=27670&view=logs&j=aac1e68a-9baa-54d2-b80b-75ce793a8506&t=bd3d1d78-06ce-563d-23c4-bd941b41a778

Chocolatey timed out waiting for the command to finish. The timeout 
 specified (or the default value) was '2700' seconds. Perhaps try a 
 higher `--execution-timeout`? See `choco -h` for details.
The install of msys2 was NOT successful.

@bors 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 Apr 28, 2020
@bors
Copy link
Collaborator

bors commented Apr 28, 2020

⌛ Testing commit 0546d11 with merge 851098390128b82e3519fe93b8c88be2115a9359...

@Dylan-DPC-zz
Copy link

@bors retry yield

@bors
Copy link
Collaborator

bors commented Apr 28, 2020

⌛ Testing commit 0546d11 with merge 825cf51...

@bors
Copy link
Collaborator

bors commented Apr 29, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 29, 2020
@bors bors merged commit 825cf51 into rust-lang:master Apr 29, 2020
@alexcrichton alexcrichton deleted the arm64-lld branch April 29, 2020 15:03
@alexcrichton
Copy link
Member Author

Ok so looking at the build just before this and just after this the numbers sort of go both ways (some arm builders got faster, some slower). I think it's all largely within variance though.

Looking at the timing specifically of building LLD, it looks like we do indeed have a wasteful build of LLD for the host platform, but otherwise it only takes 20s (cached) to build LLD, so that's not so bad in a 3h build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

6 participants