GHA/windows: enable MulitSSL in an MSVC job

[talregev]

Ref: 98da147 #14305
Closes #14276

[vszakats]

Can you make this part of an existing job?

These jobs are already flaky and each one added increases the chance for a PR to go red.

[talregev]

Can you make this part of an existing job?

These jobs are already flaky and each one added increases the chance for a PR to go red.

This PR is just meant to show the compilation problem. When it solve, we can think where to put the multissl.
I can put this PR on draft.

[talregev]

@vszakats I add another suggestion.

[talregev]

https://stackoverflow.com/questions/40738351/forward-declaration-as-struct-vs-class

I think the link is misleading, because it talk another warning.

[talregev]

https://learn.microsoft.com/en-us/cpp/overview/cpp-conformance-improvements-2017?view=msvc-170#uninitialized-const-variables

There is a problem with forward deceleration and const. Maybe it is msvc bug?

[bagder]

const object should be initialized

Global variables are guaranteed to be initialized to zero according to the standard. So what exactly does this mean?

[talregev]

const object should be initialized

Global variables are guaranteed to be initialized to zero according to the standard. So what exactly does this mean?

On the original code on line 417, is forward declaration, not a definition. It should not do initialization on this line.
For me it seems that you cannot do forward declaration with static const in msvc, even if it legal in c. That why I think it msvc bug.

[talregev]

https://learn.microsoft.com/th-th/cpp/cpp/declarations-and-definitions-cpp?view=msvc-170#definitions

[talregev]

https://stackoverflow.com/a/936589/550384
https://comp.lang.cpp.narkive.com/IYkOYzm0/forward-declaration-of-static-variable

[vszakats]

Looking good! As discussed earlier, I'd prefer integrating this into an existing job, rather than adding a new one, to avoid adding more chance for flakiness.

How about enabling Schannel in the existing 'openssl' job?

[talregev]

I know you don't want to add many tests. but my plan is to have multissl to install as much ssl backends for curl together.
I already succeeded with vcpkg to have 4
Can you consider it again?

C:\src\vcpkg\installed\x64-windows\tools\curl>curl --version
curl 8.9.0-DEV (Windows) libcurl/8.9.0-DEV GnuTLS/3.8.4 (mbedTLS/2.28.8) (OpenSSL/3.3.1) (Schannel) zlib/1.3.1
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ipfs ipns mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS HSTS HTTPS-proxy IPv6 Kerberos Largefile libz MultiSSL NTLM SPNEGO SSL SSPI threadsafe TLS-SRP Unicode UnixSockets

[vszakats]

Tests will only run on the default TLS backend, so running with extra backends will not increase test coverage. Having a single extra backend tests the MultiSSL codepath, this is covered in this PR. Adding more backends would only add build tests for the extra backends. We have this covered for many now in separate jobs, and new ones can be appended to an existing job.

I think the situation with flaky MSVC jobs is already very bad, breaking almost all runs. I don't know why that it, but adding more flaky jobs don't seem to help us because of this.

All in all, I think we'd better off we using the existing openssl job to test MultiSSL, and untested backends can be appended there too.

[talregev]

Can we just build the tests without running them on multissl job?

Build the tests in different compiler will raise new warning that we can solve like this one.

[vszakats]

Why would that be better than adding it to an existing job?

[talregev]

My goal is to add on the same build, as much backend together. If I add more backends in openssl test, it will be not be a default anymore, because curl choose the default automatic according the first in the alphabet order.
As you can see this code is not compile with msvc on curl ci and then we found warning / errors on the code.
Also I want to find these error and warning, and try to fix them if possible.

[talregev]

I am not sure why the ci is failing.

[vszakats]

My goal is to add on the same build, as much backend together. If I add more backends in openssl test, it will be not be a default anymore, because curl choose the default automatic according the first in the alphabet order. As you can see this code is not compile with msvc on curl ci and then we found warning / errors on the code. Also I want to find these error and warning, and try to fix them if possible.

You can manually select the default TLS backend using -DCURL_DEFAULT_SSL_BACKEND= if it becomes something else than OpenSSL.

The MultiSSL compiler warning came up because of enabling MultiSSL at all. Adding more backends will not uncover more such issues. Either way I've nowhere said not to enable more backends, but to add new ones to an existing job.

Also notice that MultiSSL isn't really where the puck is going in curl, e.g. because it lacks support for HTTP/3 and the original reasons for having it are now obsolete:
#12807 (comment)
#14308 (comment)

[talregev]

ok. I will Enable Schannel in openssl job.
Thank you for your review, and time for listening my ideas.

[talregev]

we can wait until the PR #14305 will be merge, and I will rebase this PR from the master after the merge.

CI broken

[vszakats]

We need to find another job for MultiSSL. The openssl one has HTTP/3 enabled which is incompatible with MultiSSL. Hence the CI failure.

How about the 'Schannel U' job? It needs the -DCURL_DEFAULT_SSL_BACKEND=schannel option for keeping Schannel the default.

[talregev]

Done.

[vszakats]

Thanks @talregev!