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.

Sign up for GitHub

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

Fix CURL ssl option for CPR recipe #3207

Closed
wants to merge 3 commits into from
Closed

Fix CURL ssl option for CPR recipe #3207

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9018abd
Fix CURL ssl option for CPR recipe
uilianries Oct 15, 2020
f20cf6a
Do not force CURL SSL option
uilianries Oct 15, 2020
9aef944
Revert "Do not force CURL SSL option"
uilianries Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion recipes/cpr/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def configure(self):
del self.options.fPIC
if self.options.with_openssl:
# If using OpenSSL, we need it to be active in libcurl too
self.options["libcurl"].with_openssl = True
self.options["libcurl"].with_ssl = "openssl"
Copy link
Contributor

@madebr madebr Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should be moved to build and changed in a check:

if self.options["libcurl"].with_openssl != "openssl":
    raise ConanInvalidConfiguration("cpr requires libcurl built with the option with_ssl=\"openssl\"")

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes more sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, because build() is not always called, and you probably want that recipe works out of the box.

Copy link
Contributor

@madebr madebr Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Then the check should (also) be called in package_info.

The options of libcurl are available and it will always be run on consumption.

Copy link
Contributor

@SpaceIm SpaceIm Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes maybe, but anyway I believe that upstream option should be forced if it's a strong requirement of this recipe. What happen if default option of libcurl is not with_ssl = openssl? No binary is produced in CCI? Downstream recipes shouldn't rely on upstream default options.

Copy link
Contributor

@madebr madebr Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Forcing of options is not correct as it will override a user passing e.g. -o libcurl:with_ssl="gnutls". This is surprising behavior for a user.
It's better to tell the user that he is wrong about an option then to override it.
Let's not treat the user as an idiot.

This recipe is not relying on libcurl default options as it actively checks the option.

Copy link
Contributor

@SpaceIm SpaceIm Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's even more surprising for a user to have a recipe not building with its default options. This recipe relies on libcurl default option (which is not openssl on Macos by the way) to build out of the box.

If you just launch conan install somerecipe/x.x.x and it raises because of some options in transitive dependencies, it's quite surprising.

Depending on recipe design, an option could be a feature, or a component which could has been its own recipe.

In conan 2.0, precedence of values should change conan-io/conan#7786

Copy link
Contributor

Choose a reason for hiding this comment

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

@SpaceIm
Agreed it's not fun for a user when it doesn't work out-of-the box (which it does at the moment), but forcing will ultimately cause problems when 2 recipes require the opposite. As long as the error message is actionable by the user, (s)he isn't puzzled by weird compiler errors.

Thanks for the link to the conan issue.
The proposed validate function is nice to write the checks for options of dependencies in only one place instead, but will not solve the matter of (forcing of) default options, I think.

Copy link
Contributor

@SpaceIm SpaceIm Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

On the same topic:

  • in old and new libcurl recipe (without and with with_ssl option): the default value for ssl lib on Macos is darwinssl
  • in old libcurl recipe, forcing with_openssl=True didn't have any effect when darwin_ssl was already True (which was the case for default options, it was with_openssl=True and darwin_ssl=True), and on Macos darwin_ssl always had precedence in old design. So CPR was somehow broken on Macos, if openssl is strictly required.
  • in new libcurl recipe, forcing with_ssl =openssl will indeed force using openssl in libcurl. But it's not the default value on Macos, so there is no pre build binaries in CCI for such package ids.
  • cpr recipe claims that openssl support is unstable, so why not disabling it by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, let's keep forcing option.


def source(self):
tools.get(**self.conan_data["sources"][self.version])
Expand Down