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
cmake: migrate from USE_DARWINSSL to USE_SECTRANSP #3769
Conversation
One thing I find quite ugly is the usage of CMAKE_ prefix for our options/variables. |
What I like about the current setup is having two distinct variables for two different levels. As you can see in a commit-by-commit review, the first commit alone would have closed #3733 by changing the internal USE_DARWINSSL -> USE_SECTRANSP without changing the public cmake interface. I have no opinion on the nicest possible naming for cmake options, as I am not a cmake expert. |
I'm hoping this can be merged before the next release? For clarity: building via CMake with the DarwinSSL/SecureTransport backend is currently broken (since 7.64.1), and this fixes it. |
@past-due did you test this patch in your setup? If yes, that is valuable feedback and could help bringing this in. @MarcelRaad was so kind to review and merge the last cmake relates PR from me. Maybe he can find the time to have a look at this. |
Unfortunately I don't have a Mac and we don't have CMake SecureTransport CI builds, so I'd prefer if someone else tried this out before merging. Looks good to me though. |
@webmaster128, @MarcelRaad: Tested this patch on macOS 10.12 & 10.13. It successfully fixes the CMake SecureTransport builds. I will look into adding CMake SecureTransport builds to the Travis CI config (as a separate PR). |
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.
Tested on macOS 10.12 & 10.13.
Successfully fixes the CMake SecureTransport builds.
Thanks! If noone else does, I'll merge as soon as I'm near a computer again, which will probably be late next week. |
Merged now. Thanks again! |
Closes #3733