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

libcurl: expose CMAKE_OPENSSL_AUTO_LOAD_CONFIG #6435

Closed
wants to merge 1 commit into from
Closed

libcurl: expose CMAKE_OPENSSL_AUTO_LOAD_CONFIG #6435

wants to merge 1 commit into from

Conversation

@rzvncj
Copy link
Contributor

@rzvncj rzvncj commented Jan 11, 2021

This does for cmake builds what --enable-openssl-auto-load-config
does for autoconf builds.

@bagder bagder added the cmake label Jan 11, 2021
@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 11, 2021

Do I need to do something about the one failing check? Is it a false positive?

@bagder
Copy link
Member

@bagder bagder commented Jan 11, 2021

No, that's a flaky test causing a false positive. Your PR looks fine!

@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 11, 2021

Thanks!

@bagder bagder requested a review from jzakrzewski Jan 11, 2021
@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Jan 11, 2021

We have this unfortunate set of options beginning with "CMAKE_". GUI tools use those prefixes to group options together, so it's kinda namespacing which we're hijacking :/. I'd prefer we wouldn't add more of those. Could you come up with a name that does not begin with "CMAKE_"?
Apart from that it looks clean to me.

@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 11, 2021

We have this unfortunate set of options beginning with "CMAKE_". GUI tools use those prefixes to group options together, so it's kinda namespacing which we're hijacking :/. I'd prefer we wouldn't add more of those. Could you come up with a name that does not begin with "CMAKE_"?

Sure, I can change it to whatever you find more suitable. I've just been following what appeared to be the current convention. I guess the simplest change would be to simply remove the CMAKE_ prefix - so making this OPENSSL_AUTO_LOAD_CONFIG. If that works for you I'll change it and update the pull request.

@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 11, 2021

I've updated the pull request. Please take a look when you've got time and let me know if there's anything else I can do. Thanks!

@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 13, 2021

Ping? :)

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Jan 13, 2021

Sorry - I'm quite busy those days.
I definately like it better but even then I'm concerned about using other project's name (OPENSSL_) at the beginning of the option name. The bad thing is we seem to use CURL_* variables internally only where it would make much sense to prefix option names with it. I don't really have a good idea at the moment. CURLOPT_* maybe?
I'd love to hear others @jay seems to have a very critical (in a good way) approach to things like that.

In case you were wondering what I mean by grouping, this is what CMake GUI does:
image

@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 13, 2021

Thanks! Yes, I did understand your grouping concern. I'm perfectly happy to name the cmake option to whatever is agreable to the project maintainers.

Looking at the current CMakeLists.txt file I see several patterns emerging:

  1. No prefix at all (e.g. ENABLE_DEBUG).
  2. A CURL_ prefix (e.g. CURL_DISABLE_FTP).
  3. A CMAKE_ prefix (e.g. - and this is why I've prefixed the new option with CMAKE_, since it's CMAKE_USE_OPENSSL-dependent - CMAKE_USE_OPENSSL).

To me, the CMAKE_ prefix looks like the best fit with the "parent" option CMAKE_USE_OPENSSL (grouping notwithstanding), but it really doesn't matter - the priority is just to have this pull request merged.

Unfortunately I don't know what else to suggest naming-wise. I'll wait for @jay's suggestion, as requested.

@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 13, 2021

Also, if we don't keep the CMAKE_ prefix, and with the current grouping, and assuming we don't want to break the cmake-based builds of existing curl clients (by renaming CMAKE_USE_OPENSSL as well), we'll have CMAKE_USE_OPENSSL in one group, and whatever else we call the (?)_OPENSSL_AUTO_LOAD_CONFIG option in another group. They feel like they should be in the same group to me.

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Jan 14, 2021

Yes, those are exactly the things I've been thinking about and haven't come up with a good answer.

As this is not only strictly CMake question, maybe also @bagder could throw in something. Some general thoughts? What about cleaning this mess up? Would that be considered breaking backwards compatibility?

@jay
Copy link
Member

@jay jay commented Jan 15, 2021

I definately like it better but even then I'm concerned about using other project's name (OPENSSL_) at the beginning of the option name. The bad thing is we seem to use CURL_* variables internally only where it would make much sense to prefix option names with it. I don't really have a good idea at the moment. CURLOPT_* maybe?
I'd love to hear others @jay seems to have a very critical (in a good way) approach to things like that.

I'm sorry to say I don't have a strong opinion about cmake namespacing. If we change the existing option names now we're going to break builds. As I mentioned in a recent PR I thought USE_xxx was for libraries and CURL_xxx was for protocols. If you run cmake -LAH you can see there's a mix in the way configuration options are prefixed, and IMO no clear winner. Here are a few to make the point:

// Use libSSH
CMAKE_USE_LIBSSH:BOOL=OFF

...

// disables HTTP
CURL_DISABLE_HTTP:BOOL=OFF

...

// Set to ON to enable c-ares support
ENABLE_ARES:BOOL=OFF

...

// Use libidn2 for IDN support
USE_LIBIDN2:BOOL=ON

What about CURL_DISABLE_OPENSSL_AUTO_LOAD_CONFIG:BOOL=OFF (which is also the name of the symbol) ?

This does for cmake builds what --disable-openssl-auto-load-config
does for autoconf builds.
@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 15, 2021

Changed to CURL_DISABLE_OPENSSL_AUTO_LOAD_CONFIG. Please let me know if there's anything else you'd like addressed. Thanks!

Copy link
Contributor

@jzakrzewski jzakrzewski left a comment

Thanks, @jay this is the voice I wanted to hear.
@rzvncj thanks for your patience.

@rzvncj
Copy link
Contributor Author

@rzvncj rzvncj commented Jan 15, 2021

No problem, all's well that ends well. 👍

@jay jay closed this in 13fe0b6 Jan 16, 2021
@jay
Copy link
Member

@jay jay commented Jan 16, 2021

Thanks

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

Successfully merging this pull request may close these issues.

None yet

4 participants