-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[vcpkg-scripts] Update msys2, add x64 PKGCONFIG #35331
Conversation
This reverts commit aa27037.
It's using x86_64-pkgconf on arm64 (was using i686), Is there something wrong? |
It must run on the host. The target is set by variables. #24160 had it wrong. |
I thought that arm64-windows and x86-windows are native. |
Native means host==target. Not sure if this is relevant here. For the build tools, we can benefit from compatibility and emulation, such as running x64 on x86 or even running x86 on arm64 windows. But whatever is executed, it is dependent on the host, not on the target (which might be arm-android or wasm32-emscripten or some other CPU). Given the windows CPU support, it would probably be enough to have x86 for pkgconf. But I don't mind being prepared for removal of x86 tools. |
native means arm64-windows runs on aarch64 CPU which means selecting aarch64-pkgconf. |
vcpkg CI builds arm64-windows and x86-windows on x64-windows...
Sure, on an arm64 host, arm64 pkgconf is native. But this is a much more specific statement than "arm64-windows is native". But there is nothing wrong with this PR, isn't it? |
Yes, I was just explaining my wrong expectations. |
I suggest to merge #34776 at the same time, without another integration run. Why? |
Pretty sure this caused a big perf problem. Compare November 28:
to last Friday:
Do you know of obvious perftrocities here? |
I'n not aware of particular problems. libunistring: This port uses a lot of shell scripting, sub-shells, pipelines. And there is a patch to parallelize some independent steps, with signifcant effect in CI. Either forks became more expensive, or parallelism is reduced. Extendig my #34274 notes, major changes were: gfortran -> 13.2 There was no update to make, so parallelism shouln't have changed. |
@BillyONeal This PR seems unrelated to the performance regression: --- 2023-11-30
+++ 2023-12-02
@@ -1,4 +1,4 @@
-Installing 959/2218 libunistring:x64-windows...
+Installing 960/2220 libunistring:x64-windows...
Building libunistring:x64-windows...
-- Downloading https://ftp.gnu.org/gnu/libunistring/libunistring-1.1.tar.xz;https://www.mirrorservice.org/sites/ftp.gnu.org/gnu/libunistring/libunistring-1.1.tar.xz -> libunistring-1.1.tar.xz...
-- Extracting source D:/downloads/libunistring-1.1.tar.xz
@@ -56,5 +56,5 @@
-- Building x64-windows-rel
-- Installing x64-windows-rel
-- Performing post-build validation
-Stored binaries in 1 destinations in 353 ms.
-Elapsed time to handle libunistring:x64-windows: 9.1 min
+Stored binaries in 1 destinations in 359 ms.
+Elapsed time to handle libunistring:x64-windows: 53 min |
What I find irritating is that many CI runs (incl. PR CI) don't take some msys2 packages from asset cache.
|
Asset cache hits don't print special output. When you see "using cached" that means "it was already downloaded and we didn't even attempt to use the asset cache". |
OK, don't loose sleep over it :) |
Maybe vcpkg should print better metrics. E.g. extract download time and time required for post build checks. This way the real portfile execution time can be better evaluated. |
At least the raw CI logs have timestamps (as shown by @BillyONeal). The slowdown is already visible in the configure step (3 min -> 13 min per config) which is not parallel. libunistring is really heavy in invoking subshells and tools, at all stages. |
To be clear, we're seeing this behavior all over the place, not just libunistring. (It was just an extreme example) And the subshells and stuff are why I thought this PR might be related. But like I said, don't lose sleep over it; my poking this PR was a shot in the dark |
I know that well. And I sleep very well, given that there is a clear indication that there was one regular build with the PR merged. Given all the current msys winpthread issues, you probably also sleep better with msys updated early ;-) |
time to build the whole msys stack within vcpkg? would also get rid of the updating issues with msys ;) |
Some builds are still at normal speed. https://dev.azure.com/vcpkg/public/_build/results?buildId=97668&view=results: https://dev.azure.com/vcpkg/public/_build/results?buildId=97657&view=results: |
Using pacman is better. |
This discussion is entirely unrelated to the slowdown. |
Comparing x64-windows for the mentioned fast x64-windows CI run with the last full CI run (slow, canceled after 2d), and filtering just "Elapsed times", there is a strong indication that all packages take more time, even those that don't use msys2 or
|
No its not. That was worse then the current situation. Maybe there is some (disk) performance issue with some runners. |
Yeah, but only the heavy msys users went up by like 6 times.
Hmmmm.... strange. I'm just going to cross my fingers and hope patch tuesday deploy helps... |
And these are the users with many forks/commands/subshells. |
https://dev.azure.com/vcpkg/public/_build/results?buildId=97842&view=results Whatever it was seems fixed now |
Includes #34274, #35299 (plus one fix), plus a reimplementation of #24160.
NB: Adding x64 pkgconf implies that x86 pkgconf is no longer tested in vcpkg CI.