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

GHA: add MSVC UWP job, expand jobs with more options #14077

Closed
wants to merge 6 commits into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Jul 1, 2024

  • Add more libs from vcpkg to test more cases in curl
  • Add uwp case. I tried first with the original mechanism, but it fail to build the uwp. It should handle by vcpkg build system.
  • Add more time to timout. vcpkg need to build more time with more libs, more tests take more time.
  • Enable WinIDN in a job. Exclude failing tests.

@github-actions github-actions bot added Windows Windows-specific CI Continuous Integration labels Jul 1, 2024
@vszakats vszakats changed the title Tal r/vcpkg more libs GHA: add MSVC UWP job, expand jobs with more options Jul 1, 2024
@talregev
Copy link
Contributor Author

talregev commented Jul 1, 2024

I share with you the times: 55 min is not enough with build and testing for openssl.
image
image

@vszakats
Copy link
Member

vszakats commented Jul 1, 2024

I share with you the times: 55 min is not enough with build and testing for openssl.

On one hand it is odd, because 27 minutes was the total build time curl-for-win needed for 3 targets (x64, arm64, x86), including openssl, curl and all the other dependencies (brotli, zlib, zstd, libssh2, nghttp2, nghttp3, ngtcp2, libpsl).

It seems that vcpkg is also installing its own copy of MSYS2, taking 2 minutes. While an MSYS2 already comes with the runner image.

Then building gsasl takes 10 minutes. Seems very long, probably all options are built in multiple flavours?
Its dependency, libicu takes 6 minutes. libicu isn't required for curl.

Then openssl takes 6 minutes.

Unless there is something we can do to make vcpkg builds more optimal, could we drop gsasl? It doesn't seem to add too much value compared to the bulk it requires in this setup.

FWIW gsasl can be built swiftly for the purpose of curl, and doesn't require libicu. If vcpkg has an options to optimize it, it could also help.

@talregev
Copy link
Contributor Author

talregev commented Jul 1, 2024

Unless there is something we can do to make vcpkg builds more optimal

I already include the cache to have vcpkg build time optimal.
All the previous times is for the first clean build. After that, with the cache, you will not see them. You will see the increase of time when vcpkg will update the library. Not all libraries is update at once.
I put the timing, to explain why I increase the timeouts, when you always saw 30 sec vcpkg build time.
These timeouts is only for the worst case scenario when all the libs updated at once.
Times after the cache for openssl:
image

@vszakats
Copy link
Member

vszakats commented Jul 1, 2024

Unless there is something we can do to make vcpkg builds more optimal

I already include the cache to have vcpkg build time optimal. All the previous times is for the first clean build. After that,

Caching is good, but it's not actually improving vcpkg build times. It'd be useful not to waste resources in the actual build.

E.g. the cache will rebuild every time it's invalidated, and with more stuff in it, it's invalided more often. How often, it's something to be seen. Ultimately we need to hit a good balance.

What is the reason for the duplicate cache keys generated for vcpkg?:
https://github.com/curl/curl/actions/caches

E.g. windows-openssl-85fa5017a117e57b6b7d2fd6e2434a8b3b413a2ba6603ccd4b6684eb31aa4bc7 is cached 17 times. Same with the other two vcpkg caches, schannel and no-ssl.

Shouldn't there be just one of each?

@talregev
Copy link
Contributor Author

talregev commented Jul 2, 2024

What is the reason for the duplicate cache keys generated for vcpkg?:
https://github.com/curl/curl/actions/caches

It generate hash every time that the vcpkg change. Most of the time it changed due requests from you to remove or change something in vcpkg. Also when vcpkg change itself. It should be different hash on such update that it will store the new libs that updated / changed. Then it take the most updated hash.

I can remove gsal as you request.
libicu come from libpsl lib.
If you remove or leave it like that, you will not notice the changes in ci in vcpkg build time. Most of the time it will be only 30 sec. Cache improve the vcpkg build time because every other time that vcpkg not change it take only 30 sec.
And if it dose change it take only the lib it change. It the most optimal that you ever get, for any set of libs that you will choose.

This PR I set new keys in the cache, so vcpkg build all the libs from scratch.

@vszakats
Copy link
Member

vszakats commented Jul 2, 2024

What is the reason for the duplicate cache keys generated for vcpkg?:
https://github.com/curl/curl/actions/caches

It generate hash every time that the vcpkg change. Most of the time it changed due requests from you to remove or change something in vcpkg. Also when vcpkg change itself. It should be different hash on such update that it will store the new libs that updated / changed. Then it take the most updated hash.

That's how it's supposed to work, I agree. But is it possible that these changed 17 times in the last couple of days? Even if we consider the tweaks done in this PR, but most copies belong to other PRs according to the web view.

The full list:

curl -A ' ' -fsS 'https://api.github.com/repos/curl/curl/actions/caches?per_page=100' | jq '.actions_caches[] | select(.key | startswith("windows"))'
curl -A ' ' -fsS 'https://api.github.com/repos/curl/curl/actions/caches?per_page=100&page=2' | jq '.actions_caches[] | select(.key | startswith("windows"))'

One interesting bit is these errors when saving the vcpkg cache, i.e. it tries to save a new version (of the same hash!), but (sometimes?) fails:

Failed to save: Unable to reserve cache with
key windows-openssl-85fa5017a117e57b6b7d2fd6e2434a8b3b413a2ba6603ccd4b6684eb31aa4bc7,
another job may be creating this cache. More details: Cache already exists.
Scope: refs/pull/14085/merge,
Key: windows-openssl-85fa5017a117e57b6b7d2fd6e2434a8b3b413a2ba6603ccd4b6684eb31aa4bc7,
Version: eb0a913cefd10ac3fc2bdab2b22da813587053e00376e7010f597fea35e8d025

Ref: https://github.com/curl/curl/actions/runs/9766007256/job/26957995299#step:6:8

A clean log looks like this:

Cache hit occurred on the primary key Windows-mingw-w64-9.5.0-x86_64, not saving cache.

I can remove gsal as you request. libicu come from libpsl lib. If you remove or leave it like that, you will not notice the changes in ci in vcpkg build time. Most of the time it will be only 30 sec. Cache improve the vcpkg build time because every other time that vcpkg not change it take only 30 sec. And if it dose change it take only the lib it change. It the most optimal that you ever get, for any set of libs that you will choose.

Dropping gsasl, and also libpsl would be best for these jobs I think. We have plenty of other jobs testing it already. Also ICU isn't necessary for libpsl after v0.21.5 and building unrelated 3rd party packages in CI here doesn't serve curl much IMO.

[ edit: As for gsasl, the vcpkg package looks clean, and I still don't see why it needs 10 minutes. It takes 3.5 (2.5 without shared) on a fairly slow local machine, with the slower mingw-w64 cross-toolchain. Even that is long; gsasl has a super-slow configure script, checking almost 600 items, among them 60 warning options. It's also fairly large project. curl uses a handful of function calls from it and has no tests: 3eebbfe. Build-tests are already done in two macOS jobs. ]

@vszakats
Copy link
Member

vszakats commented Jul 4, 2024

After experimenting, I've found these ways to streamline using vcpkg:

  • cache on a per-package basis.
    This feature comes out of the box and can be enabled with a few global envs.
    It allows to share cached packages between jobs, e.g. libssh2 only needs to be built once per platform (instead of once per job).
    It also only rebuilds only the individual dependencies (not all) as needed.
    It also seems to fix the duplicate cache entry issue.
  • rely on the vcpkg installed on the runner image, instead of rolling our own.
    It means we have to wait a bit till the new gsasl package hits the image, but
    we gain stability, save the manual repo checkout and vcpkg binary download/init.
    Sticking to the defaults makes using vcpkg easier.
    I believe we can also update it to the latest ports (it's a .git repo) if we absolutely need to. (I haven't tested it)

@talregev
Copy link
Contributor Author

talregev commented Jul 4, 2024

Hi, I have busy days and I cannot much response and I will not be available until next week.
I see the article and your changes in the other PR. I don't familiar with the other cache variable. I don't know how it calculate the hash, and how it react when vcpkg itself is updated. Maybe it a good change. One note, you mentioned the libssh2. You wanted 2 version of it. One with openssl, one without it. If you don't type anything it will bring you the openssl version.
So it can be one cache location, and every job will build their own libs.

another note, I also put the other libraries, not just to trigger curl tests. also to test curl cmake, and check that if someone change it, that it not break the vcpkg detection. we can put separate only building curl and tests without running them.
Let me know what you think about this note.

Also if you are confident with your changes feel free to advance. I can give you my opinion next week 🙏🏻

Thank you for letting me know 🙏🏻

@vszakats
Copy link
Member

vszakats commented Jul 4, 2024

Thank you @talregev.

Of course we can do build-only tests too.

I think with the granular cache we can do gsasl and libpsl, and possibly more, with reasonable resources. Yeah, the mixed libssh2 might be one reason for the cache updates. I was also suspecting a zstd confusion (which I was investigating in another PR for many days if not weeks). zstd installing a DLL might hit that, I don't know.

Now bumped into LibreSSL possibly colliding with a non-vcpkg OpenSSL. Will investigate.

One thing I'd find useful, is if libpsl would be available from vcpkg in a standalone (meaning no libicu, no libidn2) option. This should be possible with the latest libpsl release, at least as a package option.

@talregev
Copy link
Contributor Author

talregev commented Jul 4, 2024

for libpsl you can do:
libpsl[core]
I didn't try it. but it remove the features.

@vszakats
Copy link
Member

vszakats commented Jul 4, 2024

for libpsl you can do: libpsl[core] I didn't try it. but it remove the features.

libpsl requires either of the IDN libs:

CMake Error at ports/libpsl/portfile.cmake:26 (message):
  At least one of libidn2 and libicu should be selected.

https://github.com/curl/curl/actions/runs/9795570152/job/27048057034#step:4:62

They can be avoided by embedding the PSL list into the binary, and in this case pre-processing it using a libpsl-supplied Python script at build-time (probably Meson does this, I haven't tried), so ICU/IDN isn't necessary at runtime.

@vszakats
Copy link
Member

vszakats commented Jul 7, 2024

Thank you, merged.

vszakats added a commit that referenced this pull request Jul 7, 2024
- cache on a per-package basis.
  Replace manual caching with a built-in solution. It shares cached
  package builds between jobs, e.g. libssh2 only builds once
  per platform (instead of once per job). Individual packages are built
  as needed (not the whole per-job tree). It also fixes the duplicate
  cache entry issues.
  Ref: https://learn.microsoft.com/en-us/vcpkg/consume/binary-caching-github-actions-cache
  Follow-up to e26cbe2 #13979
  Follow-up to cb22cfc #14077

- add BoringSSL job with ECH enabled. The first such job in the curl CI.

- add LibreSSL job.

- use vcpkg pre-installed on the runner image, instead of rolling our
  own. This is quicker, simpler and more robust.
  Follow-up to e26cbe2 #13979

- show pre-installed vcpkg and ports version.

- drop `gsasl` dependency till it reaches the pre-installed vcpkg ports.

- re-add `find .` to see the binaries generated.

- simplify setting up `PATH`.

- exclude failing tests for any job enabling WinIDN.

- drop collecting and uploading log archives. We already dump CMake
  logs, and our build doesn't use Ninja. Rest of files weren't generated
  by the curl build. We don't aim to debug vcpkg package builds.

Closes #14090
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

2 participants