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

Don't force system LLVM libs to be linked staticly #28327

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@wthrowe
Copy link
Contributor

wthrowe commented Sep 10, 2015

This allows building against an external LLVM installation that only includes dynamic libraries. When linking against bundled LLVM we still force static linking so the bundled LLVM libraries don't have to be installed.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 10, 2015

r? @huonw

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

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 10, 2015

Unfortunately I don't think this is quite right because an external LLVM doesn't automatically imply non-static linkage (e.g. all external LLVM roots I've ever used have been static still). I think that #27937 is handling this as well (detecting there's only one LLVM lib and linking that dynamically), so I'm going to close this in favor of that. Thanks for the PR though!

@wthrowe

This comment has been minimized.

Copy link
Contributor Author

wthrowe commented Sep 10, 2015

#27937 does not fix this issue. With that patch rustc still fails to build on my system with lots of missing library errors.

This should still work if the external LLVM root is static, because #[link(name = "something")] will still link to a static library if there are no dynamic ones that match. Although, now I'm not sure why kind = "static" is needed for the bundled case, since that only supplies static libs. (It is necessary, I tried building without and it didn't work.)

@DiamondLovesYou

This comment has been minimized.

Copy link
Contributor

DiamondLovesYou commented Sep 10, 2015

@wthrowe The reason it fails is because this hasn't landed into LLVM. Without it, llvm-config doesn't know that only the shared objects are installed and thus prints out all of the static archives. This, in turn, messes up mklldeps.py.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 10, 2015

Linking via #[link(name = "...")] implies dynamic linkage so all downstream crates get linked with -lfoo as well, which is incorrect in the case of a static library (which needs to only be linked once).

Perhaps this could be updated to instead detect when libLLVM.so exists and only link to that dynamically? At least until that patch in LLVM lands.

@wthrowe

This comment has been minimized.

Copy link
Contributor Author

wthrowe commented Sep 10, 2015

I think that, while #[link(name = "foo")] is happy to link to a static library, it is still recorded in crate metadata as a transitive linking requirement, which can screw up linking against the crate in some circumstances. (Looks like @alexcrichton just said the same thing.)

I'll continue the discussion of #27937 over there, since the approach in this pull request isn't going to work without a lot of modification to something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.