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

Remove libtest's dylib #119475

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Remove libtest's dylib #119475

merged 1 commit into from
Jan 4, 2024

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Dec 31, 2023

libtest.so is only used by rustdoc, and tests seem to pass locally with this change. I suppose if this is broken, the only way to find out is to make a PR.

@rustbot
Copy link
Collaborator

rustbot commented Dec 31, 2023

r? @thomcc

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 31, 2023
@saethlin
Copy link
Member Author

cc @rust-lang/bootstrap @rust-lang/rustdoc I figure if this breaks anything it'll be one of you?

@thomcc
Copy link
Member

thomcc commented Dec 31, 2023

This is fine by me if it doesn't break anything, although IDK that I'm the right person to evaluate that.

@GuillaumeGomez
Copy link
Member

I don't see a case where rustdoc actually uses the dynamic lib. Maybe someone else from the team might be aware of such a case?

@Mark-Simulacrum
Copy link
Member

@bors try

We can check, but my guess is that this either makes libtest statically linked into rustdoc's binary or breaks that binary:

~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu$ ldd -r bin/rustdoc
        linux-vdso.so.1 (0x00007ffe45bfe000)
        libtest-4455820ab80a9235.so => /home/mark/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/libtest-4455820ab80a9235.so (0x00007f460aa21000)
        librustc_driver-0e1ab94fca9b73ca.so => /home/mark/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-0e1ab94fca9b73ca.so (0x00007f4604a00000)
        libstd-91a8c1dba89c667e.so => /home/mark/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/libstd-91a8c1dba89c667e.so (0x00007f460a8fb000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f460a8e7000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f460a8c7000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f46049fb000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f46049f6000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f460490f000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f46046e7000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f460b490000)
        libLLVM-17-rust-1.77.0-nightly.so => /home/mark/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/../lib/libLLVM-17-rust-1.77.0-nightly.so (0x00007f45fce00000)
        libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007f45fcde4000)

If rustdoc is the only binary we ship that uses libtest then this seems OK to statically link (more efficient, likely smaller on disk due to better dead code removal). But otherwise we probably want to keep it dynamically linked to reduce bandwidth/space use.

@bors
Copy link
Contributor

bors commented Dec 31, 2023

⌛ Trying commit c0fa85e with merge c657332...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 31, 2023
Remove libtest's dylib

libtest.so is only used by rustdoc, so  Tests seem to pass locally with this change. I suppose if this is broken, the only way to find out is to make a PR.
@saethlin
Copy link
Member Author

saethlin commented Dec 31, 2023

I started looking into this because the only binary I could find that's linked to libtest.so is rustdoc.

@bjorn3
Copy link
Member

bjorn3 commented Dec 31, 2023

We should currently also be using libtest as dylib if another dependency is only available as dylib (which forces -Cprefer-dynamic) or if -Cprefer-dynamic is used. I can't think of a case where you would actually depend on this though, except maybe if you have a lot of tests and want to reduce disk usage.

@bors
Copy link
Contributor

bors commented Dec 31, 2023

☀️ Try build successful - checks-actions
Build commit: c657332 (c6573324f4c1a53a7b1849da7398de8f08e1dfb0)

@cuviper
Copy link
Member

cuviper commented Jan 4, 2024

Let's do it! (and it's trivial to revert if we're wrong...)

@bors r+

@bors
Copy link
Contributor

bors commented Jan 4, 2024

📌 Commit c0fa85e has been approved by cuviper

It is now in the queue for this repository.

@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 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 4, 2024
…uviper

Remove libtest's dylib

libtest.so is only used by rustdoc, and tests seem to pass locally with this change. I suppose if this is broken, the only way to find out is to make a PR.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#117636 (add test for rust-lang#117626)
 - rust-lang#118704 (Promote `riscv32{im|imafc}` targets to tier 2)
 - rust-lang#119184 (Switch from using `//~ERROR` annotations with `--error-format` to `error-pattern`)
 - rust-lang#119325 (custom mir: make it clear what the return block is)
 - rust-lang#119391 (Use Result::flatten in catch_with_exit_code)
 - rust-lang#119431 (Support reg_addr register class in s390x inline assembly)
 - rust-lang#119475 (Remove libtest's dylib)
 - rust-lang#119532 (Make offset_of field parsing use metavariable which handles any spacing)
 - rust-lang#119553 (stop feed vis when cant access for trait item)
 - rust-lang#119556 (Reland optimized-compiler-builtins config)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#117636 (add test for rust-lang#117626)
 - rust-lang#118704 (Promote `riscv32{im|imafc}` targets to tier 2)
 - rust-lang#119184 (Switch from using `//~ERROR` annotations with `--error-format` to `error-pattern`)
 - rust-lang#119325 (custom mir: make it clear what the return block is)
 - rust-lang#119391 (Use Result::flatten in catch_with_exit_code)
 - rust-lang#119431 (Support reg_addr register class in s390x inline assembly)
 - rust-lang#119475 (Remove libtest's dylib)
 - rust-lang#119532 (Make offset_of field parsing use metavariable which handles any spacing)
 - rust-lang#119553 (stop feed vis when cant access for trait item)
 - rust-lang#119574 (Miri subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 99a8c33 into rust-lang:master Jan 4, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
Rollup merge of rust-lang#119475 - saethlin:remove-libtest-dylib, r=cuviper

Remove libtest's dylib

libtest.so is only used by rustdoc, and tests seem to pass locally with this change. I suppose if this is broken, the only way to find out is to make a PR.
@saethlin saethlin deleted the remove-libtest-dylib branch January 4, 2024 22:32
@tmandry
Copy link
Member

tmandry commented Jan 4, 2024

Hi!! 👋 This broke my project (Fuchsia), because we expect libtest to exist in toolchain builds and dynamically link to it for tests.

I suppose we could decide to stop doing that, but it could have performance implications on our testing.

What's the motivation for removing it?

@saethlin
Copy link
Member Author

saethlin commented Jan 4, 2024

What's the motivation for removing it?

Removing it decreases the size of the distribution that everyone downloads every time they install/upgrade a Rust toolchain with rustup.

@tmandry
Copy link
Member

tmandry commented Jan 4, 2024

Sure, makes sense. I'll tell you if we notice any regressions. I think ideally this would be a per-target option or something.

@cuviper
Copy link
Member

cuviper commented Jan 4, 2024

I suppose we could decide to stop doing that, but it could have performance implications on our testing.

It would be great if you're able to provide any measurements on that!

@thomcc
Copy link
Member

thomcc commented Jan 4, 2024

I think ideally this would be a per-target option or something.

The fact that lib.crate-type can't be specified per-target has bothered me for a while (not enough to file an RFC for it or anything tho).

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants