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

Windows: Add option to build CMake's internal curl with winsspi #29943

Merged
merged 4 commits into from
Apr 16, 2022

Conversation

johnwparent
Copy link
Contributor

@johnwparent johnwparent commented Apr 7, 2022

CMake's internal copy of curl used with the +ownlibs variant has the ability to link against the winsspi.dll, a Windows native implementation of SSL. This PR exposes another variant of the CMake package on Windows allowing users to build CMake with winsspi, rather than OpenSSL.

Some small, Windows specific cleanup was also done in this PR.

@spackbot-app spackbot-app bot requested a review from chuckatkins April 7, 2022 14:22
@johnwparent johnwparent changed the title Windows: Add option to build with CMake's internal curl with winsspi Windows: Add option to build CMake's internal curl with winsspi Apr 7, 2022
@johnwparent johnwparent added this to In Progress in Spack on Windows via automation Apr 7, 2022
@johnwparent johnwparent moved this from In Progress to In Review in Spack on Windows Apr 7, 2022
@chuckatkins
Copy link

Just recently in #29847 the openssl variant was dropped as it's not particularly useful to have a CMake that uses curl without ssl support. Rather than add another variant I think it just makes more sense to always use the native windows ssl.

@BetsyMcPhail ?

scheibelp
scheibelp previously approved these changes Apr 7, 2022
Spack on Windows automation moved this from Awaiting Review to In Progress Apr 7, 2022
@scheibelp
Copy link
Member

Just recently in #29847 the openssl variant was dropped as it's not particularly useful to have a CMake that uses curl without ssl support. Rather than add another variant I think it just makes more sense to always use the native windows ssl.

This preserves that choice but is allowing Spack to set the SSL implementation (and also updates the CMake package to choose the Windows-provided SSL by default). I think it's a separate discussion whether that choice should be retracted entirely (for now these changes are needed to generally build, and removing the variant is a matter of deciding whether the user should be allowed to undo that choice) so I'm inclined to merge this as is.

1 similar comment
@scheibelp

This comment was marked as duplicate.

@scheibelp scheibelp enabled auto-merge (squash) April 7, 2022 23:37
depends_on('openssl')
depends_on('openssl@:1.0', when='@:3.6.9')

if is_windows:
Copy link
Member

@haampie haampie Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the deal with all this is_windows? you should always check the target, not the host, we have platform=... for specs. All this is_windows stuff is so unreadable, please improve it. Also the nesting with - if - with - f() does not help in improving readability...

@johnwparent
Copy link
Contributor Author

Just recently in #29847 the openssl variant was dropped as it's not particularly useful to have a CMake that uses curl without ssl support. Rather than add another variant I think it just makes more sense to always use the native windows ssl.

This preserves that choice but is allowing Spack to set the SSL implementation (and also updates the CMake package to choose the Windows-provided SSL by default). I think it's a separate discussion whether that choice should be retracted entirely (for now these changes are needed to generally build, and removing the variant is a matter of deciding whether the user should be allowed to undo that choice) so I'm inclined to merge this as is.

FWIW I spoke with Brad King and Bill today, and in general it seems the consensus is that with this support for the use of winsspi.dll, we should no longer even offer the option to build with OpenSSL. I can provide more detail/context if needed, but if we want to make another PR to that effect, such detail can be reserved until that point.

@johnwparent johnwparent force-pushed the win-cmake-use-winssl branch 2 times, most recently from e400883 to 2773ba3 Compare April 12, 2022 20:38
@johnwparent johnwparent moved this from In Progress to Awaiting Review in Spack on Windows Apr 14, 2022
@johnwparent johnwparent moved this from Awaiting Review to In Progress in Spack on Windows Apr 15, 2022
Spack on Windows automation moved this from In Progress to Awaiting Review Apr 15, 2022
@scheibelp scheibelp mentioned this pull request Apr 15, 2022
@scheibelp scheibelp merged commit 840858c into spack:develop Apr 16, 2022
Spack on Windows automation moved this from Awaiting Review to Done Apr 16, 2022
joequant pushed a commit to joequant/spack that referenced this pull request Apr 17, 2022
Add option to allow using OpenSSL (by default this uses the SSL
implementation that comes with Windows, since that is more likely
to have needed certificates).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants