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

1.48 beta installer spuriously installing libLLVM.so, also into the incorrect location #78932

Closed
infinity0 opened this issue Nov 10, 2020 · 9 comments · Fixed by #78986
Closed
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Milestone

Comments

@infinity0
Copy link
Contributor

Since 1.48 beta, x.py install is now installing libLLVM.so even when dynamically linking against the system one.

Not only that, the install path is unaware of the target architecture, which breaks during cross-compiling. I am not sure exactly what happens - possibly (a) the host is installed instead of the target or (b) both are installed, and the host clobbers the target - in any case the end result I observe is that the host-arch libLLVM.so ends up in the install directory instead of the target-arch one, which would be correct.

@infinity0 infinity0 added the C-bug Category: This is a bug. label Nov 10, 2020
@infinity0
Copy link
Contributor Author

@Mark-Simulacrum Possibly to do with the new RustDev component and especially this snippet:

        // Copy libLLVM.so to the target lib dir as well, so the RPATH like
        // `$ORIGIN/../lib` can find it. It may also be used as a dependency
        // of `rustc-dev` to support the inherited `-lLLVM` when using the
        // compiler libraries.
        maybe_install_llvm(builder, target, &image.join("lib"));

This is a regression; 1.47 had the correct behaviour of not trying to install LLVM at all. Note that I am not explicitly referencing RustDev at all, I am just calling x.py install.

@Mark-Simulacrum Mark-Simulacrum added P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Nov 10, 2020
@Mark-Simulacrum Mark-Simulacrum self-assigned this Nov 10, 2020
@Mark-Simulacrum Mark-Simulacrum added this to the 1.48.0 milestone Nov 10, 2020
@Mark-Simulacrum
Copy link
Member

I plan to investigate fixing this soon, I think it's important we get this fixed.

Could you provide instructions on how to verify things are working so I can check locally? (I guess I can compare against a 1.47 build but would be good to get what command you're running).

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Nov 11, 2020
@infinity0
Copy link
Contributor Author

in config.toml:

[llvm]
link-shared = true
[target]
llvm-config = "/usr/lib/llvm-11/bin/llvm-config"

then with x.py install, the tarballs shouldn't contain a libLLVM.so but they do.

The documentation for llvm-config says "Note that if this is specified we don't compile LLVM at all for this target." - so possibly this bug can be fixed simply by changing this snippet in src/bootstrap/dist.rs:

    if !builder.config.llvm_link_shared {
        // We do not need to copy LLVM files into the sysroot if it is not
        // dynamically linked; it is already included into librustc_llvm
        // statically.
        return;
    }

to check for whether llvm-config was set or not.

@infinity0
Copy link
Contributor Author

The cross-compiling wrong-location/clobbering issue still exists as I described in the OP, but it does not affect my use-case since we're not installing libLLVM.so.

On Debian we are in fact moving everything from /usr/lib to /usr/lib/${gnu_arch_triplet} after x.py install, but I am not sure how cross-platform this is for you to adopt. I'm also not sure why it is the libLLVM.so specifically that ends up with the wrong architecture, but the other libraries (e.g. libstd.so) are fine.

@cuviper
Copy link
Member

cuviper commented Nov 12, 2020

I think it was actually #76349's commit a7b092f that changed this, specifically around here:
a7b092f#diff-2c56335faa24486df09ba392d8900c57e2fac4633e1f7038469bcf9ed3feb871L2395

Before, we would only copy libLLVM.so if it was found in the src_libdir. Now it copies everything that llvm-config --libfiles says, even if that llvm-config is external. I also see that prebuilt_llvm_config has a short path for custom llvm-config:

// If we're using a custom LLVM bail out here, but we can only use a
// custom LLVM for the build triple.
if let Some(config) = builder.config.target_config.get(&target) {
if let Some(ref s) = config.llvm_config {
check_llvm_version(builder, s);
return Ok(s.to_path_buf());
}
}

I think it would work to change maybe_install_llvm to skip out on custom llvm-config, as @infinity0 suggests.

@Mark-Simulacrum
Copy link
Member

Though one could argue that is wrong (in some cases) right? That is, if you don't copy, it'll not work unless the original location is in LD_LIBRARY_PATH or equivalent.

I'd be fine changing/reverting to not copy on custom llvm though, that seems like the right fix for now - it seems rare to have custom LLVM and want the copy.

@Mark-Simulacrum
Copy link
Member

Filed #78986 with the suggested fix here.

@cuviper
Copy link
Member

cuviper commented Nov 12, 2020

I see it the same as any other system library that we might link against. e.g. Cargo links to the system openssl by default, and we don't try to pull that into dist tarballs.

@Mark-Simulacrum
Copy link
Member

Good point :)

@bors bors closed this as completed in e3d52b8 Nov 15, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 16, 2020
Install CI llvm into the library directory

In other words, my concern in rust-lang#78932 (comment) was perfectly justified by something we were already doing. For now just special case CI LLVM, but in the future we may want a more general fix.

Fixes rust-lang#79071.

r? `@alexcrichton`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants