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.

Sign up for GitHub

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

Jump to bottom

Fix handling of OpenSSL linking on macOS #17535

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix handling of OpenSSL linking on macOS #17535

wants to merge 7 commits into from
+59 −55

Conversation

Ferroin
Copy link
Member

@Ferroin Ferroin commented Apr 26, 2024 •
edited

Summary
  • Fix dependency installation in CI.
  • Switch to using an imported target for the TLS implementation.
  • Keep the TLS related stuff together.
  • Use the same name for the WolfSSL and OpenSSL targets, once detected the build system does not care which is being used beyond what the imported target tells it.
Test Plan

CI passes on this PR.

@Ferroin Ferroin changed the title Pull in all dependencies for macOS CI jobs. Fix handling of OpenSSL linking on macOS Apr 26, 2024
@github-actions github-actions bot added the area/build Build system (autotools and cmake). label Apr 26, 2024
@Ferroin Ferroin force-pushed the macos-ssl-ci-fix branch 2 times, most recently from d3984e9 to 8d4be59 Compare April 26, 2024 13:58
@Ferroin Ferroin force-pushed the macos-ssl-ci-fix branch 2 times, most recently from 53ad3f5 to feab040 Compare May 3, 2024 14:39
@Ferroin Ferroin marked this pull request as ready for review May 3, 2024 14:49
@Ferroin Ferroin requested review from vkalintiris and a team as code owners May 3, 2024 14:49
@Ferroin Ferroin requested a review from ilyam8 May 3, 2024 14:49
thiagoftsm
thiagoftsm previously approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@thiagoftsm thiagoftsm left a 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!

@ilyam8 ilyam8 marked this pull request as draft May 7, 2024 15:55
@Ferroin Ferroin marked this pull request as ready for review May 8, 2024 12:18
@Ferroin
Copy link
Member Author

Ferroin commented May 16, 2024

Rebased to resolve merge conflicts.

@Ferroin Ferroin requested a review from thiagoftsm May 16, 2024 12:46
@Ferroin
Pull in all dependencies for macOS CI jobs.
d4c0b35 
Instead of only pulling in a basic set.
@Ferroin
Switch to using imported target for OpenSSL in most cases.
88bb964
@Ferroin
Use imported libraries for OpenSSL in all cases.
a8d157d
@Ferroin
Work around broken behavior in FindPkgConfig with static builds.
a96f4d3 
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.
@Ferroin
Correctly detect static builds.
fc85452
@Ferroin
Fix H2O linking.
e9876da
@Ferroin
Copy link
Member Author

Ferroin commented May 17, 2024

Rebased to pick up the latest changes in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagoftsm thiagoftsm thiagoftsm left review comments

@vkalintiris vkalintiris Awaiting requested review from vkalintiris vkalintiris is a code owner

@ilyam8 ilyam8 Awaiting requested review from ilyam8

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/build Build system (autotools and cmake). area/ci
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Ferroin @thiagoftsm