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

Including system library paths makes it difficult to overide library versions #11

Open
hugoduncan opened this issue Mar 11, 2015 · 29 comments

Comments

@hugoduncan
Copy link

At present, Config::find returns system library paths. This can make it difficult to override library versions.

The motiviating example is using rust-openssl with ssh2-rs on OS X. On Mac, openssl requires a non-system version of openssl for TLS support, which can successfuly be provided by homebrew, for example. ssh2-rs depends on libz-rs, which pkg-config finds as -L /usr/lib. This library path gets added before the openssl library path, resulting in the system (older) openssl libraries being used, and the linker failing.

@alexcrichton
Copy link
Member

Currently system library paths are required to allow the compiler to find static versions of libraries, but I can see where this would cause problems with multiple libs on the system...

@hugoduncan
Copy link
Author

Could the compiler not add the system default paths, independently of pkg-config, at the end of the arguments list (so they have the lowest priority)?

@alexcrichton
Copy link
Member

Perhaps, but the compiler has no knowledge of the default paths for a system (like the linker does)

@hugoduncan
Copy link
Author

What would the process be, to move towards a solution for this?

@alexcrichton
Copy link
Member

I can't really think of a great solution for this, so it would probably involve coming up with one first :)

@hugoduncan
Copy link
Author

I'm not familiar with all the tooling rust uses to compile. Is there some place I can read up on the linkers used on different platforms? Could the compiler query the linker for the default paths?

@alexcrichton
Copy link
Member

We generally just shell out to ld, although I'm not sure if we can query ld for search paths.

@julienw
Copy link

julienw commented Apr 26, 2016

So in our project we ended up on exactly the same issue, and no easy solution. Of course the root cause is the awful setup of libssl on MacOS X but we need to find a way around this...

I still don't properly get why we need the system paths for the static libs to be found. Isn't it something that ld can find out by itself ?

@julienw
Copy link

julienw commented Apr 26, 2016

If we really need it, here is a dumb idea: call pkg-config twice:

  • once with PKG_CONFIG_ALLOW_SYSTEM_LIBS
  • once without PKG_CONFIG_ALLOW_SYSTEM_LIBS

Diff the 2 outputs. Then we'd know which libs are the system libs.

@alexcrichton
Copy link
Member

I still don't properly get why we need the system paths for the static libs to be found. Isn't it something that ld can find out by itself ?

The compiler needs to load the actual .a archive file to work with it much of the time, so we can't defer it to the linker.

Diff the 2 outputs. Then we'd know which libs are the system libs.

Perhaps!

@julienw
Copy link

julienw commented Apr 27, 2016

I just re-read the manual page for pkg-config. Actually the list of paths that pkg-config considers as "system" seems to be hard-coded:

       PKG_CONFIG_ALLOW_SYSTEM_LIBS
              Don't strip -L/usr/lib or -L/lib out of libs.

So maybe it's good enough to just special-case these paths ?

@alexcrichton
Copy link
Member

I'd be game!

@julienw
Copy link

julienw commented Apr 27, 2016

So now, how do we special case them ? Do you think rustc could actually automatically always add them (and then we can remove them from pkg-config output)? Or would it be too fragile and we should instead add them as a new special build.rs output that cargo would understand ?

@alexcrichton
Copy link
Member

It's pretty rare to pick up a staticlib from there, so if we're linking a dylib we just shouldn't print out those two -L paths probably

@julienw
Copy link

julienw commented Apr 29, 2016

In that case the patch would be as simple as not adding the env variable PKG_CONFIG_ALLOW_SYSTEM_LIBS when calling pkg-config? I can't do it now sorry (afk for some time).

@lilith
Copy link

lilith commented Sep 4, 2016

See sfackler/rust-openssl#447 (comment)

I think there's a way to make this specific to certain libraries (or, at least, I did for openssl):

cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu/libssl*
cargo:rustc-link-lib=ssl
cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu/libcrypto*
cargo:rustc-link-lib=crypto

My fork: https://github.com/imazen/rust-openssl

@alexcrichton
Copy link
Member

@nathanaeljones I don't think that syntax is recognized by the compiler. It will attempt to interpret /usr/lib/x86_64-linux-gnu/libssl* as a literal directory, but it ignores errors. That means that -lssl is passed to the linker and it "just works" because the file is located in a system directory and didn't end up needing a -L in the first place.

@lilith
Copy link

lilith commented Sep 15, 2016

What about #11 (comment) ? This is a pretty painful bug.

Since cargo [replace] is non-deterministically buggy (and after 3 hours, I still can't isolate any variables as to why), this is very challenging to work around. ideas?

@lilith
Copy link

lilith commented Sep 16, 2016

For reference, this is how I'm using [replace]. Cargo pretends it doesn't exist.

https://github.com/imazen/imageflow/blob/24b3519c62241135a16c8a2414485a3bb224f72c/imageflow_server/Cargo.toml#L8-L9

[replace]
"pkg-config:0.3.8" = { git = "https://github.com/imazen/pkg-config-rs", branch="master"}
"pkg_config:0.3.8" = { git = "https://github.com/imazen/pkg-config-rs", branch="master"}

@lilith
Copy link

lilith commented Sep 22, 2016

[replace] must only be in the root/"empty" crate of a workspace: rust-lang/cargo#3099

@alexcrichton
Copy link
Member

@nathanaeljones hm the pkg-config and pkg_config shouldn't be necessary, but were both required?

@lilith
Copy link

lilith commented Jan 3, 2017

No, they both were not. But due to the non-deterministic order of link paths in cargo, it is very hard to construct tests. Failures can range from 5% of builds to 95% of builds, so isolating variables is near-impossible.

(I'm not blaming pkg-config - there are larger questions)

Friction

This is by far the worst whack-a-mole situation I've experienced in the Rust ecosystem. Since September, I've averaged 16 hours per month in fighting recurrences on Travis and AppVeyor (along with various dev environments). I forked pkg-config, but any new crates I use could pull in a standard version, causing the issue to ... eventually, even weeks later ... reoccur. It's not easy to diagnose, even the (n)th time, because sometimes it is just OpenSSL breaking its ABI again, or an actual build environment issue, and it's just infrequent enough to be extremely painful.

I should be shipping a production beta of Imageflow this month, but for now I've had to disable TLS support across all crates and purge usage of HTTPS. For the server component, I'm planning an nginx proxy to substitute, but I don't yet have an answer for making HTTPS requests, and that's kind a key feature for an image proxy. Right now I'm holding my breath and hoping that this PR goes somewhere fast. If it doesn't, I'll need to fork Hyper and most of the Iron ecosystem rather quickly.

Unfortunately, this isn't limited to OpenSSL - I've run into it with libpng, libjpeg, and various other packages.

Larger question

My understanding is that there are several issues at play:

  1. Non-determinism in link path order. In isolation, non-determinism is a serious problem. If this is agreed, perhaps we should open a bug on cargo or rustc? I realize that caching and parallelism complicate this, and it's probably hard to solve. Nonetheless, .... non-determinism. This makes debugging improbable when linking to a large number of libs.

  2. Conflicting link paths from different crates. It would be nice if duplicate archives in user link paths caused a build failure (I think). If an archive exists in two user-specified locations, I can't easily reason about which one will be selected. I could see this behavior as modal - perhaps --allow-link-candidates=any, --allow-link-candidates=one, --allow-link-candidates=one-unique-hash? Or this could be per-crate, which would let us safe-list candidate sources more cleanly. The links metadata in cargo tries to prevent this, but can't help if a directory contains more than the specified library. But there's a more generic way to do this - see end.

  3. Reliance on system-wide libraries. I realize this might still be a topic of contention, but devs have put a lot of effort into gaining control over their dependencies and lowering the costs of this control. There's a servicing trade-off here, and likely some dependencies should be linked dynamically and serviced by the system update mechanism. Docker makes this less of a problem on Linux. However, *I think it might be worthwhile to consider supporting alternate artifact sources for -sys crates

Hear me out. You've got a great solution to the OpenSSL nightmare in the works, but the problem with C dependencies (in general) persists. Cross-plat is much harder when wrangling compatibility with a range of versions for each dependency. With diamond dependencies (like openssl) this is sometimes impossible without forking the world.

What if we made it easier for developers to use specific versions of C/C++ dependencies?

I realize this feature (discovered as I write this) tries to make that easier, and I suspect I could .gitignore .cargo/config and generate it from Conan.

However, if I miss a target, or a library, I risk silently linking against the wrong binaries and risking future heap or stack corruption.

What if - rather than preventing various build.rs scripts from executing - I was able to specify a 'filter' - an executable receiving the concatenated build script output via STDIN (along with all env vars like the target triple), and producing modified results to STDOUT?

This method would permit:

  • Linting and reporting of link paths
  • Testing of link-order determinism (for potential improvements).
  • Detection of duplicate link candidates
  • On-demand production of artifacts (and their link arguments) for only the required target triples.
  • Code reuse - just a build-dependency. We could then share providers for various artifact sources. I'd probably provide one for Conan.io and all major *-sys crates.

Anticipated challenges:

  • Standardizing conventions on how *-sys crates communicate acceptable ABI versions to filters.
  • Optimizing away redundant builds. For *-sys packages that actually build from source, we don't want to do this twice. Perhaps we also invoke the filter with a command-line argument for each crate - before running build.rs, and let the filter choose whether to run or skip build.rs? I'd probably write out specific use cases before adding complexity, though.

@julienw
Copy link

julienw commented Jan 4, 2017

@nathanaeljones if you already forked to remove https://github.com/alexcrichton/pkg-config-rs/blob/e9297b2e16e40026ec546324b657b780d2c305b3/src/lib.rs#L363 in your fork, maybe you can propose a PR ?

@alexcrichton
Copy link
Member

@nathanaeljones sorry you've been having so much trouble lately! It sounds like there's definitely some improvement we could make somewhere.

That being said though this perhaps isn't the place to discuss these issues? I'm more than willing to help out with any particular issue, but general changes to Cargo should probably be discussed on internals.rust-lang.org, rather than the pkg-config-rs issue tracker.

@lilith
Copy link

lilith commented Jan 5, 2017

@julienw #35 Would have done so earlier, but I didn't get confirmation that the change was acceptable.

@alexcrichton I wasn't sure, but figured you could suggest the best forum. Thanks! I'll post there.

@lilith
Copy link

lilith commented Jan 24, 2017

In light of the refactoring of SSL in Hyper, I upgraded all crates and gave this another try. It would appear that this is still broken by default - at least, compiling openssl-sys on Ubuntu 14.04 brings in the world (cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu), and overrides my other dependencies.

@lilith
Copy link

lilith commented Jan 24, 2017

What if we could somehow pass paths to the compiler only? Is there anything we can do to make this correct?

@Fuuzetsu
Copy link

Fuuzetsu commented Nov 4, 2021

We have lost bunch of developer time due to this surprising default 7 years since the ticket was opened...

At least we were able to locate the library leaking in the system directory by grepping for it in target and subsequently ensure it's not invoking pkg-config (at all in this case).

If there's nothing that's going to happen here, close as WONTFIX?

jonhoo pushed a commit to jonhoo/curl-rust that referenced this issue Apr 12, 2022
In its default configuration, pkg-config adds system-wide library
directories to the linker search path (rust-lang/pkg-config-rs#11).
This causes these directories to be searched before other paths added
by later crates or by `-Clink-arg` in rustflags. If a library is present in
the system-wide directory and a later build step wants to specifically
link against a different version of that library in another path, the linker
will choose the library from the first search directory it finds. If the linker
doesn't find a library in any of the specified search directories, it falls
back on system-wide paths, so we don't need to print the
path we found lubcurl in if it is in one of those system paths.

rust-lang/libz-sys#50
jonhoo pushed a commit to jonhoo/curl-rust that referenced this issue Apr 12, 2022
In its default configuration, pkg-config adds system-wide library
directories to the linker search path (rust-lang/pkg-config-rs#11).
This causes these directories to be searched before other paths added
by later crates or by `-Clink-arg` in rustflags. If a library is present in
the system-wide directory and a later build step wants to specifically
link against a different version of that library in another path, the linker
will choose the library from the first search directory it finds. If the linker
doesn't find a library in any of the specified search directories, it falls
back on system-wide paths, so we don't need to print the
path we found lubcurl in if it is in one of those system paths.

See rust-lang/libz-sys#50 for the same fix to libz that landed a while back.
jonhoo pushed a commit to jonhoo/git2-rs that referenced this issue Apr 13, 2022
In its default configuration, pkg-config adds system-wide library
directories to the linker search path (rust-lang/pkg-config-rs#11).
This causes these directories to be searched before other paths added
by later crates or by -Clink-arg in rustflags. If a library is present in
the system-wide directory and a later build step wants to specifically
link against a different version of that library in another path, the linker
will choose the library from the first search directory it finds. If the linker
doesn't find a library in any of the specified search directories, it falls
back on system-wide paths, so we don't need to print the
path we found libssh2 in if it is in one of those system paths.

See rust-lang/libz-sys#50 for the same fix to libz that landed a while back.
See also alexcrichton/curl-rust#441 for the same fix to libcurl-sys.
joshtriplett pushed a commit to rust-lang/git2-rs that referenced this issue Apr 27, 2022
In its default configuration, pkg-config adds system-wide library
directories to the linker search path (rust-lang/pkg-config-rs#11).
This causes these directories to be searched before other paths added
by later crates or by -Clink-arg in rustflags. If a library is present in
the system-wide directory and a later build step wants to specifically
link against a different version of that library in another path, the linker
will choose the library from the first search directory it finds. If the linker
doesn't find a library in any of the specified search directories, it falls
back on system-wide paths, so we don't need to print the
path we found libssh2 in if it is in one of those system paths.

See rust-lang/libz-sys#50 for the same fix to libz that landed a while back.
See also alexcrichton/curl-rust#441 for the same fix to libcurl-sys.
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

No branches or pull requests

5 participants