-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[poco] fix PocoNetSSL and PocoNetSSLWin simultaneous usage under Windows #4205
[poco] fix PocoNetSSL and PocoNetSSLWin simultaneous usage under Windows #4205
Conversation
Hi, The pull request? Thanks |
With this, i only need "enable_net" correct? |
@mathbunnyru is this that im thinking in do after. Thanks for you time. |
Yes. Or you could try default options as well if you don't need any components which are not turned on by default. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
recipes/poco/all/conanfile.py
Outdated
"PocoNetSSL": _PocoComponent("enable_netssl", True, ("PocoCrypto", "PocoUtil", "PocoNet", ), True), # also external openssl | ||
"PocoNetSSLWin": _PocoComponent("enable_netssl_win", True, ("PocoNet", "PocoUtil", ), True), | ||
"PocoNetSSL": _PocoComponent("enable_netssl", not tools.os_info.is_windows, ("PocoCrypto", "PocoUtil", "PocoNet", ), True), # also external openssl | ||
"PocoNetSSLWin": _PocoComponent("enable_netssl_win", tools.os_info.is_windows, ("PocoNet", "PocoUtil", ), True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be self.settings.os == Windows
instead? my doubt that it may break cross-building scenarios (e.g. Windows -> Linux or Linux -> Windows)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can I use self in class namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SSE4 could you elaborate on this, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible. probably, you need to add both components and then process it in config_options
or somewhere else.
the issue is running on Linux with self.settings.os == Windows
, tools.os_info.is_windows
will return False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try replacing tools.os_info.is_windows with self.settings.os == Windows as suggested by @SSE4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave this option to True, since it is removed in other OSs different from windows
|
||
def build(self): | ||
cmake = CMake(self) | ||
cmake.definitions["TEST_CRYPTO"] = self.options["poco"].enable_crypto == True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
This comment has been minimized.
This comment has been minimized.
Failure in build 21 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Investigating together with @danimtb 👍 |
I would try replacing tools.os_info.is_windows with self.settings.os == Windows as suggested by @SSE4. |
Sorry, I'm on a one week vacation, so I won't be able to respond here for a while. |
We need to add the changes of test_package/conanfile.py to avoid the error of the consumer in the CI. The issue with the failed CI status is due to Jenkins blocking the test_package executable due to networking. I'd suggest to comment the lines and add the reason like:
|
@danimtb |
@dmn-star I see. I have tested with the commented lines above on our staging environment and it is working. I will try to push the changes here and see what happens |
All green in build 22 ( |
Thanks @danimtb ! 👍 |
@SpaceIm @SSE4 @ericLemanissier @danimtb please, approve the PR :) |
@jgsogo @danimtb This PR triggered a build on master and it looks stuck.. it's been about ~10 hrs with master status 'being built'. It's a PR from hell 🙃 |
OMG 😫 I've stopped and triggered again the build for |
@jgsogo @danimtb I think this should be changed again in order to again allow OpenSSL-enabled SSL support under Windows see my discussion with @mathbunnyru in my issue #6311 und my suggested solution in #6314 |
Hi, @jngrb ! This kind of thing will always happen from time to time. I know people are doing their best in this repository, but we are humans sometimes 🙂 I will add another batch of people to the EAP so your PR can run, and people with much more experience than me will notice about it and review. Anyway, if you are using these recipes in a production environment (cc @Talkless ) I encourage you to use recipe revisions and/or lockfiles. That way, you will be using always the same version of the recipe and you can decide when to update and start using modifyied versions. |
Words to live by! |
Specify library name and version: lib/1.0
conan-center hook activated.