Skip to content

[vcpkg-ci-curl] Add openssl to curl tests#40512

Merged
vicroms merged 1 commit intomicrosoft:masterfrom
talregev:TalR/curl_ci_openssl
Aug 29, 2024
Merged

[vcpkg-ci-curl] Add openssl to curl tests#40512
vicroms merged 1 commit intomicrosoft:masterfrom
talregev:TalR/curl_ci_openssl

Conversation

@talregev
Copy link
Copy Markdown
Contributor

@talregev talregev commented Aug 17, 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.

@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Aug 17, 2024

NB:

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 17, 2024

openssl port can be with all platforms.
we cannot test http3 even if ci test will be single ssl with default ssl, because non of the os have gnutls nor wolfssl as the default ssl. That why when adding http3 feature with default ssl it will be multissl and it will break the build.

The tests is working with multissl feature, when we add them we discovered problems in curl and other ports and we fix them.

As I see how fast the ci was finished, I think openssl already added to all os in the tests.

@talregev talregev marked this pull request as ready for review August 17, 2024 17:31
@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Aug 17, 2024

As I see how fast the ci was finished, I think openssl already added to all os in the tests.

Based on what I wrote: It is a default feature for the linux and android triplets. And for windows, it is explicitly pulled in by some ports (git grep -A5 curl ports/*/vcpkg.json | grep -B5 openssl for candidates, e.g. pulsar-client-cpp.)

@Cheney-W Cheney-W added category:infrastructure Pertaining to the CI/Testing infrastrucutre info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Aug 19, 2024
@Cheney-W
Copy link
Copy Markdown
Contributor

@vicroms Could you please help review this PR?

@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 19, 2024
@vicroms
Copy link
Copy Markdown
Member

vicroms commented Aug 29, 2024

Seems redundant since regular CI already builds curl with all these backends on all platforms but uwp, but I guess that makes it inoffensive enough to merge.

If we want this test port to instead test the case of SingleSSL with http3 support, then we can accept a PR for that change in the future (and most likely that would start a different discussion too).

@vicroms vicroms merged commit 8e22917 into microsoft:master Aug 29, 2024
@talregev talregev deleted the TalR/curl_ci_openssl branch August 29, 2024 08:01
@dg0yt
Copy link
Copy Markdown
Contributor

dg0yt commented Aug 30, 2024

I would like to see the original intent of this port being restored: testing with the default backend and not pulling in additional backends.

The additional backends here are not even guarded by features, so we can no longer use the test port manually for single-backend tests.

@anders-wind anders-wind mentioned this pull request Sep 2, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:infrastructure Pertaining to the CI/Testing infrastrucutre 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants