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.

Sign up for GitHub

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

Jump to bottom

[curl] ngtcp2 http3 curl features #40219

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

[curl] ngtcp2 http3 curl features #40219

wants to merge 1 commit into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Aug 1, 2024 •
edited
Loading

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

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
Contributor Author

talregev commented Aug 2, 2024 •
edited
Loading

@bradh352 Thank you for your fix!

@dg0yt
Copy link
Contributor

dg0yt commented Aug 2, 2024

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

@talregev
Copy link
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
Contributor Author

talregev commented Aug 2, 2024 •
edited
Loading

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

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

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@talregev
Copy link
Contributor Author

talregev commented Aug 2, 2024 •
edited
Loading

@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
Contributor Author

talregev commented Aug 2, 2024

This PR depends on #40224

@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
Contributor Author

talregev commented Aug 6, 2024

@JonLiu1993 This PR is ready for review.

dg0yt
dg0yt reviewed Aug 7, 2024
View reviewed changes
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
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
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
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
Contributor Author

@talregev talregev Aug 7, 2024 •
edited
Loading

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

ports/curl/portfile.cmake Outdated
@@ -54,6 +56,28 @@ endif()

set(OPTIONS "")

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

ports/curl/portfile.cmake Outdated Show resolved Hide resolved
@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
Contributor Author

talregev commented Aug 8, 2024

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

@talregev
Copy link
Contributor Author

talregev commented Aug 9, 2024 •
edited
Loading

Note to the person in charge: Before merge, this has to undergo an extra check that it is acceptable to add more feature combinations which will break the build.

What is your suggestion?
Are you in favor to add http3 feature?

@dg0yt
Copy link
Contributor

dg0yt commented Aug 11, 2024

I'm in favor of a robust feature interface.

The question is again if the team is happy with port curl failing to build for any combination of http2, http3-gnutls and http3-wolfssl, plus for any combination of http3-* with multi-ssl.

@talregev
Copy link
Contributor Author

talregev commented Aug 11, 2024 •
edited
Loading

http3 feature in curl is by definition without MutliSSL.
This is the reason you can only have one of the kind http3 without any other ssl.
Because of this curl design this feature, I create errors msgs that explain for the user how to add these features.

Other features is curl already breaks when install together, like openssl and wolfssl.

If you have better solution or other suggestion to add http3 feature to curl vcpkg, I will happy to hear your thoughts.

@data-queue data-queue added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 13, 2024
@JonLiu1993 JonLiu1993 marked this pull request as draft August 16, 2024 08:58
@JonLiu1993
Copy link
Member

@talregev, please fix Conflicting files thanks.

@talregev talregev marked this pull request as ready for review August 17, 2024 08:02
@talregev
Copy link
Contributor Author

@JonLiu1993
PR is ready for review.

@dg0yt dg0yt mentioned this pull request Aug 17, 2024
Merged
7 tasks
JonLiu1993
JonLiu1993 previously approved these changes Aug 19, 2024
View reviewed changes
@vicroms
Copy link
Member

vicroms commented Aug 29, 2024

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

@BillyONeal
Copy link
Member

BillyONeal commented Aug 29, 2024 •
edited
Loading

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

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

BillyONeal commented Sep 11, 2024 •
edited
Loading

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:reviewed Pull Request changes follow basic guidelines 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
Contributor Author

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

Thank you in advance! 🙏🏻

@BillyONeal
Copy link
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
Contributor Author

talregev commented Sep 13, 2024 •
edited
Loading

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

@BillyONeal
Copy link
Member

BillyONeal commented Sep 17, 2024 •
edited
Loading

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@JonLiu1993 JonLiu1993 JonLiu1993 left review comments

Assignees

@JonLiu1993 JonLiu1993

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@talregev @bradh352 @dg0yt @JonLiu1993 @vicroms @BillyONeal @data-queue