Skip to content

[curl] ngtcp2 http3 curl features#40219

Closed
talregev wants to merge 1 commit intomicrosoft:masterfrom
talregev:TalR/curl_ngtcp2_http3
Closed

[curl] ngtcp2 http3 curl features#40219
talregev wants to merge 1 commit intomicrosoft:masterfrom
talregev:TalR/curl_ngtcp2_http3

Conversation

@talregev
Copy link
Copy Markdown
Contributor

@talregev talregev commented Aug 1, 2024

Add ngtcp2 http3 curl features

These features is not experimental in http3 curl:
https://curl.se/docs/http3.html

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

@talregev talregev force-pushed the TalR/curl_ngtcp2_http3 branch 6 times, most recently from 73e409f to 657223b Compare August 1, 2024 21:16
@talregev talregev changed the title [curl] ngtcp2 http3 curl features [curl, vcpkg-ci-curl] ngtcp2 http3 curl features, move c-ares feature to !uwp platform Aug 1, 2024
@bradh352
Copy link
Copy Markdown

bradh352 commented Aug 1, 2024

c-ares should be fixed in c-ares/c-ares@3fd5925

@JonLiu1993 JonLiu1993 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Aug 2, 2024
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 2, 2024

@bradh352 Thank you for your fix!

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Aug 2, 2024

@bradh352 Thanks for the quick action. I did the vcpkg PR in #40224.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 2, 2024

@dg0yt I am trying to debug on my local windows why cmake fail.

[527/528] C:\Windows\system32\cmd.exe /C "cd /D D:\b\cmake\x64-windows-dbg && D:\b\cmake\x64-windows-dbg\bin\cmake.exe -P cmake_install.cmake"
FAILED: CMakeFiles/install.util 
C:\Windows\system32\cmd.exe /C "cd /D D:\b\cmake\x64-windows-dbg && D:\b\cmake\x64-windows-dbg\bin\cmake.exe -P cmake_install.cmake"
ninja: build stopped: subcommand failed.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 2, 2024

@dg0yt I cannot reproduce the error in my local machine. Maybe is a CI thing? Maybe permissions?

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Aug 2, 2024

@dg0yt I cannot reproduce the error in my local machine. Maybe is a CI thing? Maybe permissions?

