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

[curl] Add http3 feature on linux only #37146

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Mar 5, 2024

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

I added this cmake option to curl:
curl/curl#13034

Comment on lines 51 to 58
"supports": "(uwp | !windows) & !(osx | ios) & !mingw",
"dependencies": [
{
"name": "curl",
"default-features": false,
"features": [
"ssl"
]
Copy link
Contributor

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.

Copy link
Contributor Author

@talregev talregev Mar 5, 2024

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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

@talregev talregev Mar 5, 2024

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.

@talregev talregev force-pushed the TalR/curl_http3 branch 2 times, most recently from 943acf0 to 30ca630 Compare March 5, 2024 18:42
@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Mar 6, 2024
@talregev
Copy link
Contributor Author

talregev commented Mar 7, 2024

FrankXie05
FrankXie05 previously approved these changes Mar 7, 2024
@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Mar 7, 2024
@FrankXie05
Copy link
Contributor

The feature http3 has been tested successfully on x64-linux.

@@ -45,6 +46,24 @@
"nghttp2"
]
},
"http3": {
"description": "HTTP3 support",
"supports": "(uwp | !windows) & !(osx | ios) & !mingw",
Copy link
Contributor

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?

Copy link
Contributor Author

@talregev talregev Mar 7, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@talregev talregev Mar 7, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@talregev talregev Mar 7, 2024

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.

Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dg0yt
Copy link
Contributor

dg0yt commented Mar 7, 2024

To core team: Review the feature interface more thoroughly than usual before final merge.

  • Port curl is unusual in giving the user the choice between different SSL backends.
  • The new feature needs one particular backend, openssl.
  • The new feature should work on more platforms than declared here, but then the user would need to opt-out from the default (system-provided) backend on some platforms.
  • There also alternative backends such as mbedtls which may or may not clash with the use of openssl for http3.
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 8, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Mar 8, 2024

To make it clear: I'm in favor of adding the feature. However I would prefer a less restrictive approach towards supports here. It doesn't worsen the situation: Some features cannot be used together.

@talregev
Copy link
Contributor Author

talregev commented Mar 8, 2024

To make it clear: I'm in favor of adding the feature. However I would prefer a less restrictive approach towards supports here. It doesn't worsen the situation: Some features cannot be used together.

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.
But this is not for this PR.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 10, 2024

Port curl is unusual in giving the user the choice between different SSL backends.

FTR curl still allows to select the actual backend at runtime. This type choice is on the maintainer's guide allow list.

@talregev
Copy link
Contributor Author

Port curl is unusual in giving the user the choice between different SSL backends.

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.

@talregev
Copy link
Contributor Author

@vicroms Any news?

@talregev
Copy link
Contributor Author

@BillyONeal I fix the conflicts.
Please review again.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 14, 2024

I think it might be appropriate

  • to name the feature experimental-http3
  • to turn it into a no-op with warning when mutiples backends are selected.

i.e you can use it if you pay attention to the limitations.

@talregev talregev marked this pull request as ready for review March 21, 2024 21:13
@talregev
Copy link
Contributor Author

@FrankXie05 @Cheney-W Can you put the review tag, that the vcpkg team will discuss this PR again?
Thank you!

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Mar 22, 2024
@talregev talregev changed the title [curl] Add http3 feature Mar 22, 2024
@BillyONeal
Copy link
Member

BillyONeal commented Mar 22, 2024

We discussed this as a team yesterday and wrote down the following buckets:

  • We think making HTTP3 easy (a feature rather than an overlay) is more important than Schannel and Secure Transport (and wolfSSL and mbedTLS). (What we unknowningly asked for last week)
  • We think making HTTP3 easy (a feature rather than an overlay) is more important than wolfSSL and mbedTLS, and only available on non-Schannel and Secure Transport platforms. (Contributor's original PR)
    @BillyONeal
  • We want to wait until HTTP3 is no longer experimental and not change anything about TLS backend selection.
    @JavierMatosD
    @ras0219-msft

@dan-shaw abstains

Open questions:

  • What do the linuxes do with the in-box curl today?
  • Is HTTP3 overall experimental or only particular backends? (The documentation suggests that ngtcp2 is not experimental anymore?)
    * Is this change using ngtcp2? no
@BillyONeal
Copy link
Member

@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
Copy link
Member

What do the linuxes do with the in-box curl today?

  • Windows: curl: option --http3: the installed libcurl version doesn't support this
  • Ubuntu 22.04: curl: option --http3: the installed libcurl version doesn't support this
  • Alpine: curl: option --http3: the installed libcurl version doesn't support this
  • Fedora 41: curl: option --http3: the installed libcurl version doesn't support this
@talregev
Copy link
Contributor Author

talregev commented Mar 23, 2024

@BillyONeal Curl with http3 with openssl + nghttp3 will be less time experimental if people will install it and test it and report bugs.
So let only Linux user install it while it not break the port, and when it will be no longer experimental, we can do as you write.

@talregev
Copy link
Contributor Author

We discussed this as a team yesterday and wrote down the following buckets:

  • We think making HTTP3 easy (a feature rather than an overlay) is more important than Schannel and Secure Transport (and wolfSSL and mbedTLS). (What we unknowningly asked for last week)
  • We think making HTTP3 easy (a feature rather than an overlay) is more important than wolfSSL and mbedTLS, and only available on non-Schannel and Secure Transport platforms. (Contributor's original PR)
    @BillyONeal
  • We want to wait until HTTP3 is no longer experimental and not change anything about TLS backend selection.
    @JavierMatosD
    @ras0219-msft

@BillyONeal This PR is answer both point 2 and 3.
The majority vote of vcpkg team.

@BillyONeal
Copy link
Member

@BillyONeal This PR is answer both point 2 and 3. The majority vote of vcpkg team.

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 :(

@talregev
Copy link
Contributor Author

@BillyONeal From the page that you send, it seems the distro will choose the way I choose bring http3 to vcpkg:

curl’s support for OpenSSL’s QUIC (together with nghttp3) and OpenSSL’s upcoming improvements in that area (coming in OpenSSL 3.3) are for many users perhaps the most viable route to HTTP/3.

Also soon will be openssl 3.3 They already release the alpha.

@talregev
Copy link
Contributor Author

talregev commented Mar 27, 2024

@BillyONeal As curl release new version 8.7.1
Daniel made a YouTube about this release (like any other release on curl),
And he mention that on release 8.8.0 it might that openssl + quic will be non experimental.
So I guess we will wait.
https://youtu.be/x6XbvwB_b6Q?t=1805

@BillyONeal
Copy link
Member

@BillyONeal From the page that you send, it seems the distro will choose the way I choose bring http3 to vcpkg:

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

@talregev
Copy link
Contributor Author

talregev commented Mar 27, 2024

@BillyONeal From the page that you send, it seems the distro will choose the way I choose bring http3 to vcpkg:

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?
Wait until openssl + nghttp3 will be no experiment it probably will be in next curl version as I posted here.

@BillyONeal
Copy link
Member

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

@BillyONeal
Copy link
Member

BillyONeal commented Mar 28, 2024

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

@BillyONeal BillyONeal removed info:reviewed Pull Request changes follow basic guidelines requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Mar 29, 2024
@BillyONeal
Copy link
Member

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.

@talregev talregev marked this pull request as draft June 14, 2024 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
5 participants