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

Add support for cpr 1.6.2 #5353

Merged
merged 11 commits into from
May 12, 2021
Merged

Add support for cpr 1.6.2 #5353

merged 11 commits into from
May 12, 2021

Conversation

ollien
Copy link
Contributor

@ollien ollien commented Apr 28, 2021

Specify library name and version: cpr/1.6.2

I had just worked on cpr/1.6.0, and now that cpr/1.6.2 is released, I figured I'd give it an update :) Flags are added for darwinssl, and for disabling ssl entirely (we previously disabled SSL if none was selected, but doing so precludes CPR from attempting to auto-detect).


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@property
def _supports_darwinssl(self):
# https://github.com/whoshuu/cpr/releases/tag/1.6.1
return tools.Version(self.version) >= "1.6.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return tools.Version(self.version) >= "1.6.1"
return tools.Version(self.version) >= "1.6.1" and tools.is_apple_os(self.settings.os)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read this change and I could have SWORN I already did it. Guess not, lol. On it.

Comment on lines 19 to 22
"with_openssl": [True, False],
"with_winssl": [True, False],
"with_darwinssl": [True, False],
"no_ssl": [True, False],
Copy link
Contributor

Choose a reason for hiding this comment

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

I was afraid of this...

Can we copy the curl recipe and use an with_ssl and provide string options? and deprecate the older two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, that makes total sense, and frankly I don't know why I didn't think about it. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a proper way to deprecate options, by the way? I took a quick peek at the docs and didn't see it

Copy link
Contributor

Choose a reason for hiding this comment

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

#5020 We just wrote it recently 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh awesome! Will flip to using that way

@conan-center-bot

This comment has been minimized.

@ollien
Copy link
Contributor Author

ollien commented Apr 28, 2021

I know why CI is failing, small error on my part with one of the old SSL options. Will be fixed along with everything else...

@conan-center-bot

This comment has been minimized.

@@ -18,16 +22,14 @@ class CprConan(ConanFile):
"fPIC": [True, False],
"with_openssl": [True, False],
"with_winssl": [True, False],
"with_darwinssl": [True, False],
"no_ssl": [True, False],
"ssl": ["openssl", "darwinssl", "winssl", AUTO_SSL, NO_SSL]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please stick with our usual, with_ syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

SSL_CURL_LIBS = {
"openssl": "openssl",
"darwinssl": "darwinssl",
"winssl": "schannel"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier if we used the name value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not sure what you're referring to here. Could you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bit of complexity from the CPR values being different from the curl recipe...

But looking at it again, it's just combinatorics hell, since the options and support have changed a lot it's very complicated

from conans import ConanFile, CMake, tools
from conans.errors import ConanInvalidConfiguration
import os


class CprConan(ConanFile):
AUTO_SSL = "auto"
Copy link
Contributor

Choose a reason for hiding this comment

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

Custom attributes should always be prefixed with _

@conan-center-bot

This comment has been minimized.

Comment on lines 197 to 198
self.output.info("Auto SSL is not available below version 1.6.0. Falling back to openssl")
return "openssl"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should be moved to config_options

such that the default is sensibly configured. We also try to no shock the user by override their explicit input and a minimum this should be a raise ConanInvalidConfiguration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes total sense. Will fix this up later today.

ssl_library = self._get_ssl_library()
if not self._supports_ssl_library(ssl_library):
raise ConanInvalidConfiguration(
f"Invalid SSL selection for the given configuration: {SSL_FAILURE_MESSAGES[ssl_library]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

We are trying typing to maintain support for older python versions, could you please use "".format() instead please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah shoot - wasn't thinking about that. I've been in the f-string mindset for too long 😅

@conan-center-bot

This comment has been minimized.

prince-chrismc
prince-chrismc previously approved these changes Apr 29, 2021
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

LGTM!

@ollien
Copy link
Contributor Author

ollien commented May 6, 2021

Just wanted to bump this. Anything you want to see changed? Or are we good to push this through?

@conan-center-bot conan-center-bot requested a review from SSE4 May 6, 2021 12:00
@prince-chrismc
Copy link
Contributor

Requested review: New feature is starting to kick in?

You can take a peak at my bot, 21 is fairly high so theres a bit of a back lock and lots of new PRs (which tend to get seen first)
prince-chrismc/conan-center-index-pending-review#1

@SSE4
Copy link
Contributor

SSE4 commented May 6, 2021

Requested review: New feature is starting to kick in?

You can take a peak at my bot, 21 is fairly high so theres a bit of a back lock and lots of new PRs (which tend to get seen first)
prince-chrismc/conan-center-index-pending-review#1

right now, the selection is completely random. we'll see how is it going, and if further tuning is needed.

@@ -1,9 +1,13 @@
from typing import no_type_check
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from typing import no_type_check

Because this will break python2 and there is no typing in conanfiles at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bleh, I think my editor added this in and I didn't catch it. Thanks.

Comment on lines 114 to 118
ssl_library = self._get_ssl_library()
# Make sure libcurl uses the same SSL implementation
if self.options.get_safe("with_openssl", False):
# self.options["libcurl"].with_openssl = True # deprecated in https://github.com/conan-io/conan-center-index/pull/2880
self.options["libcurl"].with_ssl = "openssl"
if self.options.get_safe("with_winssl", False):
# self.options["libcurl"].with_winssl = True # deprecated in https://github.com/conan-io/conan-center-index/pull/2880
self.options["libcurl"].with_ssl = "schannel"
# "auto" will be handled by cpr
if self._supports_ssl_library(ssl_library) and ssl_library in SSL_CURL_LIBS:
self.options["libcurl"].with_ssl = SSL_CURL_LIBS[ssl_library]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would instead raise in validate, if libcurl uses different SSL implementation because overriding user's desires is not something we should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey doke. I did this primarily to mirror what was already there but I'll give this a fiddle and see how it works out.

Comment on lines 190 to 193
if library not in validators:
# This should never happen, as the options are validated by conan.
raise ValueError("Unknown SSL library: {}".format(library))

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if library not in validators:
# This should never happen, as the options are validated by conan.
raise ValueError("Unknown SSL library: {}".format(library))

Let's just remove, because accessing validators[library] will raise more-or-less the same error and as you noted it's not gonna happen at all.

Comment on lines 23 to 34
+ find_package(CURL)
+ if(CURL_FOUND)
+ message(STATUS "Curl found on this system.")
else()
- find_package(CURL COMPONENTS HTTP)
- if(CURL_FOUND)
- message(STATUS "Curl found on this system.")
- else()
- message(FATAL_ERROR "Curl not found on this system. To use the build in version set CPR_FORCE_USE_SYSTEM_CURL to OFF.")
- endif()
+ message(FATAL_ERROR "Curl not found on this system. To use the build in version set CPR_FORCE_USE_SYSTEM_CURL to OFF.")
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use find_package(REQUIRED) here?

@conan-center-bot
Copy link
Collaborator

All green in build 5 (b99b4f68280550aa044eb792727c0c6c054b3f05):

  • cpr/1.3.0@:
    All packages built successfully! (All logs)

  • cpr/1.5.0@:
    All packages built successfully! (All logs)

  • cpr/1.6.0@:
    All packages built successfully! (All logs)

  • cpr/1.6.2@:
    All packages built successfully! (All logs)

  • cpr/1.5.2@:
    All packages built successfully! (All logs)

  • cpr/1.4.0@:
    All packages built successfully! (All logs)

@ghost
Copy link

ghost commented May 9, 2021

I detected other pull requests that are modifying cpr/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@ghost ghost mentioned this pull request May 9, 2021
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Big improvement, including deprecation process.

Copy link
Contributor

@SSE4 SSE4 left a comment

Choose a reason for hiding this comment

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

wow, so many changes for new version

@conan-center-bot conan-center-bot merged commit 9447db0 into conan-io:master May 12, 2021
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

Successfully merging this pull request may close these issues.

7 participants