-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
[curl] Add http3 feature on linux only #37146
base: master
Are you sure you want to change the base?
Conversation
ports/curl/vcpkg.json
Outdated
"supports": "(uwp | !windows) & !(osx | ios) & !mingw", | ||
"dependencies": [ | ||
{ | ||
"name": "curl", | ||
"default-features": false, | ||
"features": [ | ||
"ssl" | ||
] |
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.
The dependency chooses the platform's default SSL backend. But the new feature/patch seems to require that the backend is openssl
.
OTOH the supports
field excludes platforms which could use openssl
instead of another default backend.
This probably needs a more thorough review than usual.
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.
It need ssl, not just openssl. And vcpkg install another ssl as default beside openssl for the excluded platforms.
Then it create multi ssl feature enable.
Http3 cannot install with multi ssl feature enable.
Also I try manual (with change of the json local on my pc) to compile on windows curl with openssl and ssl and http3 and tool features without multi ssl feature.
It compile, but cannot run curl tool, because openssl cannot communicate with the os cartificate, and search for certificate file, and it create a strange error that for normal user it will be hard to debug. (and cannot use curl anyway that way).
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.
By depending on feature ssl
, it always pulls in the default SSL - the user cannot opt out of schannel
or sectransp
when requesting http3
.
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.
feature ssl is coming by default with curl in vcpkg.
What are you suggesting?
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.
@dg0yt Do you have windows on your side?
Try to build curl with tool and http3, and use the tool on windows.
Let me know what do you think.
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.
I can change dependencies of http3 from curl[ssl] -> curl[openssl], and remove the excluded platforms.
But then, when review here will test all feature installation on windows, it will create build fail.
Also when people will install all curl features on windows, it create build fail.
Is it wanted behavior?
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.
I changed dependencies of http3 from curl[ssl] -> curl[openssl].
I did not remove the excluded platforms.
I didn't remove the the excluded platforms because when people install all the features in the excluded platforms it will create a build failure.
943acf0
to
30ca630
Compare
@FrankXie05 @Cheney-W Any news? |
The feature |
@@ -45,6 +46,24 @@ | |||
"nghttp2" | |||
] | |||
}, | |||
"http3": { | |||
"description": "HTTP3 support", | |||
"supports": "(uwp | !windows) & !(osx | ios) & !mingw", |
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.
This is complex and probably wrong. Are mingw and osx really unsupported? Is uwp really supported, and regular windows isn't?
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.
I copy that from the ssl support for openssl. It exact the same.
https://github.com/microsoft/vcpkg/pull/37146/files#diff-a91e0d663e8719fb2cb62552c202144c9be4e9a2548b555644fd3399673cddb0R171
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.
@dg0yt As I mention and you keep ignore my comment.
When you install all the features in the excluded platforms, it will cause build failure.
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.
I copy that from the ssl support for openssl. It exact the same.
Note: This is the selection of openssl as the default. The use of openssl
is not restricted. You can vcpkg install curl[core,openssl]:x64-windows
if you don't want schannel
.
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.
vcpkg don't let you truly to use ssl as you want. The choise of ssl also backed in the features.
A simple example that will represent common use that represent the problem I am talking about. On windows:
vcpkg install curl[core,http3,http2] ->
a. from http3 -> curl[http3,openssl]
b. from http2 -> curl[http2,ssl] -> curl[http2,ssl,schannel]
together: curl[http3,http2,openssl,schannel,ssl] -> enable multissl feature then cause build error with http3.
Beside that simple example, you cannot use curl[http3] on excluded platforms with other ports that use openssl, because they install ssl feature by default, and that make curl[http3] useless for them.
So until curl port will really let choose ssl backend on user choice and not by os type, I am put that restrictions. You can try to fix curl port after my merge, and see the problems yourself.
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.
The vcpkg maintainers talked about this today and we agree with removing the TLS backend features in favor of adding an HTTP3 feature; @BillyONeal to test that upgrade does the right thing (we are 90% sure it does)
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.
@BillyONeal I already have PR for that. #37450
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.
I will put this one on draft, and activate the other one.
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.
The vcpkg maintainers talked about this today and we agree with removing the TLS backend features in favor of adding an HTTP3 feature; @BillyONeal to test that upgrade does the right thing (we are 90% sure it does)
What does this mean? openssl everywhere?
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.
Backend comparision: https://curl.se/docs/ssl-compared.html
To core team: Review the feature interface more thoroughly than usual before final merge.
|
To make it clear: I'm in favor of adding the feature. However I would prefer a less restrictive approach towards |
We can always add the feature with restrictions, and the next PR we will try to remove them, and fix the port such that it can let the user more flexibility. I am also in favor to change curl port such that I can install curl[core,http3,http2] on windows, but we need to change the curl port for that. |
FTR curl still allows to select the actual backend at runtime. This type choice is on the maintainer's guide allow list. |
This feature on curl is call multissl, it enable when you have multiple ssl backend compiled. But it not allowed and will cause build error when you compile multi ssl with http3. |
@vicroms Any news? |
30ca630
to
f8dcda9
Compare
@BillyONeal I fix the conflicts. |
I think it might be appropriate
i.e you can use it if you pay attention to the limitations. |
@FrankXie05 @Cheney-W Can you put the review tag, that the vcpkg team will discuss this PR again? |
We discussed this as a team yesterday and wrote down the following buckets:
@dan-shaw abstains Open questions:
|
@bagder confirms that HTTP3 as a whole is no longer experimental but that ngtcp2 is the only non-experimental backend here: https://mastodon.social/@bagder/112141781319579277 |
|
@BillyONeal Curl with http3 with openssl + nghttp3 will be less time experimental if people will install it and test it and report bugs. |
@BillyONeal This PR is answer both point 2 and 3. |
I believe the majority position right now is to not add an http3 feature, but that was pending further investigation, including discussions with curl's maintainers and other distributors of libcurl to hear about what they're doing. I started a discussion on the curl distros mailing list ( https://daniel.haxx.se/blog/2024/03/25/curl-distro-report/ ) to see if anyone has better ideas. That none of the platforms' TLS implementations work is really debilitating for such a feature :( |
@BillyONeal From the page that you send, it seems the distro will choose the way I choose bring http3 to vcpkg:
Also soon will be openssl 3.3 They already release the alpha. |
@BillyONeal As curl release new version 8.7.1 |
Unfortunately the response I have gotten so far is basically 'nobody has any idea how http3 will work'. The only vaguely affirmative response I got was from from MacPorts folks who said, if I understand correctly, 'we chose gnutls for all the things and it works with http3 so we turned it on'. |
@BillyONeal What is your thoughts about this and how you want to proceed? |
Unfortunately, I don't know at this time. I hope the concerns that we have raised here make sense; it isn't that we don't want an HTTP3 feature. We do want an HTTP3 feature. But we also have to make sure we aren't breaking folks or introducing problems for the integrity of the registry as a whole for such an important and widely used port as curl. That's why I followed this up with the upstream maintainers and other distributions: these problems are not unique to vcpkg; the other distros (apart from, apparently, MacPorts) are going to have these issues too. |
@dan-shaw @vicroms @ras0219-msft and I discussed this one more time. We do still think http3 is important, and we would be happy to add instructions to portfile.cmake for folks who want to do this with an overlay. However, given that the curl project itself seems uncertain about how this is intended to work, that no other curl distribution (except MacPorts) seems to have a solution that works for their customers, and that attempting to do this forces us to remove features that existing vcpkg customers may be using, we do not wish to add an HTTP3 feature at this time. We will reevaluate this if/when the curl project's stance on this topic changes or better solutions to the quic TLS backend problem. |
I'm going to leave this one open notwithstanding the above because I think this is very similar to what will actually be necessary if the situation changes. |
f8dcda9
to
4a8a795
Compare
4a8a795
to
a43c256
Compare
./vcpkg x-add-version --all
and committing the result.I added this cmake option to curl:
curl/curl#13034