-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[openssl 3.x.x] fix "official" OpenSSL vars for CMakeDeps gen #14426
Conversation
I detected other pull requests that are modifying openssl/3.x.x recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
test_type = "explicit" | ||
|
||
@property | ||
def _skip_test(self): |
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.
You can't skip a test. If something does not work, please, raise a ConanInvalidConfiguration in validate()
method in the recipe
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 just copied & pasted this part from the "v2 test package" for 1.x.x: https://github.com/conan-io/conan-center-index/blame/master/recipes/openssl/1.x.x/test_package/conanfile.py#L16
I originates here: #9208 (comment)
As this test skipping was not part of the "v1 test package" for 3.x.x, I will give it a try not to have this in the "v2 test package" for 3.x.x
This comment has been minimized.
This comment has been minimized.
Giving some background for this change: I would like to be able to override openssl to version 3.0.7 in Poco. As of now, since Poco was changed to CMakeDeps in #12868 , this no longer works:
=> works
(I added a --require-override=openssl/3.0.7@ here.) => fails as of now, because Poco's CMakeLists.txt does not see a positive OPENSSL_FOUND and thus I run into #13577 (so far only fixed for OpenSSL 1.x.x) (Note: I only disabled any irrelevant Poco component just to have it compile faster so that I get results more quickly.) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e3e8a46
to
7802015
Compare
This comment has been minimized.
This comment has been minimized.
Based on my tests with the last commits, I see that the MacOS failures are not introduced with this PR. It seems that the legacy provider from the shared object A workaround is to either use As I cannot change the default value for these options without introducing breaking changes for the consumer, I can only raise a ConanInvalidConfiguration exception (in line with the comment above) for |
7802015
to
cb49200
Compare
This comment has been minimized.
This comment has been minimized.
assert os.path.exists(os.path.join(self.deps_cpp_info["openssl"].rootpath, "licenses", "LICENSE.txt")) | ||
|
||
for fn in ("libcrypto.pc", "libssl.pc", "openssl.pc",): | ||
assert os.path.isfile(os.path.join(self.build_folder, fn)) |
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 did you delete this?
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 these files are no lot generated for the CMakeDeps generator.
I would need the cmake, cmake_find_package, and pkg_config generators as for the test_v1_package test package (where this check is included).
Also compare that the *.pc files are also only checked for the 1.x.x test_v1_package, not for the 1.x.x test_package.
Co-authored-by: Michael Keck <git@cr0ydon.com>
plus some cleanup
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e1eb0c0openssl/3.0.7
openssl/3.0.5
|
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.
LGTM
Attempted the conan v2 port for OpenSSL 3.x.x in PR #16563 |
Is there a reason why this is still in the queue? |
Hi @roalter - this is on my stack today, including for support for Conan 2.0 for this recipe. The issues that @jngrb mentions in #14426 (comment) were addressed not long ago. I will make sure to include version 3.1.0 as well, thanks for the heads up! |
Conan v1 pipeline ✔️All green in build 16 (
Conan v2 pipeline (informative, not required for merge) ❌
The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future. See details:Failure in build 17 (
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. |
Hooks produced the following warnings for commit 4d4c533openssl/3.0.7
openssl/3.0.8
openssl/3.0.5
|
Hi @jngrb - I've partially added the changes from this PR into this one, preserving your authorship: Please let me know if this addresses the issues with poco that you have mentioned. If there are any other changes from this PR that you wish you bring forward, please let us know once the PR branch has been sync'ed up with the head of master, and we'll happily review this. If the recipe is already satisfactory, please close this PR :) Thanks for your contribution! |
Thank you. All my requirements are now met. I close this PR. |
this extends the improvements of PR #12838 done for the 1.x.x recipe to the 3.x.x one; also there is a new 3.x.x test_package using the CMakeDeps generator
Reason: without this change, it is currently not possible to override the OpenSSL version to 3.0.x in Poco (see comment below)