Skip to content

Allow macOS to build LLVM as shared library - #98418

Merged
bors merged 1 commit into
rust-lang:masterfrom
topjohnwu:macos-dylib
Jul 2, 2022
Merged

Allow macOS to build LLVM as shared library#98418
bors merged 1 commit into
rust-lang:masterfrom
topjohnwu:macos-dylib

Conversation

@topjohnwu

@topjohnwutopjohnwu commented Jun 23, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Inspired by how homebrew builds and distributes llvm, here we manually create a symlink with a versioned dylib path to make llvm-config work properly. Note, the resulting rustc executable and librustc_driver-<hash>.dylib still links to the un-versioned libLLVM.dylib as expected when distributed in the final output. I have confirmed this by checking otool -L on both binaries.

After the change, enabling llvm.link-shared and llvm.thin-lto will be possible on macOS.

@rust-highfive

rust-highfive commented Jun 23, 2022

Copy link
Copy Markdown
Contributor

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

Please see the contribution instructions for more information.

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2022
jyn514
jyn514 suggested changes Jun 23, 2022
View reviewed changes

@jyn514jyn514 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How likely is this to break? Has homebrew ever had to change the way they do this?

Comment threadsrc/bootstrap/native.rs Outdated
Comment threadsrc/bootstrap/native.rs Outdated
@topjohnwu

topjohnwu commented Jun 23, 2022

Copy link
Copy Markdown
ContributorAuthor

How likely is this to break? Has homebrew ever had to change the way they do this?

llvm-config has not changed its behavior a very long time, and I don't expect it to change anytime soon. The issue was raised and reported in 2019 Homebrew/homebrew-core#47146, at that time the symlink workaround was working but not accepted. Eventually the homebrew folks decided to merge the same "fix" later Homebrew/homebrew-core#76646, you can check out some of the discussions there.

IMO, the workaround should be pretty stable. I'll be tracking closely whether LLVM has any upstream fixes as I'm building rustc regularly for the ONDK Project

@Mark-Simulacrum

Mark-Simulacrum commented Jun 23, 2022

Copy link
Copy Markdown
Member

Has this been reported upstream to LLVM? Is there a reason to fix this in our code, vs. in LLVM?

@topjohnwu

topjohnwu commented Jun 23, 2022

Copy link
Copy Markdown
ContributorAuthor

Has this been reported upstream to LLVM? Is there a reason to fix this in our code, vs. in LLVM?

Yup, you can check out this comment. There is no response in upstream LLVM, which is why Homebrew decided to workaround the issue instead.

@topjohnwu
topjohnwu requested a review from jyn514June 23, 2022 21:01
jyn514
jyn514 approved these changes Jun 23, 2022
View reviewed changes

@jyn514jyn514 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM except for the nits

Comment threadsrc/bootstrap/native.rs Outdated
Comment threadsrc/bootstrap/native.rs Outdated
@jyn514jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 23, 2022
@topjohnwu
topjohnwu requested a review from jyn514June 24, 2022 12:05
jyn514
jyn514 suggested changes Jun 24, 2022
View reviewed changes

@jyn514jyn514 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

r=me with the dry_run check added

Comment threadsrc/bootstrap/lib.rs Outdated
Comment threadsrc/bootstrap/native.rs Outdated
@jyn514jyn514 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 Jun 24, 2022
@jyn514jyn514 assigned jyn514 and unassigned Mark-SimulacrumJun 24, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@topjohnwu
Allow dynamically linking against libLLVM on macOS
b6e28b5
Create symlinks to workaround file missing error in llvm-config
@topjohnwu
topjohnwu requested a review from jyn514June 24, 2022 22:22
@jyn514

jyn514 commented Jun 24, 2022

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Jun 24, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit b6e28b5 has been approved by jyn514

@borsbors 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 Jun 24, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 29, 2022
@GuillaumeGomez
Rollup merge ofrust-lang#98418- topjohnwu:macos-dylib, r=jyn514
744059c
Allow macOS to build LLVM as shared library
Inspired by how [homebrew](https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/llvm.rb) builds and distributes llvm, here we manually create a symlink with a versioned dylib path to make `llvm-config` work properly. Note, the resulting `rustc` executable and `librustc_driver-<hash>.dylib` still links to the un-versioned `libLLVM.dylib` as expected when distributed in the final output. I have confirmed this by checking `otool -L` on both binaries.
After the change, enabling `llvm.link-shared` and `llvm.thin-lto` will be possible on macOS.
@GuillaumeGomezGuillaumeGomez mentioned this pull request Jun 29, 2022
Closed
@GuillaumeGomezGuillaumeGomez mentioned this pull request Jul 1, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 1, 2022
@bors
Auto merge ofrust-lang#98781- GuillaumeGomez:rollup-798kb8u, r=Guil…
9a6fa4f
…laumeGomez
Rollup of 5 pull requests
Successful merges:
- rust-lang#97249 (`<details>`/`<summary>` UI fixes)
- rust-lang#98418 (Allow macOS to build LLVM as shared library)
- rust-lang#98460 (Use CSS variables to handle theming)
- rust-lang#98497 (Improve some inference diagnostics)
- rust-lang#98708 (rustdoc: fix 98690 Panic if invalid path for -Z persist-doctests)
Failed merges:
- rust-lang#98761 (more `need_type_info` improvements)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 01aa10c into rust-lang:masterJul 2, 2022
@rustbotrustbot added this to the 1.64.0 milestone Jul 2, 2022
@topjohnwu
topjohnwu deleted the macos-dylib branch July 2, 2022 07:50
@topjohnwutopjohnwu mentioned this pull request Jul 4, 2022
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 5, 2022
@GuillaumeGomez
Rollup merge ofrust-lang#98880- topjohnwu:macos-dylib-cross, r=jyn514
3fe0191
Proper macOS libLLVM symlink when cross compiling
Follow up of rust-lang#98418
When cross compiling on macOS with `llvm.link-shared` enabled, the symlink creation will fail after compiling LLVM for the target architecture, because it will attempt to create the symlink in the host LLVM directory, which was already created when being built.
This commit changes the symlink path to the actual LLVM output.
r? `@jyn514`
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jyn514jyn514Awaiting requested review from jyn514

Assignees

@jyn514jyn514

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-bootstrapRelevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Milestone

1.64.0

Development

Successfully merging this pull request may close these issues.

7 participants

@topjohnwu@rust-highfive@Mark-Simulacrum@rust-log-analyzer@jyn514@bors@rustbot