-
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
cmake/3.x.x: bump OpenSSL #20438
cmake/3.x.x: bump OpenSSL #20438
Conversation
🤖 Beep Boop! This pull request is making changes to 'recipes/cmake//'. 👋 @Croydon you might be interested. 😉 |
recipes/cmake/3.x.x/conanfile.py
Outdated
@@ -38,7 +38,7 @@ def config_options(self): | |||
|
|||
def requirements(self): | |||
if self.options.with_openssl: | |||
self.requires("openssl/1.1.1t") | |||
self.requires("openssl/1.1.1w") |
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.
Why don't you use version range like all other recipes?
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.
because versions of cmake go far back and the support for 3.x is not clear. Given those are not being built by cci, I'd rather go the safe way at first. It can still be made a version range later with the correct pivot.
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.
Could you test if it pass with openssl 3.x? It would avoid all these PR bumping versions of dependencies.
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 can't test this on macOS. The build does not use OpenSSL.
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.
So why openssl is dragged irrespective of os? 🤔
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.
Do a test build in CI with self.requires("openssl/[>=1.1 <4]")
and update the recipe accordingly, perhaps?
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.
Do a test build in CI with
self.requires("openssl/[>=1.1 <4]")
and update the recipe accordingly, perhaps?
Alternatively, just use the version range for the newest CMake versions and leave it to 1.1 for older.
Hi @mayeut, thanks for your contribution. As for the OpenSSL version, please also be aware that 1.1.1 is already End-of-Life: https://www.openssl.org/blog/blog/2023/03/28/1.1.1-EOL/ - we are working towards removing any references to OpenSSL 1.x across all of Conan Center - so this should be using a newer version. |
I know, that's in one of my comments & I just added 3.27.7 in #20440
Yes, I'm aware of that. But if something doesn't work with OpenSSL 3.x, what would you do, just remove the version ? I know that for CMake, 3.22.3 does support OpenSSL 3.x (all of CMake tests are passing in https://github.com/scikit-build/cmake-python-distributions), before that I don't know (it was built but not tested in 3.21.3 there). If you agree to to remove at least 3.19.8 & 3.20.6 (or can test on those versions), I can update the PR with the OpenSSL range, otherwise, please close the PR. |
if self.options.with_openssl: | ||
self.requires("openssl/1.1.1t") | ||
if self.options.get_safe("with_openssl", default=False): | ||
self.requires("openssl/[>=1.1 <4]") |
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 have tested that the oldest version in the recipe 3.19.8
is compatible with the most recent OpenSSL 3.x
Conan v1 pipeline ❌Sorry, the system is under maintenance and it doesn't accept builds right now. Motivation: Internal maitenance Jun 12th Please, check https://status.conan.io to obtain more information. Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping See details:Sorry, the system is under maintenance and it doesn't accept builds right now. Motivation: Internal maitenance Jun 12th Please, check https://status.conan.io to obtain more information. |
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.
Thanks @mayeut - I've tested OpenSSL 3.x with the oldest version supported by the recipe, so have amended this to use the version range.
On the other hand, I've removed the patch to "remove" the with_openssl
options "when not on Linux" - I suspect that other platforms (such as a xBSD) also use OpenSSL - so would not want to remove that ability
Specify library name and version: cmake/3.x.x