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

Replace libLLVM symlink with linker script #121967

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Mar 4, 2024

It turns out that the libLLVM-N.so -> libLLVM.so.N.1 symlink is also needed when projects like miri link against librustc_driver.so. As such, we have to distribute it in real rustup components like rustc-dev, rather than only for download-ci-llvm.

To avoid actually distributing symlinks (which are not supported or not fully supported by the rustup infrastructure) replace it with a linker script that does the same thing instead.

Fixes #121889.

r? @cuviper

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 4, 2024
@nikic
Copy link
Contributor Author

nikic commented Mar 4, 2024

@bors try

@bors
Copy link
Contributor

bors commented Mar 4, 2024

⌛ Trying commit 3501f4a with merge f73e623...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
Replace libLLVM symlink with linker script

It turns out that the libLLVM-N.so -> libLLVM.so.N.1 symlink is also needed when projects like miri link against librustc_driver.so. As such, we have to distribute it in real rustup components like rustc-dev, rather than only for download-ci-llvm.

To avoid actually distributing symlinks (which are not supported or not fully supported by the rustup infrastructure) replace it with a linker script that does the same thing instead.

Fixes rust-lang#121889.

r? `@cuviper`
@bors
Copy link
Contributor

bors commented Mar 4, 2024

☀️ Try build successful - checks-actions
Build commit: f73e623 (f73e623e5edfb62d8c85784c0c5acb62ea094813)

@nikic
Copy link
Contributor Author

nikic commented Mar 4, 2024

Breaks download-ci-llvm, because that doesn't have the symlink now either, so the new logic doesn't apply to it anymore.

I'll restore the symlink for the download-ci-llvm case.

It turns out that the libLLVM-N.so -> libLLVM.so.N.1 symlink is
also needed when projects like miri link against librustc_driver.so.
As such, we have to distribute it in real rustup components like
rustc-dev, rather than only for download-ci-llvm.

To avoid actually distributing symlinks (which are not supported
or not fully supported by the rustup infrastructure) replace it
with a linker script that does the same thing instead.
@nikic
Copy link
Contributor Author

nikic commented Mar 4, 2024

@bors try

@bors
Copy link
Contributor

bors commented Mar 4, 2024

⌛ Trying commit 5d1d408 with merge 618690d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
Replace libLLVM symlink with linker script

It turns out that the libLLVM-N.so -> libLLVM.so.N.1 symlink is also needed when projects like miri link against librustc_driver.so. As such, we have to distribute it in real rustup components like rustc-dev, rather than only for download-ci-llvm.

To avoid actually distributing symlinks (which are not supported or not fully supported by the rustup infrastructure) replace it with a linker script that does the same thing instead.

Fixes rust-lang#121889.

r? `@cuviper`
@bors
Copy link
Contributor

bors commented Mar 4, 2024

☀️ Try build successful - checks-actions
Build commit: 618690d (618690d791b4cc1d90b113179a25ae3cd7189ed2)

Copy link
Contributor

@nbdd0121 nbdd0121 left a comment

Choose a reason for hiding this comment

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

Code looks good, although it's a bit sad that w have separate code path for download-ci-llvm and everything else.

@nikic
Copy link
Contributor Author

nikic commented Mar 4, 2024

I've confirmed that download-ci-llvm works again now, and also confirmed that miri can be built against this toolchain (I had to edit miri-script to disable rustfmt and clippy, which are not part of the try toolchain).

@RalfJung
Copy link
Member

RalfJung commented Mar 4, 2024

Breaks download-ci-llvm, because that doesn't have the symlink now either, so the new logic doesn't apply to it anymore.

Why does the linker script not work for download-ci-llvm?

@nikic
Copy link
Contributor Author

nikic commented Mar 4, 2024

Breaks download-ci-llvm, because that doesn't have the symlink now either, so the new logic doesn't apply to it anymore.

Why does the linker script not work for download-ci-llvm?

It does work for the purposes of linking, but it breaks the code being modified here that handles libLLVM installation. The logic is based around having the symlink. If we don't ship the symlink for download-ci-llvm, then the logic would additionally also have to support reading the linker script to determine which shared object it needs to install in the libdir.

@RalfJung
Copy link
Member

RalfJung commented Mar 4, 2024

Oh, this function later works on its own output as input? Wow that's recursive.^^

Might be worth having a comment in the code explaining that.

@workingjubilee
Copy link
Member

To avoid actually distributing symlinks (which are not supported or not fully supported by the rustup infrastructure) replace it with a linker script that does the same thing instead.

This is partly because the OS infrastructure we run all this stuff on doesn't always support this very well. It is a royal pain in the ass to create symlinks on Windows/NTFS. Windows 10 has made it significantly easier, and with the deprecation of Windows 7 support, that's closer to an option, but I don't know if it's easy enough to be able to assume symlinks are viable on Windows yet, because I don't remember which patch it was. When Windows 11 is our minimum, maybe.

@bjorn3
Copy link
Member

bjorn3 commented Mar 5, 2024

On Windows 10 it still requires developer mode to be enabled, right?

@mati865
Copy link
Contributor

mati865 commented Mar 5, 2024

Both Windows 10 and 11 require either developer mode or modification of group policies.

@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2024

@cuviper this is blocking all tools linking against rustc_driver, would be nice to get it landed soon. :)

@Mark-Simulacrum
Copy link
Member

@bors r+

The risk here seems fairly low (I expect to hit some problems but that's almost always true with changes like this). In the meantime, it should unblock miri etc.

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit 5d1d408 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 6, 2024
@RalfJung
Copy link
Member

RalfJung commented Mar 6, 2024 via email

@bors
Copy link
Contributor

bors commented Mar 6, 2024

⌛ Testing commit 5d1d408 with merge bfe762e...

@bors
Copy link
Contributor

bors commented Mar 6, 2024

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing bfe762e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 6, 2024
@bors bors merged commit bfe762e into rust-lang:master Mar 6, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 6, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bfe762e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.8% [2.0%, 4.2%] 3
Regressions ❌
(secondary)
2.7% [0.9%, 3.6%] 12
Improvements ✅
(primary)
-1.4% [-1.4%, -1.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.7% [-1.4%, 4.2%] 4

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 646.923s -> 644.65s (-0.35%)
Artifact size: 175.06 MiB -> 175.06 MiB (0.00%)

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 7, 2024
…rk-Simulacrum

Include all library files in artifact summary on CI

It's not worth it to maintain any custom logic here. Just print all files in the `lib` directory, this should be forward compatible.

This fixes rust-lang#121866, based on rust-lang#121967.

r? `@Mark-Simulacrum`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 7, 2024
Record mtime in bootstrap's LLVM linker script

As discovered in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60ui.60.20tests.20re-running.3F the linker script added in rust-lang#121967 can trigger rebuilds or new test executions, as it's more recent than some of the existing files themselves.

This PR copies the mtime to the linker script to prevent a second invocation of `./x test tests/ui` from rerunning all of the tests.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Rollup merge of rust-lang#122138 - lqd:llvm-mtime, r=clubby789

Record mtime in bootstrap's LLVM linker script

As discovered in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.60ui.60.20tests.20re-running.3F the linker script added in rust-lang#121967 can trigger rebuilds or new test executions, as it's more recent than some of the existing files themselves.

This PR copies the mtime to the linker script to prevent a second invocation of `./x test tests/ui` from rerunning all of the tests.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
Rollup merge of rust-lang#122136 - Kobzol:opt-dist-lookup-logic, r=Mark-Simulacrum

Include all library files in artifact summary on CI

It's not worth it to maintain any custom logic here. Just print all files in the `lib` directory, this should be forward compatible.

This fixes rust-lang#121866, based on rust-lang#121967.

r? `@Mark-Simulacrum`
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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet