-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-45449: [R][CI] Remove OpenSSL 1.x builds #47280
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
Conversation
|
|
| #if OPENSSL_VERSION_NUMBER >= 0x30000000L | ||
| #error Using OpenSSL version 3 | ||
| #endif |
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 you need to remove this now, you either have openssl 3.0 or not
| #if OPENSSL_VERSION_NUMBER >= 0x30000000L | |
| #error Using OpenSSL version 3 | |
| #endif |
| # openssl is < 3.0 | ||
| lg("Found libcurl and OpenSSL >= 1.1") | ||
| return("openssl-1.1") | ||
| # There was no error in compiling: so we found libcurl and OpenSSL >= 3.0, |
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.
See comment above, if you don't remove that second check, the script will error on openssl >= 3.0 so this won't be true.
| } | ||
| lg("Found libcurl and OpenSSL < 1.1") | ||
| return("openssl-1.0") | ||
| } else if (any(grepl("Using OpenSSL version 3", errs))) { |
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.
And I think you can remove this last else if
|
Thanks for the review @nealrichardson! Just FYI, I'm now no longer entirely convinced that the source of the problem with the binaries was definitely OpenSSL 1.X compatibility and I'm going to look into that a bit more. |
|
This fix isn't actually needed; superseded by #47299 |
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?