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: Add CURL_CA_BUNDLE/CURL_CA_FALLBACK/CURL_CA_PATH #1461

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@webmaster128
Contributor

webmaster128 commented May 1, 2017

No description provided.

@mention-bot

This comment has been minimized.

mention-bot commented May 1, 2017

@webmaster128, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Sukender, @jzakrzewski and @billhoffman to be potential reviewers.

@bagder bagder added the cmake label May 1, 2017

@Paxxi

This comment has been minimized.

Paxxi commented May 20, 2017

Looks good to me except one thing.

auto should probably be ignored on Windows as none of the paths are valid and there's really no concept of a system wide cert bundle for OpenSSL. I would check for the WIN32 variable and skip the test. It's true both for x86 and x64 despite it's name.

@webmaster128

This comment has been minimized.

Contributor

webmaster128 commented May 20, 2017

auto should probably be ignored on Windows as none of the paths are valid and there's really no concept of a system wide cert bundle for OpenSSL. I would check for the WIN32 variable and skip the test. It's true both for x86 and x64 despite it's name.

Do I understand correctly that this a matter of taste with no impact on the behaviour of the script?

Whenever possible, I'd prefer a general solution over a platform specific solution to avoid extra complexity that comes with case distinctions. When we set a bunch of possible paths, that do not exist on Windows, fine. But why not add a Windows path into the mix? The current code would allow that (but as you said, there is no such path).

One thing that is let's me tent to a general solution is that "being on Windows" is not a black or white question. With Cygwin and MinGW in the mix, we need to support systems that are more or less like Windows, which makes feature testing much easier than OS distinctions. When you search the Internet for CMAKE_LEGACY_CYGWIN_WIN32 you see that there was a lot of confusion about if Cygwin is a WIN32 for cmake or not (impacting our 2.8 minimum version as well). So I'd rather not be in the business of detection how much Windows-like a system is.

As far as I can see, there is no harm in finding and using given paths on a Windows-like system with a Unix-like file system.

@Paxxi

Paxxi approved these changes May 20, 2017

I think you're right. I generally try to avoid checks not needed on the platform out of habit.

To make sure I did test this PR on Windows now and the tests doesn't generate any message/warnings/errors so should be perfectly fine as is.

@bagder

This comment has been minimized.

Member

bagder commented May 21, 2017

Thanks a lot for the extra set of eyes and comments. I'll merge this set!

@webmaster128: just a request for the future: please check the commit message style we use and try to write the first line in that style. I've edited the commits now to adhere.

@bagder bagder closed this in 6a9489d May 21, 2017

@webmaster128 webmaster128 deleted the webmaster128:ca-path-bundle branch May 21, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018

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