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.

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

cmake: migrate from USE_DARWINSSL to USE_SECTRANSP #3769

Closed
wants to merge 2 commits into from
Closed

cmake: migrate from USE_DARWINSSL to USE_SECTRANSP #3769

wants to merge 2 commits into from

Conversation

webmaster128
Copy link
Contributor

Closes #3733

@bagder bagder added the cmake label Apr 12, 2019
@jzakrzewski
Copy link
Contributor

One thing I find quite ugly is the usage of CMAKE_ prefix for our options/variables.
Could we avoid this at least for the new code? Tools also tend to group the options according to the variable prefix. Ideally we would decide on some kind of naming scheme and clean this up once but I'm a bit worried about existing scripts.

@webmaster128
Copy link
Contributor Author

webmaster128 commented Apr 12, 2019 •
edited

One thing I find quite ugly is the usage of CMAKE_ prefix for our options/variables.
Could we avoid this at least for the new code? Tools also tend to group the options according to the variable prefix. Ideally we would decide on some kind of naming scheme and clean this up once but I'm a bit worried about existing scripts.

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.

@past-due
Copy link

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.

@webmaster128
Copy link
Contributor Author

@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.

@MarcelRaad
Copy link
Member

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.

@past-due
Copy link

@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).

past-due
past-due approved these changes Apr 19, 2019
View reviewed changes
Copy link

@past-due past-due left a 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.

@MarcelRaad
Copy link
Member

Thanks! If noone else does, I'll merge as soon as I'm near a computer again, which will probably be late next week.

@MarcelRaad
Copy link
Member

Merged now. Thanks again!

@webmaster128 webmaster128 mentioned this pull request Apr 27, 2019
Closed
@past-due past-due mentioned this pull request May 17, 2019
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Jul 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@past-due past-due past-due approved these changes

Assignees
No one assigned
Labels
cmake
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cmake: DarwinSSL setting not passed to curl configuration
5 participants
@webmaster128 @jzakrzewski @past-due @MarcelRaad @bagder