[quictls] create a new port by luncliff · Pull Request #17949 · microsoft/vcpkg

luncliff · 2021-05-15T08:58:35Z

What does your PR fix?

Create a port for https://github.com/quictls/openssl (fork of openssl/openssl)

Support microsoft/msquic#1426.

Which triplets are supported/not supported? Have you updated the CI baseline?

Build check

MinGW, PowerPC, BSD is not in consideration for this port version.

Windows (static only)
- x64-windows
- x64-windows-static
- x64-windows-static-md
- x86-windows
- x86-windows-static
- x86-windows-static-md
- arm64-windows
- arm64-windows-static
- arm64-windows-static-md
UWP (static only)
- x64-uwp
- x86-uwp
- arm64-uwp
- arm-uwp
Darwin
- x64-osx
- arm64-osx
iOS
- arm64-ios
- x64-ios
Linux
- x64-linux
Android
- arm64-android
- arm-android
- x64-android
- x86-android

References

https://github.com/quictls/openssl/tree/OpenSSL_1_1_1k+quic
- ${SOURCE_PATH}/Configuration/README.design
- ${SOURCE_PATH}/Configuration/10-main.conf

And see comments below :D

Import check

curl: import via find_package(OpenSSL REQUIRED)
Replace "openssl" dependency to "quictls" and run vcpkg install curl[openssl] for all triplets above
- Windows
- UWP
- Darwin
- iOS
- Linux
- Android

Q. The port's install collides with openssl. Do I have to put skip in the ci.baseline.txt?

--> The port's build check is skipped. Need to test manually

Does your PR follow the maintainer guide?

Yes.

I read the existing configurations of openssl port and microsoft/msquic. Then reorganized in my style.
I tried to simplify them with comments but additional suggestions will be helpful!

nibanks · 2021-05-15T15:48:39Z

+    SHA512 881639f0bfd83858ce5d28aaf013dc34105cbc9c2fcc040c873c41ca45a0ea3dcb9dfd8e81821b21e92d4b9577f86d11ac010bb1f3746713892969c62d46fda6
+    HEAD_REF openssl-3.0.0-alpha15+quic
+    PATCHES
+        fix-http.patch


Why is this needed? We don't need then when we build via submodule in msquic.

What I intended is supporting SSL selection as a feature of future ms-quic port. It will be like ms-quic[core](with schannel), ms-quic[quictls](with quictls/opnessl), etc.

* remove unused options

NancyLi1013 · 2021-05-19T07:43:52Z

Hi @luncliff
Thanks for your PR.
Can you please help confirm the status on uwp? Does this port support uwp?

luncliff · 2021-05-19T07:50:20Z

Seems like the branch doesn't. 😑 I will update "support" soon. I think I can leave those uwp related platform names in detect_platform script, but it can be confusing for users... Do you think VC-WIN-UWP things need to be removed?

…

On Wed, May 19, 2021, 4:44 PM NancyLi1013 ***@***.***> wrote: Hi @luncliff <https://github.com/luncliff> Thanks for your PR. Can you please help confirm the status on uwp? Does this port support uwp? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17949 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLSYE7WCI6W5CSZUVAUTADTONT47ANCNFSM445V2T4Q> .

NancyLi1013 · 2021-05-19T08:07:46Z

Seems like the branch doesn't. 😑 I will update "support" soon. I think I can leave those uwp related platform names in detect_platform script, but it can be confusing for users... Do you think VC-WIN-UWP things need to be removed?

I noticed that the information from configure-perl-x64-uwp-dbg-out.log:

Configuring OpenSSL version 1.1.1k+quic (0x101010bfL) for VC-WIN64A-UWP
Using os-specific seed configuration

But I'm not familiar with these, so I cannot give you any suggestions. Sorry, I cannot help you. But I suggest to use support field in vcpkg if it doesn't support. If not, please try to fix the problems on uwp.

Q. The port's install collides with openssl. Do I have to put skip in the ci.baseline.txt?

Yes, we should add skip in ci.baseline.txt.

luncliff · 2021-05-19T08:20:45Z

Understood. I will update this within days.