It is unclear because there is no error message. It might help to inspect the file.
(Sorry, I don't have MSVC. I would have to do this check with vcpkg CI.)

@JonLiu1993
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 2, 2024

@JonLiu1993 Can you stop the ci for this PR?
We reproduce the error on #40224

@talregev talregev marked this pull request as draft August 2, 2024 07:34
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 2, 2024

This PR depends on #40224

@talregev talregev force-pushed the TalR/curl_ngtcp2_http3 branch from 657223b to 8e37981 Compare August 2, 2024 09:25
@talregev talregev changed the title [curl, vcpkg-ci-curl] ngtcp2 http3 curl features, move c-ares feature to !uwp platform [curl] ngtcp2 http3 curl features Aug 2, 2024
@talregev talregev marked this pull request as ready for review August 6, 2024 08:25
@talregev talregev marked this pull request as draft August 6, 2024 08:26
@talregev talregev marked this pull request as ready for review August 6, 2024 13:29
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 6, 2024

@JonLiu1993 This PR is ready for review.

Comment thread ports/curl/math.patch Outdated
set(USE_NGTCP2 ON)
include_directories(${NGTCP2_INCLUDE_DIRS})
- list(APPEND CURL_LIBS ${NGTCP2_LIBRARIES})
+ list(APPEND CURL_LIBS ${NGTCP2_LIBRARIES} ${MATH_LIBRARY})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to link m with the absolute path. You can use find_library to check for it, but just link m.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok. I already submit this patch to upstream and they merge it.
So next version of curl, you will need to patch it again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And the other issue is that fix belongs to the wolfssl cmake package.
curl/curl#14343 (comment)

Copy link
Copy Markdown
Contributor Author

@talregev talregev Aug 7, 2024

Choose a reason for hiding this comment

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

I think it better to create a PR and fix the way you think it is, instead just tell them a comment on a closed PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding a note close to the issue is just the quick start. Creating good PRs is done separately.

Comment thread ports/curl/portfile.cmake Outdated

if(FEATURES MATCHES "http3-(gnutls|wolfssl)")
if("http3-gnutls" IN_LIST FEATURES AND "http3-wolfssl" IN_LIST FEATURES)
message(FATAL_ERROR "Cannot have both features of http3 (http3-gnutls and http3-wolfssl). Choose one")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So these features implement alternatives.

Basically curl wouldn't need two http3 features because the choice of ssl backend has already been made with other features. But you want to have a dependency on ngtcp2 with the right ssl feature enabled.

Comment thread ports/curl/portfile.cmake Outdated
@talregev talregev force-pushed the TalR/curl_ngtcp2_http3 branch 2 times, most recently from 1496aa2 to 85363e5 Compare August 7, 2024 05:43
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 8, 2024

@JonLiu1993 I answered all the points for @dg0yt .
Please review my PR

@talregev talregev force-pushed the TalR/curl_ngtcp2_http3 branch from b89c1ff to 82867ef Compare August 17, 2024 08:01
@talregev talregev requested a review from JonLiu1993 August 17, 2024 08:02
@talregev talregev marked this pull request as ready for review August 17, 2024 08:02
@talregev
Copy link
Copy Markdown
Contributor Author

@JonLiu1993
PR is ready for review.

JonLiu1993
JonLiu1993 previously approved these changes Aug 19, 2024
@vicroms
Copy link
Copy Markdown
Member

vicroms commented Aug 29, 2024

We'll be discussing this one in the next meeting.

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Aug 29, 2024

Related / new information from last we looked: https://samueloph.dev/blog/debian-curl-now-supports-http3/

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Aug 30, 2024

Related / new information from last we looked: https://samueloph.dev/blog/debian-curl-now-supports-http3/

Unfortuately, the Debian situation can't be easily serve as a guide for vcpkg. (Binary packages, alternative packages, GPL, schannel.)

@BillyONeal
Copy link
Copy Markdown
Member

Related / new information from last we looked: https://samueloph.dev/blog/debian-curl-now-supports-http3/

Unfortuately, the Debian situation can't be easily serve as a guide for vcpkg. (Binary packages, alternative packages, GPL, schannel.)

I agree, we certainly wouldn't be able to do this for Windows. But we at least aren't going to be trailblazers on doing this.

I'm still mildly pessimistic about being able to ship this as is but I haven't truly looked into the details. In particular, I'm worried about vcpkg customers out there who might be talking to OpenSSL in their process with the expectation of that affecting libcurl, that would break if we changed the default backend.

Crucially, the system's configured TLS root certificates need to be respected at a bare minimum; this is what forces SChannel on Windows.

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Sep 11, 2024

Unfortuately, the Debian situation can't be easily serve as a guide for vcpkg. (Binary packages, alternative packages, GPL, schannel.)

Yeah, this looks to be the case. The thing that changed in Debian is what curl the command line utility uses -- it now uses libcurl-gnutls.so as its backend. libcurl.so remains the OpenSSL flavor. As vcpkg models the library one rather than the command line utility I still think people would be angry if we changed this, sadly.

I'm going to ask other maintainers for more thoughts given this information.

@BillyONeal BillyONeal removed the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Sep 11, 2024
@talregev talregev force-pushed the TalR/curl_ngtcp2_http3 branch 2 times, most recently from 1373a36 to 8fecd20 Compare September 12, 2024 17:53
@talregev
Copy link
Copy Markdown
Contributor Author

@BillyONeal
Can I know the result of the vcpkg team review?

Thank you in advance! 🙏🏻

@BillyONeal
Copy link
Copy Markdown
Member

I explained the above and @AugP @vicroms @JavierMatosD @data-queue and @ras0219-msft seem convinced that Debian unfortunately can't serve as a blueprint for us here, given that we model libcurl.so Debian did not change, not /bin/curl which Debian changed.

We would still be extremely happy with instructions in the port describing how to make an overlay-port that has these effects, but we don't think changing the default TLS implementation from OpenSSL on the POSIX platforms is a change we can safely make at this time. :(

@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 12, 2024
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Sep 13, 2024

Thank you for the details answer.
How my change here change the default ssl?.

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Sep 17, 2024

Thank you for the details answer. How my change here change the default ssl?.

It breaks multi-TLS meaning we would have to make requesting one of the other TLS backends require making an overlay-port under https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-use-features-to-implement-alternatives

(I'm not closing the PR though because this area is still kind of in a state of flux)

@JavierMatosD JavierMatosD marked this pull request as draft October 8, 2024 22:10
@BillyONeal
Copy link
Copy Markdown
Member

Is this replaced by #46459 ?

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 8, 2025

Yes.

@talregev talregev closed this Aug 8, 2025
@talregev talregev deleted the TalR/curl_ngtcp2_http3 branch August 8, 2025 20:48
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants