-
Notifications
You must be signed in to change notification settings - Fork 5.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
Fix handling of OpenSSL linking on macOS #17535
Conversation
d3984e9
to
8d4be59
Compare
53ad3f5
to
feab040
Compare
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.
On FreeBSD. the nearest I can be from macOS, everything worked as expected. PR compiled, dbengine is working as expected, and I can see host on cloud. LGTM!
Rebased to resolve merge conflicts. |
Rebased to pick up the latest changes in master. |
09ec009
to
ac554fd
Compare
Rebased to resolve merge conflicts. |
Instead of only pulling in a basic set.
It happened to be working before without this because we weren’t hitting any edge cases, but the use of IMPORTED targets with PkgConfig requires these fixes to behave correctly for transitive dependencies in static builds.
If we fail to find libcrypto when we found openssl with pkg_check_modules, then the openssl install is horribly broken and we will see failures either at link time or at runtime, so there is no point in not checking for it on macOS. This also more clearly delineates that we _do_ require libcrypto irrespective of the platform.
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 am approving again considering tests on Linux and FreeBSD. It is necessary to test on Mac.
* Pull in all dependencies for macOS CI jobs. Instead of only pulling in a basic set. * Switch to using imported target for OpenSSL in most cases. * Use imported libraries for OpenSSL in all cases. * Work around broken behavior in FindPkgConfig with static builds. It happened to be working before without this because we weren’t hitting any edge cases, but the use of IMPORTED targets with PkgConfig requires these fixes to behave correctly for transitive dependencies in static builds. * Correctly detect static builds. * Fix H2O linking. * Fix typo. * Always check for libcrypto if we found openssl. If we fail to find libcrypto when we found openssl with pkg_check_modules, then the openssl install is horribly broken and we will see failures either at link time or at runtime, so there is no point in not checking for it on macOS. This also more clearly delineates that we _do_ require libcrypto irrespective of the platform.
* Pull in all dependencies for macOS CI jobs. Instead of only pulling in a basic set. * Switch to using imported target for OpenSSL in most cases. * Use imported libraries for OpenSSL in all cases. * Work around broken behavior in FindPkgConfig with static builds. It happened to be working before without this because we weren’t hitting any edge cases, but the use of IMPORTED targets with PkgConfig requires these fixes to behave correctly for transitive dependencies in static builds. * Correctly detect static builds. * Fix H2O linking. * Fix typo. * Always check for libcrypto if we found openssl. If we fail to find libcrypto when we found openssl with pkg_check_modules, then the openssl install is horribly broken and we will see failures either at link time or at runtime, so there is no point in not checking for it on macOS. This also more clearly delineates that we _do_ require libcrypto irrespective of the platform.
Summary
Test Plan
CI passes on this PR.