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

Link curl to the OpenSSL targets instead of lib absolute paths #2753

Closed
wants to merge 4 commits into from

Conversation

@johnb003
Copy link
Contributor

@johnb003 johnb003 commented Jul 17, 2018

Linking to targets is preferred to linking directly to libraries, as the whole set of transitive dependencies can be included. Additionally, when exporting the target, we can expose the dependency as a target, and have the downstream dependent project also look for OpenSSL in order to link the target as well, making the dependencies relocatable.

Fixes issue: #2746

list(APPEND CURL_LIBS ${OPENSSL_LIBRARIES})
include_directories(${OPENSSL_INCLUDE_DIR})
list(APPEND CURL_LIBS OpenSSL::SSL OpenSSL::Crypto)

Copy link
Contributor Author

@johnb003 johnb003 Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, the include_directories is also implied when linking the target, so this isn't needed any more.

Loading

Copy link
Contributor Author

@johnb003 johnb003 Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not certain if these targets are safe to use without checking if the component is found. Do your tests cover if ssl or crypto is missing or does anyone know? Otherwise we should research it before accepting this.

Loading

Copy link
Contributor

@jzakrzewski jzakrzewski Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fail to build and CMake should complain about the imported targets missing if OpenSSL is not found.
But the problem is that we require CMake 2.8.12 and the OpenSSL module gained imported targets in 3.4, so we cannot use them unless we're willing to copy newer version of the FindOpenSSL.cmake to the curl repository.

Loading

Copy link
Contributor Author

@johnb003 johnb003 Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then I think the concern I raised is resolved. There's just the matter of cmake version now.

Well, this is your call. Personally I don't think you're gaining that much compatibility sticking with very old versions of cmake. Even if someone is on an old linux distribution, it's trivial to update the cmake version. But it sounds like you can just copy the find module, from a modern version, and not even have to update the version, so that sounds like a good compromise to me.

Loading

Copy link
Member

@snikulov snikulov Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnb003 Could you please update PR with cmake_minimum_required(VERSION 3.4... as well?

Loading

Copy link
Contributor Author

@johnb003 johnb003 Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradking This sounds like an interesting problem for CMake in general.

My take is, if you're expecting to be this regimented about versions, you should be describing the known compatible version range of the library you're working with in the first place (as @jzakrzewski suggests), but practically this doesn't happen that often, and besides most of the established libraries won't be making big breaking API changes.

So the situations are:
You build curl expecting X and end up linking in the app with Y.

By the way, with DLLs this is kind of intended behavior. The question is the compatibility between X and Y, and is there any really standard way to determine this?

if Y < X, that can be be bad, but it can also work if it's the same major version and there's no breaking API changes.

if Y > X, this is probably quite common, as you can update the system dlls and not rebuild everything that depends on them, again it has to be within API compatibility though.

I guess if the ConfigXXX.cmake is written with a variable, you can inject the value you want during the install with the known minimum major version of the library. I mean, that's what configure_file is for right?

Loading

Copy link
Contributor

@jzakrzewski jzakrzewski Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK find_package (and, by extension, find_dependency) normally does the right thing with versions, so if you write find_package(OpenSSL VERSION 1.0.0) it should find also 1.1.0 for example but not 0.9.8.

The find_dependency must however be told about the expected version. Doing it with the magic of configure_file seems right for me but I don't have enough experience to tell if this is the way to go - I'm maintaining CMake build system in a strictly-controlled environment, so such things do not happen ;)

Loading

Copy link
Member

@snikulov snikulov Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jay almost all LTEs provides packages for cmake3. And travis-ci checks are passed.
Of course, we could ask for votes in the mailing list before merging.

@jzakrzewski Not sure if the explicit version should be provided unless automake already enforces specific version. AFAIR, we should mimic autotools build...

Loading

Copy link
Contributor

@jzakrzewski jzakrzewski Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snikulov Well, I think that if curl pulls OpenSSL 0.9.8 and user program pulls OpenSSL 1.0.0 it'll break badly. If you're lucky it will be linker errors. If you're not - random runtime errors and crashes. I had two versions of an LDAP library linked once. It was no fun debugging it. I don't think mimicking autotools in every aspect is right if we can do better.

Loading

Copy link
Contributor Author

@johnb003 johnb003 Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I matched only the major version, because OpenSSL can use non-standard patch description versioning semantics, like 1.0.2o which will create an error if used as the search version for find_dependency.

find_package called with invalid argument "1.0.2o"

Loading

@bagder bagder added the cmake label Jul 17, 2018
@johnb003
Copy link
Contributor Author

@johnb003 johnb003 commented Jul 23, 2018

Ping?

Loading

include(CMakeFindDependencyMacro)
if(CURL_FIND_REQUIRED_libcurl)
find_dependency(OpenSSL "@OPENSSL_VERSION_MAJOR@")
endif()
Copy link
Contributor

@bradking bradking Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once content substitution is done then the input file should be renamed with a .in extension so that nothing ever tries to read it as a valid .cmake file.

Loading

Copy link
Contributor Author

@johnb003 johnb003 Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. + rebased since curl-config.cmake also changed upstream from original PR, which resulted in a force push to my branch.

Loading

Copy link
Contributor

@bradking bradking Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

The CURL_FIND_REQUIRED_libcurl value is set only if find_package is called with REQUIRED. Even if it is not a required package we still need to look for curl's dependencies.

Loading

@johnb003
Copy link
Contributor Author

@johnb003 johnb003 commented Jul 24, 2018

@snikulov @jzakrzewski are we good now? I'd like to finish soon or hand it off.

Loading

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Jul 24, 2018

No time to test it but looks good otherwise.

Loading

@snikulov
Copy link
Member

@snikulov snikulov commented Jul 24, 2018

LGTM.

Loading

@bagder
Copy link
Member

@bagder bagder commented Aug 8, 2018

Thanks, all of you!

Loading

@bagder bagder closed this in 7867aaa Aug 8, 2018
bagder added a commit that referenced this issue Aug 8, 2018
xquery added a commit to xquery/curl that referenced this issue Aug 9, 2018
Reviewed-by: Jakub Zakrzewski
Reviewed-by: Sergei Nikulov
Closes curl#2753
xquery added a commit to xquery/curl that referenced this issue Aug 9, 2018
falconindy added a commit to falconindy/curl that referenced this issue Sep 10, 2018
Reviewed-by: Jakub Zakrzewski
Reviewed-by: Sergei Nikulov
Closes curl#2753
falconindy added a commit to falconindy/curl that referenced this issue Sep 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants