[poco] re-enabling support for NetSSL with OpenSSL on Windows #6314

jngrb · 2021-07-13T15:29:03Z

Specify library name and version: poco/1.8.1 to 1.11.0

This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!

Fixing #6311 : I want to be able to consume poco (any version, currently available are Poco v1.8.1 to v1.11.0) with enable_netssl=True and enable_netssl_win=False to have SSL support using the OpenSSL on Windows (instead of Windows' TLS).

Before #4205 and 6ea152e were implemented, I could do this using:

enable_netssl=True
enable_netssl_win=False
force_openssl=True

on Windows. As force_openssl is redundant information, this PR supports this use case with

enable_netssl=True
enable_netssl_win=False

only. Of course, setting both to False gives no SSL support (no NetSSL component) and WinTLS can be consumed by reversing these two flags. And setting both to True gives an ConanInvalidConfiguration exception.

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.

Changed Poco recipe (any version) to support the NetSSL component with OpenSSL on Windows again. Consumers should be able to either use NetSSL or NetSSL_win components with OpenSSL- or Windows TLS-enabled SSL support - or none - but not both. No changes for any non-Windows OS.

CLAassistant · 2021-07-13T15:29:07Z

All committers have signed the CLA.

ghost · 2021-07-13T15:39:02Z

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

fix poco 1.11.0 cmake bundle #6296

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

Talkless · 2021-07-16T07:59:16Z

I've hit on this issue when migrated packages to use new conan center repository. Would be pretty nice to have this fixed ASAP.

Talkless · 2021-07-16T08:34:36Z

Tested with my applications, builds and runs fine. @uilianries Please consider reviewing this.

mathbunnyru

With this change, unfortunately, you will completely disable Windows CCI builds.

Why will this happen?
Both enable_netssl and enable_netssl_win are set True by default (and CCI always uses default config).
You don't do anything to these options on Windows and you will raise ConanInvalidConfiguration, which means it will pass on CI, but you will not be actually building this package for Windows.

mathbunnyru · 2021-07-16T11:07:29Z

I do agree with you, that I made a mistake, but we have to come up with a solution that is checked in the CCI, if it's possible.

* the default for the Windows-only option enable_netssl_win is set to false * as the enable_netssl option's default is true (for any Platform), any consumer not specifying options will get the OpenSSL-supported NetSSL component * for explicitly use the WinTLS-supported NetSSL_win, the options have to be switched by the consumer * note that the Conan-Center-Index CI/CD will build and test the OpenSSL version on Windows

jngrb · 2021-07-16T12:24:35Z

@mathbunnyru , I agree. I was unaware about the skipped CI tests until you told me.

I changed my PR by setting the default for the Windows-only option enable_netssl_win to false.
As the enable_netssl option's default is true (for any Platform), any consumer not specifying options will get the OpenSSL-supported NetSSL component.
For explicitly usage of the Windows Schannel SSP-supported NetSSL_win, the options have to be switched by the consumer.
Note that the Conan-Center-Index CI/CD will build and test the OpenSSL version on Windows.

Is that OK? Or should the CI/CD test the Schannel component on Windows?

(Note: by mistake, I wrote WinTLS in my earlier comments but I meant Windows Schannel SSP).

mathbunnyru

I think it's ok to test openssl even on Windows.

prince-chrismc · 2021-07-16T19:26:58Z

Note that the Conan-Center-Index CI/CD will build and test the OpenSSL version on Windows.

Is that OK? Or should the CI/CD test the Schannel component on Windows?

Totally acceptable, we have other recipes with many SSL options and most default to OpenSSL

SSE4 · 2021-07-19T06:57:55Z

recipes/poco/all/conanfile.py

@@ -48,7 +48,7 @@ class PocoConan(ConanFile):
        "PocoMongoDB": _PocoComponent("enable_mongodb", True, ("PocoNet", ), True),
        "PocoNet": _PocoComponent("enable_net", True, ("PocoFoundation", ), True),
        "PocoNetSSL": _PocoComponent("enable_netssl", True, ("PocoCrypto", "PocoUtil", "PocoNet", ), True),    # also external openssl
-        "PocoNetSSLWin": _PocoComponent("enable_netssl_win", True, ("PocoNet", "PocoUtil", ), True),
+        "PocoNetSSLWin": _PocoComponent("enable_netssl_win", False, ("PocoNet", "PocoUtil", ), True),


for reviewers: it's not obvious from the diff itself what does True/False mean here
it's just a default option value according to the:

conan-center-index/recipes/poco/all/conanfile.py

Line 32 in 1ba9584

_PocoComponent = namedtuple("_PocoComponent", ("option", "default_option", "dependencies", "is_lib"))

conan-center-bot · 2021-07-19T11:27:33Z

All green in build 5 (1ba9584da61a7119ac3f26d91ec0a6e7a9380dba):

poco/1.10.1@:
All packages built successfully! (All logs)
poco/1.11.0@:
All packages built successfully! (All logs)
poco/1.10.0@:
All packages built successfully! (All logs)
poco/1.9.4@:
All packages built successfully! (All logs)
poco/1.8.1@:
All packages built successfully! (All logs)
poco/1.9.3@:
All packages built successfully! (All logs)

… Windows * [poco] support NetSSL with OpenSSL on Windows conan-io#6311 Changed Poco recipe (any version) to support the NetSSL component with OpenSSL on Windows again. Consumers should be able to either use NetSSL or NetSSL_win components with OpenSSL- or Windows TLS-enabled SSL support - or none - but not both. No changes for any non-Windows OS. * [poco] removed unnecessary default option * [poco] default of enable_netSSL_win set to false * the default for the Windows-only option enable_netssl_win is set to false * as the enable_netssl option's default is true (for any Platform), any consumer not specifying options will get the OpenSSL-supported NetSSL component * for explicitly use the WinTLS-supported NetSSL_win, the options have to be switched by the consumer * note that the Conan-Center-Index CI/CD will build and test the OpenSSL version on Windows

jngrb added 2 commits July 13, 2021 16:59

[poco] removed unnecessary default option

61fc059

This comment has been minimized.

Sign in to view

conan-center-bot added the No Beta user label Jul 13, 2021

ghost mentioned this pull request Jul 13, 2021

[service] Potentially conflicting recipes #3571

Closed

ghost mentioned this pull request Jul 13, 2021

fix poco 1.11.0 cmake bundle #6296

Merged

4 tasks

This was referenced Jul 15, 2021

[package] poco/1.9.4 to 1.11.0: cannot use NetSSL component with OpenSSL on Windows #6311

Closed

[poco] fix PocoNetSSL and PocoNetSSLWin simultaneous usage under Windows #4205

Merged

mathbunnyru suggested changes Jul 16, 2021

View reviewed changes

Merge branch 'conan-io:master' into master

bf2de23

This comment has been minimized.

Sign in to view

mathbunnyru approved these changes Jul 16, 2021

View reviewed changes

conan-center-bot removed the No Beta user label Jul 16, 2021

github-actions bot mentioned this pull request Jul 16, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

prince-chrismc approved these changes Jul 16, 2021

View reviewed changes

SSE4 requested a review from uilianries July 19, 2021 06:56

SSE4 reviewed Jul 19, 2021

View reviewed changes

SSE4 approved these changes Jul 19, 2021

View reviewed changes

SSE4 closed this Jul 19, 2021

SSE4 reopened this Jul 19, 2021

conan-center-bot merged commit e12d4f3 into conan-io:master Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[poco] re-enabling support for NetSSL with OpenSSL on Windows #6314

[poco] re-enabling support for NetSSL with OpenSSL on Windows #6314

jngrb commented Jul 13, 2021

CLAassistant commented Jul 13, 2021 •

edited

Loading

This comment has been minimized.

ghost commented Jul 13, 2021

Talkless commented Jul 16, 2021

Talkless commented Jul 16, 2021

mathbunnyru left a comment

mathbunnyru commented Jul 16, 2021

This comment has been minimized.

This comment has been minimized.

jngrb commented Jul 16, 2021

mathbunnyru left a comment

prince-chrismc commented Jul 16, 2021

SSE4 Jul 19, 2021

conan-center-bot commented Jul 19, 2021

[poco] re-enabling support for NetSSL with OpenSSL on Windows #6314

[poco] re-enabling support for NetSSL with OpenSSL on Windows #6314

Conversation

jngrb commented Jul 13, 2021

CLAassistant commented Jul 13, 2021 • edited Loading

This comment has been minimized.

ghost commented Jul 13, 2021

Talkless commented Jul 16, 2021

Talkless commented Jul 16, 2021

mathbunnyru left a comment

Choose a reason for hiding this comment

mathbunnyru commented Jul 16, 2021

This comment has been minimized.

This comment has been minimized.

jngrb commented Jul 16, 2021

mathbunnyru left a comment

Choose a reason for hiding this comment

prince-chrismc commented Jul 16, 2021

SSE4 Jul 19, 2021

Choose a reason for hiding this comment

conan-center-bot commented Jul 19, 2021

CLAassistant commented Jul 13, 2021 •

edited

Loading