…

On Wed, May 19, 2021, 5:08 PM NancyLi1013 ***@***.***> wrote: Seems like the branch doesn't. 😑 I will update "support" soon. I think I can leave those uwp related platform names in detect_platform script, but it can be confusing for users... Do you think VC-WIN-UWP things need to be removed? I noticed that the information from configure-perl-x64-uwp-dbg-out.log: Configuring OpenSSL version 1.1.1k+quic (0x101010bfL) for VC-WIN64A-UWP Using os-specific seed configuration But I'm not familiar with these, so I cannot give you any suggestions. Sorry, I cannot help you. But I suggest to use support field in vcpkg if it doesn't support. If not, please try to fix the problems on uwp. Q. The port's install collides with openssl. Do I have to put skip in the ci.baseline.txt? Yes, we should add skip in ci.baseline.txt. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17949 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLSYE3JFJM3Q57L3QX6FGTTONWWTANCNFSM445V2T4Q> .

* the branch doesn't provide conf file for UWP * remove unused patch file

luncliff · 2021-06-01T14:30:31Z

@NancyLi1013 @nibanks, I just checked some community triplet builds. I think It's fine now.

nibanks · 2021-06-01T14:43:08Z

cc @ThadHouse

This seems Ok to me, but I am not knowledgeable about vcpkg enough to be able to completely review.

* comment referenced branch for UWP

luncliff · 2021-06-01T16:36:20Z

For future reviewers, this port is skipped in CI. Manual build check is required.

pushd ${VCPKG_ROOT}
    git remote add pr-17949 https://github.com/luncliff/vcpkg
    git checkout pr-17949/port/quictls
popd

cd ${VCPKG_ROOT}
./vcpkg install \
    quictls:arm64-android \
    quictls:arm-android \
    quictls:x64-android \
    quictls:x86-android \
    quictls:x64-osx \
    quictls:arm64-osx \
    quictls:x64-ios \
    quictls:arm64-ios
...

ThadHouse · 2021-06-01T16:42:59Z

UWP should work. We support UWP in MsQuic, and use this branch of quictls, so it shouldn't have any issues. But otherwise looks good to me.

NancyLi1013 · 2021-06-02T02:09:22Z

Have you tested these official triplets on your local? @luncliff
CI will not check the build results since they have been skipped on our CI. So we should keep it work fine on our machine. For community triplets, I noticed you had checked them. As @ThadHouse said above, seems you need to update the changes to support uwp.

@nibanks and @ThadHouse thanks for your help and review.

luncliff · 2021-06-02T02:58:02Z

@NancyLi1013 Some import test of quictls will be done in #18225 (ms-quic). Let me cross-check both project again for UWP.

I'm checking with fine_package(OpenSSL REQUIRED). Feel free to let me know when you or other maintainers think another methods need to be tested

NancyLi1013 · 2021-06-02T06:23:45Z

Thanks for your information. @luncliff

Since all official triplets have been set as skip in ci.baseline.txt, quictls will not be built on these triplets on our CI pipeline.

So even though you have tested the usage of quictls in #18225, it still not be built. Please see
quictls:x64-windows: skip: 0085d7fe644868ba35a0011b7fe70ed861eaeb15 https://dev.azure.com/vcpkg/public/_build/results?buildId=54413&view=logs&j=c7e67a25-2047-5d01-7913-57de0573f534&t=0e07d234-e12f-5893-a7f1-2767a24f64ae

I suggest to test these triplets on your local machine. It would be better to make sure quictls can work fine on all official triplets.

NancyLi1013 · 2021-06-07T09:23:10Z

Have you tested these triplets on your local machine? @luncliff

luncliff · 2021-06-07T09:27:35Z

Build and find_package(Openssl). For all triplets above. I don't know whether there is a way to test QUIC related API

NancyLi1013 · 2021-06-07T09:31:11Z

Build and find_package(Openssl). For all triplets above. I don't know whether there is a way to test QUIC related API

Can you build quictls successfully on your machine?

luncliff · 2021-06-07T09:32:48Z

Can you build quictls successfully on your machine?

Yes, I did. Was there any trouble?

NancyLi1013 · 2021-06-07T10:23:33Z

Can you build quictls successfully on your machine?

Yes, I did. Was there any trouble?

Of course not. Since CI will not check the build status, we must make sure it can build successfully on our local.

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

* update git-tree SHA

strega-nil-ms

Needs some changes in detect_platform! Thanks :)

strega-nil-ms · 2021-06-11T17:09:00Z

+
+elseif(VCPKG_TARGET_IS_LINUX)
+    if(VCPKG_TARGET_ARCHITECTURE MATCHES "arm")
+        set(PLATFORM "linux-armv4")


Is there a reason we're skipping arm64?

No. It's sad I can't check build works with the triplet :(
Added it in e7e0901

strega-nil-ms · 2021-06-11T17:09:17Z

+    elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL "x64")
+        set(PLATFORM "linux-x86_64")
+    else()
+        set(PLATFORM "linux-generic32")


this feels... wrong, what is linux-generic32? Shouldn't this check for STREQUAL "x86"?

Fixed in e7e0901. Changed the value to linux-x86 and checks VCPKG_TARGET_ARCHITECTURE.

strega-nil-ms · 2021-06-11T17:10:46Z

@@ -0,0 +1,86 @@
+if(VCPKG_TARGET_IS_ANDROID)
+    # ${SOURCE_PATH}/Configuration/15-android.conf
+    if(VCPKG_TARGET_ARCHITECTURE MATCHES "arm64")


In general, also, this should use STREQUAL

I habitually used the expression. Replaced them to STREQUAL in e7e0901.

strega-nil-ms · 2021-06-11T17:11:25Z

+    endif()
+
+elseif(VCPKG_TARGET_IS_FREEBSD OR VCPKG_TARGET_IS_OPENBSD)
+    set(PLATFORM "BSD-generic64")


please make sure that this only sets up x64 on x64.

Added explicit check of VCPKG_TARGET_ARCHITECTURE like the other cases in e7e0901.

strega-nil-ms · 2021-06-11T17:12:00Z

+
+elseif(VCPKG_TARGET_IS_MINGW)
+    if(VCPKG_TARGET_ARCHITECTURE MATCHES "64")
+        set(PLATFORM "mingw64")


probably wants to be x86_64 or x86, not doing this matching.

That was a mistype. Nice point 👍 Fixed in e7e0901.

strega-nil-ms · 2021-06-11T17:13:27Z

+    elseif(VCPKG_TARGET_ARCHITECTURE STREQUAL "arm64")
+        set(PLATFORM "VC-WIN64-ARM")
+    else()
+        message(FATAL_ERROR "Unknown UWP target architecture: ${VCPKG_TARGET_ARCHITECTURE}")


We already have fatal errors at the bottom of this file.

I wanted to give more hint that it's a issue for the platform.
Made fallthrough to the bottom check.

* Use STREQUAL instead of MATCHES * Add some dropped PLATFORM cases * Always check VCPKG_TARGET_ARCHITECTURE instead of `else()` * update git-tree SHA

ras0219-msft · 2021-06-16T00:03:40Z

Because we're providing zero testing coverage of this in CI (since it conflicts with the blessed official openssl), it seems like this would be more appropriately hosted in a community registry.

See our pinned discussion of community registries: #17161

nibanks · 2021-06-16T00:24:46Z

Because we're providing zero testing coverage of this in CI (since it conflicts with the blessed official openssl), it seems like this would be more appropriately hosted in a community registry.

See our pinned discussion of community registries: #17161

@ras0219-msft Is there a way to not conflict with openssl. BTW, this fork of openssl is blessed for Microsoft usage, explicitly used by MsQuic.

luncliff · 2021-06-16T02:24:19Z

Because we're providing zero testing coverage of this in CI (since it conflicts with the blessed official openssl), it seems like this would be more appropriately hosted in a community registry.

See our pinned discussion of community registries: #17161

