Adds world to the link path; bad neighbor

[lilith]

As openssl-sys adds the world to the link path, it prevents linking against any libs that have a different version in /usr/lib

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

It appears that link path order is randomized by cargo, so this will cause nicely randomized link failures if other native crates are in use (or silently cause the wrong/unpatched version of a lib to be linked instead).

This can be fixed by producing the following output instead:

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

[sfackler]

We pull this information directly from pkg-config. This seems like an upstream issue?

[lilith]

What would you expect pkg-config to provide? It's the correct folder.

[sfackler]

I guess I'm a bit confused about what's being proposed. This crate uses the pkg-config-rs crate which just calls down to pkg-config. Is there a C or Rust project that postprocesses the output of pkg-config in the way you're proposing that I can look at for reference?

[lilith]

What method on pkg-config is in use? I find build.rs a bit hard to reconcile with the output I'm seeing.

[lilith]

Snap! pkg_config::find_library writes to STDOUT. That was highly unexpected.

[lilith]

As far as I can tell, pkg-config is behaving as expected:

PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 pkg-config --cflags --libs openssl
 -L/usr/lib/x86_64-linux-gnu -lssl -lcrypto  

The output is correct; and we're telling it it's okay to include system lib folders. Have you seen pkg-config provide -L flags more specific than a folder? What does this command give you?

[sfackler]

I don't think I've seen it though I couldn't say that nothing is that specific.

[lilith]

This produces the desired output on my system. Instead of letting pkg_config control STDOUT, we replicate the code paths for a dynamic link. Is static linking of libssl a thing?

        if !target.contains("windows") {

            let mut conf = ::pkg_config::Config::new();
            // Disable writing to STDOUT from pkg_config directly; we need to 
            // take a less clobbering approach
            conf.cargo_metadata(false);
            //Probe for openssl
            if let Ok(info) = conf.probe("openssl") {
                //We don't need to consider frameworks -F

                let ref pkg_libs = info.libs;
                let ref pkg_link_paths = info.link_paths;
                // handle (-l) and (-L)
                for libname in pkg_libs {
                    // We asked pkg-config to include system lib folders, 
                    // So it's up to use to prevent clobbering
                    // rust link ordering is non-deterministic
                    for path in pkg_link_paths{
                        println!("cargo:rustc-link-search=native={}/lib{}*", path.to_str().unwrap(), libname);
                    }

                    //We aren't supporting static linking here
                    //but assuming this is OK
                    println!("cargo:rustc-link-lib={}", libname);

                }                   

                // handle (-I), avoid empty include paths as they are not supported by GCC
                if info.include_paths.len() > 0 {
                    let paths = env::join_paths(info.include_paths).unwrap();
                    println!("cargo:include={}", paths.to_str().unwrap());
                }
                return;
            }

Produces:

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

[sfackler]

Static linkage is a very common scenario, particularly for OpenSSL.

This kind of logic seems like it should be handled in pkg-config-rs itself since this seems like not an issue unique to this library?

[lilith]

Do you think it can be generalized? Can we always know the library names to generate more specific paths?

[sfackler]

I don't see any library names hardcoded in that snippet?

I am still confused as to why this is necessary. What other projects do this kind of thing? Is this a policy change that needs to be made over the entire Rust ecosystem?

[sfackler]

cc @alexcrichton

[alexcrichton]

To me this is both simultaneously correct and incorrect behavior. It's correct that pkg-config prints out a search path because the lib is actually located there. It's incorrect because it's already baked in the linker and wasn't necessary (and then allows picking up the wrong libs).

I'm not really sure what the best solution is here for everyone, but for OpenSSL specifically it's probably worth having very specific logic as it's just so common. I wouldn't mind adding a pkg-config to suppress /usr/lib linkage requests, @sfackler what do you think?

[sfackler]

@alexcrichton why is PKG_CONFIG_ALLOW_SYSTEM_LIBS set? According to the man page is has the effect of doing exactly this - adding /usr/lib to the -L paths.

[lilith]

I didn't realize that system libs were automatically linked, since I've had linkage fail even with a system alternative to a lib present. I suppose it makes sense for linkers to not support globs, but I wasn't sure of the mechanics there.

I've been using Conan pretty extensively to avoid spending time on linking issues in my C libs, and if you make system libs the fallback (vs priority) for linking, 'conan install' with a 2-line conanfile might be sufficient for providing an x-plat (incl. windows) OpenSSL build. This might not be desirable on well-updated linux distro, but could be a good option for Windows and older macs, perhaps? Upstream releases are quite quick to pass through conan.io.

Example conanfile.txt

[requires]
ConanCargoWrapper/0.1@lasote/testing
OpenSSL/1.0.2g@lasote/stable

[generators]
ConanCargoWrapper

I'm calling tangent on myself here, but there are specific benefits to de-prioritizing system libs over those explicitly provided through build.rs as Conan generates. I have not seen any order determinism in link paths provided via cargo:rustc-link-search=native, so consistent link behaviour occurs only when there is just one potential cargo-provided -L link path for any given library. If this issue is resolved, I might create an 'empty' crate that only triggers conan install and provides the appropriate cargo:rustc-link-search=native paths.

[alexcrichton]

@sfackler at this point I... don't really remember. It may have been for static linking so the compiler could pick up .a files in system locations? I'd be fine adding an option to not set that.

[sfackler]

We should probably move this discussion over to the pkg-config-rs repo then.

[lilith]

pkg-config-rs can't change the default behavior without a breaking change. See rust-lang/pkg-config-rs#11

I pulled another all-nighter on this problem, and I've hit a final wall. My last resort was ./.cargo/config, but I cannot figure out how to give it the configuration flags it wants that way. TOML can't handle duplicates, and it ignores arrays.

[target.x86_64-unknown-linux-gnu.openssl]
rustc-link-search=["/home/n/.conan/data/OpenSSL/1.0.2j/lasote/stable/package/8dc0071dc4241826d308f93b7589b40fa9c51286/lib"]
rustc-link-lib=["ssl", "crypto"]
rustc-cfg=["ossl102", "ossl10x", "v102"]

I'm using OpenSSL 1.0.2j.

https://github.com/imazen/imageflow/tree/server_ssl

Surely there is some known method for using openssl (on linux only) without clobbering the link path? It can't be this hard.

[sfackler]

Sounds like it's time for a breaking change to pkg-config-rs?

[alexcrichton]

Ok seriously this is not that hard to fix. I'm sorry I'd like to avoid making a breaking change to pkg-config-rs, but let's please work through this before just assigning blame. @nathanaeljones I'm sorry you're running into problems here, working with OpenSSL shouldn't be so difficult!

I've pushed 0.3.9 which has an option to disable the relevant env var. The openssl-sys crate does not work without it enabled because it can't find the headers. This'll need some more work.

[sfackler]

We could potentially do pkg-config lookup twice - the first time configured to emit system directories but not write anything to stdout for the include paths, and then run it a second time for the rust linkage stuff.

[alexcrichton]

Sounds plausible to me!

[lilith]

I didn't mean to imply any blame - it's clear that the system dirs are needed for header lookup, and that the pkg-config default is orthogonal to this issue (I've also eliminate all other crates using it).

1. Did I misunderstand that rustc requires the /usr/lib/x86_64-linux-gnu to be provided in order to find the static archive, but the linker does not - yet we only have a single argument that is always provided to both?

2. Do we know of anyone successfully using .cargo/config to override the rust-openssl build script successfully? If not, is said medium capable of providing the necessary rustc-cfg values to rust-openssl? I have not yet hit upon the incantation.

[sfackler]

From looking at the cargo source, those arrays should work fine? https://github.com/rust-lang/cargo/blob/de6bce9aaef528572cd2307fbf3b245c0dab8566/src/cargo/ops/cargo_compile.rs#L519

[lilith]

Does it work for you? (Even with /usr/lib, but using .cargo/config)?

[sfackler]

Seems to:

~/foo ❯ cat .cargo/config
[target.x86_64-apple-darwin.openssl]
rustc-link-search = ["/usr/local/opt/openssl/lib"]
rustc-link-lib = ["ssl", "crypto"]
rustc-cfg = ["ossl102"]
version = "102"
conf = ""
include = "/usr/local/opt/openssl/include"
~/foo ❯ cargo clean
~/foo ❯ cargo build --verbose
   Compiling lazy_static v0.2.2
   Compiling openssl-sys v0.9.6
   Compiling libc v0.2.20
   Compiling openssl v0.9.6
   Compiling bitflags v0.7.0
     Running `rustc /Users/sfackler/.cargo/registry/src/github.com-1ecc6299db9ec823/lazy_static-0.2.2/src/lib.rs --crate-name lazy_static --crate-type lib -g -C metadata=7f1b96a3a3eb529d -C extra-filename=-7f1b96a3a3eb529d --out-dir /Users/sfackler/foo/target/debug/deps --emit=dep-info,link -L dependency=/Users/sfackler/foo/target/debug/deps --cap-lints allow`
     Running `rustc /Users/sfackler/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.20/src/lib.rs --crate-name libc --crate-type lib -g --cfg feature=\"use_std\" --cfg feature=\"default\" -C metadata=29ef97a68464c2b7 -C extra-filename=-29ef97a68464c2b7 --out-dir /Users/sfackler/foo/target/debug/deps --emit=dep-info,link -L dependency=/Users/sfackler/foo/target/debug/deps --cap-lints allow`
     Running `rustc /Users/sfackler/.cargo/registry/src/github.com-1ecc6299db9ec823/openssl-0.9.6/build.rs --crate-name build_script_build --crate-type bin -g -C metadata=4be389b07254fbae --out-dir /Users/sfackler/foo/target/debug/build/openssl-4be389b07254fbae --emit=dep-info,link -L dependency=/Users/sfackler/foo/target/debug/deps --cap-lints allow`
     Running `rustc /Users/sfackler/.cargo/registry/src/github.com-1ecc6299db9ec823/bitflags-0.7.0/src/lib.rs --crate-name bitflags --crate-type lib -g -C metadata=0e272044714c8076 -C extra-filename=-0e272044714c8076 --out-dir /Users/sfackler/foo/target/debug/deps --emit=dep-info,link -L dependency=/Users/sfackler/foo/target/debug/deps --cap-lints allow`
     Running `/Users/sfackler/foo/target/debug/build/openssl-4be389b07254fbae/build-script-build`
     Running `rustc /Users/sfackler/.cargo/registry/src/github.com-1ecc6299db9ec823/openssl-sys-0.9.6/src/lib.rs --crate-name openssl_sys --crate-type lib -g -C metadata=6c510e83c5b0b664 -C extra-filename=-6c510e83c5b0b664 --out-dir /Users/sfackler/foo/target/debug/deps --emit=dep-info,link -L dependency=/Users/sfackler/foo/target/debug/deps --extern libc=/Users/sfackler/foo/target/debug/deps/liblibc-29ef97a68464c2b7.rlib --cap-lints allow -L /usr/local/opt/openssl/lib --cfg ossl102 -l ssl -l crypto`
     Running `rustc /Users/sfackler/.cargo/registry/src/github.com-1ecc6299db9ec823/openssl-0.9.6/src/lib.rs --crate-name openssl --crate-type lib -g -C metadata=c9e7b933bde3d3a6 -C extra-filename=-c9e7b933bde3d3a6 --out-dir /Users/sfackler/foo/target/debug/deps --emit=dep-info,link -L dependency=/Users/sfackler/foo/target/debug/deps --extern openssl_sys=/Users/sfackler/foo/target/debug/deps/libopenssl_sys-6c510e83c5b0b664.rlib --extern libc=/Users/sfackler/foo/target/debug/deps/liblibc-29ef97a68464c2b7.rlib --extern bitflags=/Users/sfackler/foo/target/debug/deps/libbitflags-0e272044714c8076.rlib --extern lazy_static=/Users/sfackler/foo/target/debug/deps/liblazy_static-7f1b96a3a3eb529d.rlib --cap-lints allow --cfg ossl102 --cfg ossl10x --cfg osslconf=\"\" -L /usr/local/opt/openssl/lib`
   Compiling foo v0.1.0 (file:///Users/sfackler/foo)
     Running `rustc src/main.rs --crate-name foo --crate-type bin -g -C metadata=387e546625dc9263 --out-dir /Users/sfackler/foo/target/debug --emit=dep-info,link -L dependency=/Users/sfackler/foo/target/debug/deps --extern openssl=/Users/sfackler/foo/target/debug/deps/libopenssl-c9e7b933bde3d3a6.rlib -L /usr/local/opt/openssl/lib`
    Finished debug [unoptimized + debuginfo] target(s) in 4.42 secs

[sfackler]

I believe this should do the right thing: #565.

[lilith]

Thanks! Those vars seem to be letting compilation complete!

I am seeing that it prevents caching: INFO:cargo::ops::cargo_rustc::fingerprint: fingerprint error for openssl-sys v0.9.6: precalculated components have changed: overridden build state with hash: 6489623238946944324 != overridden build state with hash: 16599502416394507546

The last hash changes every build, although I can verify .cargo/config is not being touched. Is this happening for you?

[sfackler]

Yeah, I am seeing openssl-sys rebuilding if I touch src/main.rs. Seems like a bug to file on Cargo I think. Build script overrides have not been very heavily used I don't think so it's not super surprising.

[lilith]

I took a couple weeks off Rust to try out several different versions of the flu.

I'm back, and have filed rust-lang/cargo#3658

For me, no file changes (nor modified date changes) are required to force a rebuild.

[lilith]

It would not appear there there is a workaround this. Cargo randomizes order before hashing.

[lilith]

Dropping "conf" increased the success rate from 25% to 50% (leaving just the randomized order of rustc-link-lib).

[lilith]

Cargo PR: rust-lang/cargo#3659