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

Prevent silently falling back to dynamic linking when static linking is explicitly requested #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

golddranks
Copy link

There were two problems with the current code:

  1. if lib flags are parsed before directory flags, the full search paths are not available yet, and a library may be mistakenly deemed as a system library. Resolution: process the lib flags only after the loop, when search paths are fully known.

  2. The variable statik is true when static linking has been explicitly requested. However, the is_system check can prevent the static branch from being selected, and dynamic link flags are passed. Because static build has been explicitly requested, it would be better just to panic with an error message instead of doing unexpected behaviour.

@alexcrichton
Copy link
Member

Hm is this really the behavior we want? This means that if you set PKG_CONFIG_ALL_STATIC you're basically guaranteed to get a build failure in most situations.

I could imagine though that if you say config.statik(true) we should generate an error though?

@golddranks
Copy link
Author

Hm, I thought that that flag is precisely for signalling that you want to have everything linked statically? I couldn't imagine setting that flag unless I'm deliberately trying to link everything statically. But that might very well be because of my lack of imagination :D

@kornelski
Copy link
Contributor

kornelski commented Mar 2, 2017

Is it possible to check whether the static library exists in the filesystem?

On macOS some critical libraries are dynamic only, e.g. /usr/lib/libSystem.B.dylib, /usr/lib/libobjc.A.dylib, /System/Library/Frameworks/*.

OTOH I need everything else to be really static (this might require cooperation from the gcc crate too). For example, on macOS gcc does not produce redistributable executables unless you compile with -static-libgcc (otherwise it dynamically links to /usr/local/lib/gcc/6/libgcc_s.1.dylib which is only installed with the compiler, and no normal user has this).

Similarly /usr/local/opt/gcc/lib/gcc/6/libgomp.1.dylib is very compiler-specific and nobody has it, so it must be linked statically for the executable to work.

@alexcrichton
Copy link
Member

@golddranks my intention with this was to just pass --static to pkg-config itself, not do much more than that. I'd inteprete .statik(true) as a request that only the library in question is desired to be static, but not necessarily saying anything about other libraries.

@pornel yeah makes sense to me that we'd use the filesystem to drive decisions about linkage

@golddranks
Copy link
Author

Ah, I see. In that case, (a part of) the current behaviour makes totally sense.

I still think that resolving whether a lib is a system lib or not before knowing all the search paths may be a bug; what do you think?

For example: pkg-config --libs --cflags --static openssl-I/musl/include -lssl -lcrypto -L/musl/lib -ldl. The -L flag is in the middle, and that's why the search path isn't yet known when checking whether ssl and crypto are system libs. (Btw. -ldl seemed an odd library to link when linking statically, but in a discussion I found from the internet it was said that it shouldn't matter if it's a musl version of it that is meant to be linked statically.) So at least if it checks the libs, it should do that only after all the search paths are known, right? Or then there is some significance to the order of -l and -L flags that's escaping me.

@alexcrichton
Copy link
Member

I mean in general linkage is just impossible to get right. Everyone wants something different so no matter what heuristic you have someone will complain that it's not correct for them. If you have all the heuristics then others will complain that it's too complicated and they can't just do what they know is right.

I don't mind bug fixes of course, but there's got to be a line somewhere at some point... Where do lookup paths come into play here? I don't think this crate currently probes the filesystem yet?

@golddranks
Copy link
Author

The lookup paths come into play because the is_system check currently performed by this crate uses them. It actually probes with fs::metadata() whether the library exists:

        !d.starts_with(root) && fs::metadata(&d.join(&libname)).is_ok()

@golddranks
Copy link
Author

This check was introduced in this commit, if it helps to refresh your memory of its purpose: 9a57960

@alexcrichton
Copy link
Member

Could this change to just check self.statik instead of statik? That'd be fine by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants