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

Allow downloading LLVM on Windows and MacOS #80932

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jan 12, 2021

  • Don't ignore packaging llvm/lib/ for rust-dev when LLVM is linked
    statically

  • Add link-type.txt so bootstrap knows whether llvm was linked
    statically or dynamically

  • Don't assume CI LLVM is linked dynamically in bootstrap::config

  • Fall back to dynamic linking if link-type.txt doesn't exist

  • Fix existing bug that split the output of llvm-config on lines, not spaces

  • Only special case MacOS when dynamic linking. Static linking works fine.

  • Enable building LLVM tests

    This works around the following llvm bug:

    llvm-config: error: component libraries and shared library
    
    llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest.a
    llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest_main.a
    llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libLLVMTestingSupport.a
    thread 'main' panicked at 'command did not execute successfully: "/home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-config" "--libfiles"
    

    I'm not sure why llvm-config thinks these are required, but to avoid
    the error, this builds them anyway.

  • Bump version of download-ci-llvm-stamp

    src/llvm-project hasn't changed, but the generated tarball has.

Fixes #77084.

Current Status

This works on both MacOS and Windows! 🎉 🎉 Thanks to @nagisa, @Halkcyon, @Lokathor, @jryans, and @poliorcetics for helping me test!

The if-available check now supports all tier 1 platforms. Although only x64 apple and x64 msvc have been tested, none of the changes here are Windows or Mac specific, and I expect this to work anywhere that LLVM artifacts are uploaded to CI (i.e. the rust-dev component exists).

Windows

Note that if you have an old version of MSVC build tools you'll need to update them. VS Build Tools 2019 14.28 and later are known to work. With old tools, you may see an error like the following:

error LNK2001: unresolved external symbol __imp___std_init_once_complete

@jyn514 jyn514 added the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Jan 12, 2021
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Jan 12, 2021
@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Jan 12, 2021
@jyn514 jyn514 force-pushed the download-windows-llvm branch from 13db0d3 to 98ef3a8 Compare January 12, 2021 01:37
@jyn514

This comment has been minimized.

@bors

This comment has been minimized.

@jyn514

This comment has been minimized.

@jyn514

This comment has been minimized.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the download-windows-llvm branch from 98ef3a8 to 71bd5d6 Compare January 12, 2021 01:46
@jyn514

This comment has been minimized.

@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@jyn514

This comment has been minimized.

@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514

This comment has been minimized.

@bors

This comment has been minimized.

@jyn514 jyn514 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 13, 2021
@jyn514 jyn514 changed the title Allow downloading LLVM on Windows Allow downloading LLVM on Windows and MacOS Jan 13, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the download-windows-llvm branch from 8dd7666 to 3bfa458 Compare January 13, 2021 14:45
supported_platforms = [
"x86_64-unknown-linux-gnu",
# FIXME: maybe windows-gnu should be supported too?
"x86_64-pc-windows-msvc",
Copy link
Member

@nagisa nagisa Jan 13, 2021

Choose a reason for hiding this comment

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

I'd be fairly comfortable putting all T1 targets on this list at this point. x86_64 working seems good enough indication that 32-bit equivalents and the aarch64 target will work too, since the complexity is in primarily in the "os" part rather than the "arch" part of the triple.

Copy link
Member Author

@jyn514 jyn514 Jan 13, 2021

Choose a reason for hiding this comment

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

Ok, I added them to the list. Is there a way to see a list of all the artifacts that are uploaded? e.g. https://ci-artifacts.rust-lang.org/rustc-builds/695f18ebab0ca895841514da841eb3bd6f6bbd88 gives a 404.

@jyn514 jyn514 force-pushed the download-windows-llvm branch from 3bfa458 to aad9437 Compare January 13, 2021 22:14
src/bootstrap/config.rs Outdated Show resolved Hide resolved
src/bootstrap/dist.rs Outdated Show resolved Hide resolved
src/bootstrap/native.rs Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

r=me with two small comments addressed

- Don't ignore packaging `llvm/lib/` for `rust-dev` when LLVM is linked
statically
- Add `link-type.txt` so bootstrap knows whether llvm was linked
  statically or dynamically
- Don't assume CI LLVM is linked dynamically in `bootstrap::config`
- Fall back to dynamic linking if `link-type.txt` doesn't exist
- Fix existing bug that split the output of `llvm-config` on lines, not spaces
- Enable building LLVM tests

  This works around the following llvm bug:

  ```
  llvm-config: error: component libraries and shared library

  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest.a
  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest_main.a
  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libLLVMTestingSupport.a
  thread 'main' panicked at 'command did not execute successfully: "/home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-config" "--libfiles"
  ```

  I'm not sure why llvm-config thinks these are required, but to avoid
  the error, this builds them anyway.

- Temporarily set windows as the try builder. This should be reverted
  before merging.

- Bump version of `download-ci-llvm-stamp`

  `src/llvm-project` hasn't changed, but the generated tarball has.

- Only special case MacOS when dynamic linking. Static linking works fine.
- Store `link-type.txt` to the top-level of the tarball

  This allows writing the link type unconditionally. Previously, bootstrap
  had to keep track of whether the file IO *would* succeed (it would fail
  if `lib/` didn't exist), which was prone to bugs.

- Make `link-type.txt` required

  Anyone downloading this from CI should be using a version of bootstrap
  that matches the version of the uploaded artifacts. So a missing
  link-type indicates a bug in x.py.
... and update the comment in `config.toml.example`
@jyn514 jyn514 force-pushed the download-windows-llvm branch from dd1f5dc to 5c4adbe Compare January 16, 2021 03:09
@jyn514
Copy link
Member Author

jyn514 commented Jan 16, 2021

@bors r=Mark-Simulacrum

Thanks for the review!

@bors
Copy link
Contributor

bors commented Jan 16, 2021

📌 Commit 5c4adbe 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 Jan 16, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
…k-Simulacrum

Allow downloading LLVM on Windows and MacOS

- Don't ignore packaging `llvm/lib/` for `rust-dev` when LLVM is linked
statically
- Add `link-type.txt` so bootstrap knows whether llvm was linked
  statically or dynamically
- Don't assume CI LLVM is linked dynamically in `bootstrap::config`
- Fall back to dynamic linking if `link-type.txt` doesn't exist
- Fix existing bug that split the output of `llvm-config` on lines, not spaces
- Only special case MacOS when dynamic linking. Static linking works fine.
- Enable building LLVM tests

  This works around the following llvm bug:

  ```
  llvm-config: error: component libraries and shared library

  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest.a
  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest_main.a
  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libLLVMTestingSupport.a
  thread 'main' panicked at 'command did not execute successfully: "/home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-config" "--libfiles"
  ```

  I'm not sure why llvm-config thinks these are required, but to avoid
  the error, this builds them anyway.

- Bump version of `download-ci-llvm-stamp`

  `src/llvm-project` hasn't changed, but the generated tarball has.

Fixes rust-lang#77084.

# Current Status

This works on both MacOS and Windows! 🎉 🎉 Thanks to `@nagisa,` `@halkcyon,` `@Lokathor,` `@jryans,` and `@poliorcetics` for helping me test!

The `if-available` check now supports all tier 1 platforms. Although only x64 apple and x64 msvc have been tested, none of the changes here are Windows or Mac specific, and I expect this to work anywhere that LLVM artifacts are uploaded to CI (i.e. the `rust-dev` component exists).

## Windows

Note that if you have an old version of MSVC build tools you'll need to update them. VS Build Tools 2019 14.28 and later are known to work. With old tools, you may see an error like the following:

```
error LNK2001: unresolved external symbol __imp___std_init_once_complete
```
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 17, 2021
…k-Simulacrum

Allow downloading LLVM on Windows and MacOS

- Don't ignore packaging `llvm/lib/` for `rust-dev` when LLVM is linked
statically
- Add `link-type.txt` so bootstrap knows whether llvm was linked
  statically or dynamically
- Don't assume CI LLVM is linked dynamically in `bootstrap::config`
- Fall back to dynamic linking if `link-type.txt` doesn't exist
- Fix existing bug that split the output of `llvm-config` on lines, not spaces
- Only special case MacOS when dynamic linking. Static linking works fine.
- Enable building LLVM tests

  This works around the following llvm bug:

  ```
  llvm-config: error: component libraries and shared library

  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest.a
  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libgtest_main.a
  llvm-config: error: missing: /home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/lib/libLLVMTestingSupport.a
  thread 'main' panicked at 'command did not execute successfully: "/home/joshua/rustc2/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-config" "--libfiles"
  ```

  I'm not sure why llvm-config thinks these are required, but to avoid
  the error, this builds them anyway.

- Bump version of `download-ci-llvm-stamp`

  `src/llvm-project` hasn't changed, but the generated tarball has.

Fixes rust-lang#77084.

# Current Status

This works on both MacOS and Windows! 🎉 🎉 Thanks to ``@nagisa,`` ``@halkcyon,`` ``@Lokathor,`` ``@jryans,`` and ``@poliorcetics`` for helping me test!

The `if-available` check now supports all tier 1 platforms. Although only x64 apple and x64 msvc have been tested, none of the changes here are Windows or Mac specific, and I expect this to work anywhere that LLVM artifacts are uploaded to CI (i.e. the `rust-dev` component exists).

## Windows

Note that if you have an old version of MSVC build tools you'll need to update them. VS Build Tools 2019 14.28 and later are known to work. With old tools, you may see an error like the following:

```
error LNK2001: unresolved external symbol __imp___std_init_once_complete
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2021
Rollup of 13 pull requests

Successful merges:

 - rust-lang#79298 (correctly deal with late-bound lifetimes in anon consts)
 - rust-lang#80031 (resolve: Reject ambiguity built-in attr vs different built-in attr)
 - rust-lang#80201 (Add benchmark and fast path for BufReader::read_exact)
 - rust-lang#80635 (Improve diagnostics when closure doesn't meet trait bound)
 - rust-lang#80765 (resolve: Simplify collection of traits in scope)
 - rust-lang#80932 (Allow downloading LLVM on Windows and MacOS)
 - rust-lang#80983 (Remove is_dllimport_foreign_item definition from cg_ssa)
 - rust-lang#81064 (Support non-stage0 check)
 - rust-lang#81080 (Force vec![] to expression position only)
 - rust-lang#81082 (BTreeMap: clean up a few more comments)
 - rust-lang#81084 (Use Option::map instead of open-coding it)
 - rust-lang#81095 (Use Option::unwrap_or instead of open-coding it)
 - rust-lang#81107 (Add NonZeroUn::is_power_of_two)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cfe2253 into rust-lang:master Jan 17, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 17, 2021
@jyn514 jyn514 deleted the download-windows-llvm branch January 17, 2021 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use dynamic linking and download LLVM from CI for rustc across platforms
7 participants