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] Update #18971

Merged
merged 18 commits into from
Aug 12, 2021
Merged

[curl] Update #18971

merged 18 commits into from
Aug 12, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jul 16, 2021

Describe the pull request

@dg0yt dg0yt mentioned this pull request Jul 16, 2021
1 task
@NancyLi1013 NancyLi1013 self-assigned this Jul 16, 2021
@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Jul 16, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 16, 2021

Curl now also supports "WinIDN" so this can be used for Windows instead of libidn2.
I will try to make IDN support work for mingw, too.
Libidn2 needs a review of how it publishes its icon and unistring dependencies.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 18, 2021

I won't care much with the osx gdal issue before #17698 is merged.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 20, 2021

Needs #19002 before submitting commit which re-adds proper libidn2 support.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 22, 2021

Waiting for merge of
#19002 for libidn2 support
#17698 for osx gdal issue

@NancyLi1013 NancyLi1013 added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jul 22, 2021
@dg0yt dg0yt changed the title [curl] Update WIP [curl] Update Jul 27, 2021
@NancyLi1013 NancyLi1013 removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jul 27, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 27, 2021

I'm looking into upgrading to curl 0.78.0, and I want to get rid of the nghttp2 patch by #19163.

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 4, 2021

failed on x64-windows-static with the following errors

@NancyLi1013 Can you please verify that you are you using the latest version of port nghttp2? The static build should define NGHTTP2_STATICLIB at the end of include/nghttp2/nghttp2ver.h.

In CI, curl[core,http2,non-http,openssl,schannel,ssl,sspi,winssl]:x64-windows-static-md built just fine:
https://dev.azure.com/vcpkg/c1ee48cb-0df2-4ab3-8384-b1df5a79fe53/_apis/build/builds/57226/logs/117
(The agent for x64-windows-static died too early, but it reached curlpp so I assume there is no issue with curl[http2].)

@dg0yt dg0yt marked this pull request as draft August 4, 2021 20:59
@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 4, 2021

Reset to draft for the following changes:

What is wrong non-http:

  • It is a default feature, making it hard to opt out. (Every port which depends on default features of curl pulls in non-http.)
    This wouldn't be a problem but
  • It enables auto-discovery of libraries, so it may pick up system libraries, e.g. LDAP.

To ensure proper binary packages, we must disable each feature which has additional dependencies unless

  • it is enabled by a non-default feature, and
  • that feature adds proper dependencies

An additional feature "system-libraries" may allow the discovery of non-vcpkg libraries if desired. (Prototype: port gdal.)

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 5, 2021

* review `non-http` feature

I guess I checked this before: I don't see what other system libraries would be picked up by curl ATM. There are many default-OFF switches so this should all be safe.

@dg0yt dg0yt marked this pull request as ready for review August 5, 2021 06:31
@NancyLi1013
Copy link
Contributor

Have you fixed the problem on feature http2?

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 5, 2021

Have you fixed the problem on feature http2?

I asked you to double-check that you tested the feature with the latest version of port nghttp2. May I kindly repeat my request?
CI didn't find any problem when I did a test build with the http2 enabled, as I reported in another comment here.

@NancyLi1013
Copy link
Contributor

Have you fixed the problem on feature http2?

I asked you to double-check that you tested the feature with the latest version of port nghttp2. May I kindly repeat my request?
CI didn't find any problem when I did a test build with the http2 enabled, as I reported in another comment here.

I tested this feature based on your branch. nghttp2 is 1.42.0, the latest version in vcpkg master branch is 1.44.0. I think you need to merge master to your branch.

@dg0yt
Copy link
Contributor Author

dg0yt commented Aug 6, 2021

I think you need to merge master to your branch.

Oh, you test the HEAD of this branch without master. That's different from CI, of course. I did the merge now. But you could do this as well when testing. (Or should do it that way? It you test just the branch, it is different from what the user gets after the PR is merged.)

@NancyLi1013
Copy link
Contributor

I think you need to merge master to your branch.

Oh, you test the HEAD of this branch without master. That's different from CI, of course. I did the merge now. But you could do this as well when testing. (Or should do it that way? It you test just the branch, it is different from what the user gets after the PR is merged.)

I have update to the latest changes to test the feature http2. It can be built successfully on x64-windows-static now.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Aug 6, 2021
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your updates and improvements @dg0yt.

@JackBoosY
Copy link
Contributor

Fixes #14030.

@JackBoosY JackBoosY linked an issue Aug 9, 2021 that may be closed by this pull request
@BillyONeal BillyONeal merged commit 6a9ecfd into microsoft:master Aug 12, 2021
@BillyONeal
Copy link
Member

Thanks for your hard work!

@dg0yt dg0yt deleted the curl-update branch March 31, 2022 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to compile curl with LDAPS
5 participants