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

Use of openssl-sys is problematic #30

Closed
codyps opened this issue Nov 25, 2014 · 7 comments
Closed

Use of openssl-sys is problematic #30

codyps opened this issue Nov 25, 2014 · 7 comments

Comments

@codyps
Copy link

codyps commented Nov 25, 2014

  1. curl-sys/build.rs (in some cases) uses pkg-config, which will already pull in the correct ssl library for that particular compilation of curl.
  2. curl-sys includes openssl-sys as an extern crate and proceeds to never use it
  3. For non-blessed targets, using target-specific dependencies to pull in openssl-sys breaks the build of curl-rust as it actually uses the openssl crate.

Some of these are probably due to a mismatch in cargo between the way linking actually works and how it currently pretends it works (ie: trys to avoid adding duplicate native libraries in the linker command line where this is actually the norm).

@alexcrichton
Copy link
Owner

I don't really follow this, could you be a little more specific as to what the problem is?

@codyps
Copy link
Author

codyps commented Nov 27, 2014

It's a few problems:

  1. non-blessed targets (those without a particular triple in curl-sys's Cargo.toml) are broken. Adding all targets is not possible (upstream) because some of them are generated by build scripts.
  2. curl-rust built with the feature unix depends on openssl-sys (it calls a function from it), but doesn't actually depend on it (via Cargo.toml), leaving that to curl-sys (which proceeds to depend on openssl based on triplets rather than features, a mismatch between the usage and dependency).

@codyps
Copy link
Author

codyps commented Nov 27, 2014

Also, on the pkg-config bit:

The normal operation with feature(unix) and target={i686,x86_64}-unknown-linux-gnu in curl-sys causes the openssl libraries to appear three times in the link command (pkg-config --static curl adds them twice, then depending on openssl-sys adds them again)

I'd argue that it may not be entirely correct to have curl-sys depend on openssl-sys: we're essentially writing out duplicate dependency information that already exists elsewhere, and having that dependency information be wrong some of the time.

My understanding is: the curl-sys -> openssl-sys dep exists so that in the case of a system lacking curl and openssl, we can have openssl automatically built. Unfortunately, that is not all that dependency does, and curl-rust presently reliese on it for more than that (improperly, as I argue above).

It's less clear what the right way to fix this is.

@alexcrichton
Copy link
Owner

The lack of non-blessed targets sounds like a limitation of Cargo, not the usage here. This was an explicit drawback in the RFC that drafted this support.

Note that we really do need to depend on OpenSSL on unix because we're depending on that as part of the libcurl build process. If libcurl is found on the system, then it wasn't really up to use (so the openssl-sys dep doesn't do much), but if we build libcurl from source then we require OpenSSL to be available to enable https support.

I do agree that the lack of other platform support is somewhat worrisome, but I don't see how it's specific to this project or how it relates to pkg-config though. What do you mean that the openssl libraries appear three times? This sounds like a bug?

@codyps
Copy link
Author

codyps commented Dec 3, 2014

Thanks for the link to the RFC. I don't see it mentioning that target-based dependencies will make non-blessed targets unusable without patching within the cargo ecosystem, but yes, that is something we'll want to address in cargo. In this case, it could be solved by somehow having dependencies triggered by cfg(unix).

On the curl-sys -> openssl-sys dependency: the problem is that that real-world-dependency doesn't always exist. It only shows up when we need to build the c library that backs curl-sys. In the alternate where we have pkg-config & libcurl already, pkg-config indicates the c library dependencies that exist.
And in both cases, we don't need the ffi interface curl-sys provides. [And I'd argue that curl-rust, which needs that ffi interface, should be depending on it]

While I've thrown in some other complaints, I think for now this issue can be summarized as:

Missmatch between usage and dependence on openssl-sys breaks non-blessed triples

and it'll probably be blocking on a cargo issue for that support.

@alexcrichton
Copy link
Owner

Oh wow, sorry about that! I misremembered and then didn't even read my own RFC! Would you be ok moving this issue to rust-lang/cargo instead of curl-rust?

@codyps
Copy link
Author

codyps commented Dec 3, 2014

Closing in favor of more specific ticket(s)

@codyps codyps closed this as completed Dec 3, 2014
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

2 participants