Fix CMake variable check in OpenSSL test #16389
Conversation
This comment has been minimized.
This comment has been minimized.
ghost
mentioned this pull request
|
I detected other pull requests that are modifying openssl/1.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. |
This comment has been minimized.
This comment has been minimized.
| @@ -19,7 +19,7 @@ set(_custom_vars | |||
| OPENSSL_VERSION | |||
| ) | |||
| foreach(_custom_var ${_custom_vars}) | |||
| if(DEFINED _custom_var) | |||
| if(DEFINED ${_custom_var}) | |||
There was a problem hiding this comment.
I think it is correct as it is
There was a problem hiding this comment.
(unless it contains the name of another variable that is what you actually want to test).
We do not care about checking if _custom_var is defined. We care about checking if OPENSSL_VERSION etc are defined.
There was a problem hiding this comment.
I also came across this issue when implementing the OpenSSL 3.x.x test package in #14426 . There, I changed it to
if(DEFINED ${_custom_var} AND NOT "${${_custom_var}}" STREQUAL "" )
to also ensure that the variable is not empty
|
Good catch. It might be worth fixing this in test package of bzip2, expat, freetype, jasper, libxml2 and xz_utils. |
|
What is preventing this PR to be merged by merge-bot? |
|
V2 failed but its required for openssl |
|
It was a flaky fail so let's try again |
Why is it required? openssl 3.x is not v2 ready. |
Yes, I also would like to know this. And despite my fixes #14426 the v2 test still fails and I cannot see any results (I get an "access forbidden" page.) Thus, I cannot provide any further fixes. Given that conan v2 is released, I think OpenSSL 3.x.x should be made v2 ready soon. |
|
You need to follow the links posted by the bot to see the results. I might need to murder this not sure why it was added 🤢 |
|
I am pretty surprised how fast the v2 packages are being filled.... one this backlog of PRs get down we are going to check the number and see if we can require v2 for everything 🤞 |
Conan v1 pipeline ✔️All green in build 3 (
Conan v2 pipeline (informative, not required for merge) ✔️
All green in build 2 (
|
I did and I get "403 forbidden" ( for https://ci-conan-prod.jfrog.team/job/prod-v2/job/cci/job/PR-14426/16/display/redirect of PR #14426 ) sorry, out-of-topic here, but as the instruction was added here, I want to reply here. |
|
Don't look at status checks at the bottom. Look at the comments the bot posts #16389 (comment) |
Previously, the logic was checking if
_custom_varwas defined, which is clearly always true. Instead, we want to see if the variable referenced by_custom_varis defined.