Thanks for review. I already have my registry in #17161, so that's an option for me to support quictls and ms-quic in it...
But at a glance this port looks like a similar case of boringssl(#8455). And I did reference its portfile.

In #18225 ac64cb5, I found the port can be built in CI environment. So if there is a way to check without harming openssl related check, I would like to try for this.

ras0219-msft · 2021-06-17T20:30:45Z

BTW, this fork of openssl is blessed for Microsoft usage, explicitly used by MsQuic.

We would love to support it, but we cannot fracture the entire catalog over it.

We do not want to add libraries to the curated catalog that we cannot build. This is a maintenance nightmare, because we have no way to validate that updates do not break everything.
We build a consistent universe -- this means that if one package sees include/openssl.h from openssl, another package must not see include/openssl.h from my-custom-ssl.

Is there a way to not conflict with openssl

Some scattered points:

If MsQuic is consuming only the official openssl interface, we could include msquic built on openssl in the main catalog. quictls can be hosted in a third party catalog and it's very easy to choose quictls to replace all uses of openssl via an overlay port.
If quictls had a build mode that augmented the blessed copy of openssl instead of replacing it (such as not building any symbols that openssl provides and not providing openssl.h), then we would be happy to include that version in the public catalog. Note that just deleting openssl.h is insufficient, because static linking would still result in One Definition Rule (ODR) violations and Undefined Behavior (UB, nasal demons, very bad!).

But at a glance this port looks like a similar case of boringssl(#8455).

BoringSSL was added to the catalog long before we had third party registry support -- today, it would be better served by a third-party registry with the exact arguments above.

nibanks · 2021-06-17T20:47:55Z

@ras0219-msft there is a long story around why we had to create the quictls fork, but essentially this fork is the base OpenSSL + QUIC functionality. I don't want to onboard this without build coverage. My question is if there is a way to still onboard this with coverage.

The goal isn't to have quictls replace usage for anything that wants openssl. The goal is to make it available for anyone who expressly wants quictls.

ras0219-msft · 2021-06-24T21:08:57Z

After looking further into quictls, it's possible we could express it as a feature in the existing openssl port (openssl[quictls]). When chosen by the user's dependency tree (off by default), this would activate using the quictls fork of the openssl codebase.

However, this still wouldn't fix the issue of build coverage -- in CI we would still need to build and deploy the upstream copy of openssl, which means the quictls feature would not be selected and not covered. From a pure build perspective, we could add a test port that doesn't install any files but does build quictls; however test ports are not available for customers on the vcpkg cli (and it wouldn't help anyway -- the port installs no files).

Additionally, even if we solve the problem of coverage for quictls, there's no way we can add build coverage for msquic -- by design, the openssl.h we will install and make available in CI must be the vanilla upstream openssl.h, not a fork.

There is a potentially extreme option of vendoring quictls inside msquic's port and only supporting dynamic linkage (static linking quictls to hide the inner openssl symbols). This means users would see openssl.h and link against the upstream openssl, but could also link against msquic.

luncliff · 2021-06-25T02:53:01Z

There is a potentially extreme option of vendoring quictls inside msquic's port and only supporting dynamic linkage (static linking quictls to hide the inner openssl symbols). This means users would see openssl.h and link against the upstream openssl, but could also link against msquic.

Thanks for check @ras0219-msft. I wanted this port can be separated with ms-quic, but if so I will nest quictls inside ms-quic.

Let me keep this PR open until this weekend. I will sync this with #18225 and then close this one.

* bring work of microsoft/vcpkg#17949

luncliff added 3 commits May 15, 2021 17:43

[quictls] create a new port

06220d5

[quictls] update baseline and port SHA

8e86e4a

[quictls] remove support constraint

352eae1

luncliff mentioned this pull request May 15, 2021

Support vcpkg microsoft/msquic#1426

Closed

nibanks reviewed May 15, 2021

View reviewed changes

Comment thread ports/quictls/portfile.cmake Outdated

nibanks reviewed May 15, 2021

View reviewed changes

Comment thread ports/quictls/portfile.cmake

nibanks reviewed May 15, 2021

View reviewed changes

Comment thread versions/baseline.json Outdated

[quictls] change code branch

1220cd5

* remove unused options

PhoebeHui assigned NancyLi1013 May 18, 2021

NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label May 18, 2021

NancyLi1013 added the requires:author-response label May 19, 2021

luncliff added 3 commits May 23, 2021 19:10

[quictls] disable UWP build

c829902

* the branch doesn't provide conf file for UWP * remove unused patch file

[quictls] update ci.baseline.txt

30ffc9d

[quictls] fix android build for static linkage

99e85ce

nibanks reviewed Jun 1, 2021

View reviewed changes

Comment thread ports/quictls/detect_platform.cmake Outdated

nibanks approved these changes Jun 1, 2021

View reviewed changes

[quictls] fix URL in detect_platform script

c6a995c

* comment referenced branch for UWP

luncliff mentioned this pull request Jun 1, 2021

[ms-quic] create a new port #18225

Merged

8 tasks

NancyLi1013 suggested changes Jun 7, 2021

View reviewed changes

Comment thread ports/quictls/detect_platform.cmake Outdated

Comment thread scripts/ci.baseline.txt

luncliff and others added 2 commits June 7, 2021 20:09

Update scripts/ci.baseline.txt

a9f6cc6

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

[quictls] remove comments in cmake script

cdd1597

* update git-tree SHA

NancyLi1013 approved these changes Jun 8, 2021

View reviewed changes

NancyLi1013 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed requires:author-response labels Jun 8, 2021

strega-nil-ms suggested changes Jun 11, 2021

View reviewed changes

[quictls] update detect_platform script

e7e0901

* Use STREQUAL instead of MATCHES * Add some dropped PLATFORM cases * Always check VCPKG_TARGET_ARCHITECTURE instead of `else()` * update git-tree SHA

luncliff requested a review from strega-nil-ms June 15, 2021 03:02

NancyLi1013 removed the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jun 25, 2021

luncliff closed this Jun 27, 2021

luncliff deleted the port/quictls branch September 10, 2021 16:02

luncliff added a commit to luncliff/vcpkg-registry that referenced this pull request Sep 21, 2021

[quictls] port update

0e6e1b9

* bring work of microsoft/vcpkg#17949

luncliff added a commit to luncliff/vcpkg-registry that referenced this pull request Sep 25, 2021

[quictls] port update

bc0f4ff

* bring work of microsoft/vcpkg#17949

luncliff mentioned this pull request Sep 29, 2021

[openssl] port update to OpenSSL 3.0.0 #20428

Closed

4 tasks

This was referenced Aug 1, 2023

[curl] Curl port is missing multiple features like ZSTD #32109

Closed

[ms-quic]:x64-windows build failure #29459

Closed

Conversation

luncliff commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Build check

Import check

Does your PR follow the maintainer guide?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NancyLi1013 commented May 19, 2021

Uh oh!

luncliff commented May 19, 2021 via email

Uh oh!

NancyLi1013 commented May 19, 2021

Uh oh!

luncliff commented May 19, 2021 via email

Uh oh!

luncliff commented Jun 1, 2021

Uh oh!

nibanks commented Jun 1, 2021

Uh oh!

Uh oh!

luncliff commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThadHouse commented Jun 1, 2021

Uh oh!

NancyLi1013 commented Jun 2, 2021

Uh oh!

luncliff commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NancyLi1013 commented Jun 2, 2021

Uh oh!

Uh oh!

Uh oh!

NancyLi1013 commented Jun 7, 2021

Uh oh!

luncliff commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NancyLi1013 commented Jun 7, 2021

Uh oh!

luncliff commented Jun 7, 2021

Uh oh!

NancyLi1013 commented Jun 7, 2021

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

luncliff commented May 15, 2021 •

edited

Loading

luncliff commented Jun 1, 2021 •

edited

Loading

luncliff commented Jun 2, 2021 •

edited

Loading

luncliff commented Jun 7, 2021 •

edited

Loading

ras0219-msft commented Jun 16, 2021 •

edited

Loading

nibanks commented Jun 16, 2021 •

edited

Loading

luncliff commented Jun 16, 2021 •

edited

Loading