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

cmake/3.x.x: bump OpenSSL #20438

Merged
merged 3 commits into from
Jun 12, 2024
Merged

cmake/3.x.x: bump OpenSSL #20438

merged 3 commits into from
Jun 12, 2024

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Oct 8, 2023

Specify library name and version: cmake/3.x.x


@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2023

🤖 Beep Boop! This pull request is making changes to 'recipes/cmake//'.

👋 @Croydon you might be interested. 😉

@@ -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")
Copy link
Contributor

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?

Copy link
Contributor Author

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.

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 test if it pass with openssl 3.x? It would avoid all these PR bumping versions of dependencies.

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 can't test this on macOS. The build does not use OpenSSL.

Copy link
Contributor

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? 🤔

Copy link
Contributor

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?

Copy link
Contributor

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.

@AbrilRBS AbrilRBS requested a review from jcar87 October 9, 2023 08:40
@jcar87
Copy link
Contributor

jcar87 commented Oct 9, 2023

Hi @mayeut, thanks for your contribution.
Please be aware that this recipe is not actively built by our CI service, it has been kept here for reference and convenience for users who wish to build CMake from source. The actively maintained version of the recipe simply re-packages the binaries provided by KitWare.

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.

@mayeut
Copy link
Contributor Author

mayeut commented Oct 9, 2023

@jcar87,

Please be aware that this recipe is not actively built by our CI service, it has been kept here for reference and convenience for users who wish to build CMake from source. The actively maintained version of the recipe simply re-packages the binaries provided by KitWare.

I know, that's in one of my comments & I just added 3.27.7 in #20440

As for the OpenSSL version, please also be aware that 1.1.1 is already End-of-Life

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]")
Copy link
Contributor

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-center-bot
Copy link
Collaborator

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.
Thanks for your understanding and help with the Conan Center Index!

Conan v2 pipeline ❌

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

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 @conan-io/barbarians on the PR and we will help you.

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.
Thanks for your understanding and help with the Conan Center Index!

Copy link
Contributor

@jcar87 jcar87 left a 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

@jcar87 jcar87 merged commit e3d0450 into conan-io:master Jun 12, 2024
6 of 8 checks passed
@mayeut mayeut deleted the cmake-bump-openssl branch June 13, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants