ci, gha: Run all MSVC tests on Windows natively

hebasto commented at 9:50 pm on August 15, 2023: member

This PR addresses the #1398 (review) and is an alternative to #1398 in some part.

Advantages:

That would get us rid of the entire business of “installing MSVC” and it would make this Docker image much smaller.

Disadvantages:

We won’t have Dockerfile anymore that people can use locally, e.g., when working on problems that appear only with MSVC.

We’ll probably need some separate CI script for Windows / CMake. Currently, we simply reuse the Linux / autotools script, which is neat.

I do prefer this approach because it is natural.

hebasto cross-referenced this on Aug 15, 2023 from issue ci, gha: Add Windows jobs based on Linux image by hebasto

hebasto force-pushed on Aug 16, 2023

in .github/workflows/ci.yml:60 in 82b59f0ece outdated

58           build\src\RelWithDebInfo\bench_internal.exe
59           build\src\RelWithDebInfo\bench.exe
60 
61+      - uses: ilammy/msvc-dev-cmd@v1
62+      - name: C++ (public headers)
63+        run: cl.exe -c -WX -TP include/*.h

real-or-random commented at 8:55 am on August 16, 2023:

Is the ilammy/msvc-dev-cmd@v1 required only for the C++ compiler? To be honest, we could drop that part then. But do you really need this? Can’t we use the already installed MSVC? (I mean, we use it above to compile the library, so it must be somewhere on these machines, right?)

hebasto commented at 8:58 am on August 16, 2023:

We can use C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/14.35.32215/bin/HostX64/x64/cl.exe, but it needs to be updated every time GH Windows image is updated due to the version part in the path.

real-or-random commented at 9:07 am on August 16, 2023:

Couldn’t we just invoke it with a glob like C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/*/bin/HostX64/x64/cl.exe? (or whatever the windows equivalent of this is, or use bash).

hebasto commented at 9:38 am on August 16, 2023:

Couldn’t we just invoke it with a glob…

I guess not:

 0  dir "C:/Program Files/Microsoft Visual Studio/2022/Enterprise/VC/Tools/MSVC/"
 1  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
 2
 3    Directory: C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC
 4
 5Mode                 LastWriteTime         Length Name
 6----                 -------------         ------ ----
 7d----            8/4/2023 10:41 PM                14.16.27023
 8d----            8/4/2023 10:39 PM                14.29.30133
 9d----            8/4/2023 11:07 PM                14.35.32215
10d----            8/4/2023 10:28 PM                14.36.32532

hebasto commented at 9:43 am on August 16, 2023:

To be honest, we could drop that part then.

It has been dropped.

real-or-random commented at 10:13 am on August 16, 2023:

Okay, after some digging, I’m actually convinced that https://github.com/ilammy/msvc-dev-cmd is the commonly accepted way of doing this. I also found https://github.com/actions/runner-images/issues/5986#issuecomment-1201012941, but not sure if it’s better.

So I’m leaning towards bringing https://github.com/ilammy/msvc-dev-cmd back. Then it will be even cleaner to do this step before the regular CMake invocations, too? And If this ever breaks in the future, it should still be easy to get rid of it. Sorry for the back and forth.

hebasto commented at 10:21 am on August 16, 2023:

OK.

So I’m leaning towards bringing ilammy/msvc-dev-cmd back. @achow101 or @fanquake

Please add ilammy/msvc-dev-cmd@* to the repository’s Actions permissions.

sipa commented at 3:04 pm on August 16, 2023:

It seems to have been added already.

hebasto commented at 3:22 pm on August 16, 2023:

It seems to have been added already.

It works!

real-or-random commented at 4:25 pm on August 16, 2023:

Then it will be even cleaner to do this step before the regular CMake invocations, too?

What do you think? Having all the tools in the %PATH% seems to be the most natural setup, and it seems that CMake works here more or less by accident. The environment is set up for MSBuild, but because this often (IIUC) used with CMake support, it seems that CMake is supported, too.

Anyway, this is more of a cosmetic thing right now.

hebasto commented at 6:30 pm on August 16, 2023:

… it seems that CMake works here more or less by accident.

The Windows image has CMake installed separately from the Visual Studio tools, which is nice.

In turn, CMake itself is smart enough to find paths to all tools it requires.

Only direct invocation of the cl.exe compiler requires having it in the %PATH%.

That’s why I do prefer keeping these lines of code together.

real-or-random commented at 7:12 am on August 17, 2023:

Ok, makes sense. Keeping them together also self-documents the fact that the external action is only required for that single line of code.

in ci/linux-debian.Dockerfile:25 in b07265333b outdated

20@@ -21,7 +21,8 @@ RUN apt-get update && apt-get install --no-install-recommends -y \
21         gcc-aarch64-linux-gnu libc6-dev-arm64-cross libc6-dbg:arm64 \
22         gcc-powerpc64le-linux-gnu libc6-dev-ppc64el-cross libc6-dbg:ppc64el \
23         gcc-mingw-w64-x86-64-win32 wine64 wine \
24-        gcc-mingw-w64-i686-win32 wine32
25+        gcc-mingw-w64-i686-win32 wine32 \
26+        python3

real-or-random commented at 8:56 am on August 16, 2023:

Where do we need python?

hebasto commented at 9:00 am on August 16, 2023:

0make  precompute_ecmult_gen
1make  precompute_ecmult
2mkdir -p src/wycheproof
3python3 ./tools/tests_wycheproof_generate.py ./src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json > src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h
4/bin/bash: line 1: python3: command not found

real-or-random commented at 9:04 am on August 16, 2023:

Oh indeed, I forgot.

real-or-random commented at 9:26 am on August 16, 2023: contributor

We won’t have Dockerfile anymore that people can use locally, e.g., when working on problems that appear only with MSVC.

I don’t think that’s a big deal. This feature is rarely needed, and if it’s needed, people can use the one here: https://github.com/mstorsjo/msvc-wine/blob/master/Dockerfile

So yes, Concept ACK. I agree it’s more natural to test Windows builds on native Windows. :)

hebasto force-pushed on Aug 16, 2023

real-or-random added the label ci on Aug 16, 2023

real-or-random commented at 11:28 am on August 16, 2023: contributor

We’ll need to bring the wineboot back now (https://github.com/bitcoin-core/secp256k1/commit/c7db4942b34acd2a34e6249112f6c1db6cf5681d) to resolve the failures.

We could add it to the CI script (instead of the Docker image). It runs very fast, but it will add 1.3 GB to the image, so having it in the script may be the better trade-off. (wine prefixes seem to be pretty big these days. I heard this is because wine will copy a lot of DLLs there now, instead of symlinking them… This improves the emulation for anti-cheat detection for games… ^^)

hebasto force-pushed on Aug 16, 2023

in ci/cirrus.sh:42 in 299922f074 outdated

39+        wine64 wineboot --init
40+
41+        # Wait until the wineserver process has
42+        # exited, to avoid corrupting the wine prefix.
43+        while (ps -A | grep wineserver) > /dev/null; do sleep 1; done
44+

real-or-random commented at 3:53 pm on August 16, 2023:

nit: I think this can be dropped if we call it here… The issue here was that if we call it at the end of the Docker build, it exists because the wineserver exits, and this was bad. Here, this should be fine, we’ll anyway want a running wineserver.

in ci/cirrus.sh:37 in 299922f074 outdated

30@@ -31,12 +31,20 @@ print_environment() {
31 }
32 print_environment
33 
34-# Start persistent wineserver if necessary.
35-# This speeds up jobs with many invocations of wine (e.g., ./configure with MSVC) tremendously.
36 case "$WRAPPER_CMD" in
37     *wine*)
38+        # Initialize the wine environment.
39+        wine64 wineboot --init

real-or-random commented at 4:05 pm on August 16, 2023:

0        wineboot --init

That’s enough, and this should fix the failure.

nit: I wonder if it makes sense to do this in the yaml file. People tend to run this script locally on their machine, and it’s probably a tiny bit better to let it only do “non-intrusive” stuff. wineboot --init shouldn’t do any harm, but it touches the user’s wine prefix.

real-or-random commented at 4:08 pm on August 16, 2023: contributor

Pretty happy with this now, modulo the nits

hebasto force-pushed on Aug 16, 2023

in ci/cirrus.sh:39 in 71fc13ae0b outdated

37     *wine*)
38         # Make sure to shutdown wineserver whenever we exit.
39         trap "wineserver -k || true" EXIT INT HUP
40-        wineserver -p
41         ;;
42 esac

real-or-random commented at 7:21 am on August 17, 2023:

Oh, I didn’t mean to remove this code. Starting the wineserver is still useful, because it speeds up the tests. I just meant that we can drop these lines (which you did now):

0        # Wait until the wineserver process has
1        # exited, to avoid corrupting the wine prefix.
2        while (ps -A | grep wineserver) > /dev/null; do sleep 1; done

hebasto commented at 7:34 am on August 17, 2023:

With wineserver -p script fails:

0+ trap wineserver -k || true EXIT INT HUP
1+ wineserver -p
2+ wineserver -k
3Exit status: 2

real-or-random commented at 7:56 am on August 17, 2023:

oops, let me have a look in the Cirrus console

real-or-random commented at 8:40 am on August 17, 2023:

Sorry for constantly offering tiny suggestions that break stuff… -.-

What happens here is that wineserver -p fails because there’s a server running already (from the wineboot still, but it’s not a persistent one…) While the “wait until the wineserver process has exited” code was still there, this didn’t happen.

I took a step back and did some testing on my local machine:

wineserver -p is at most useful with MSVC builds. With mingw builds, there won’t be many wine invocations. And even with MSVC builds, it’s questionable: The server stays around for 3 seconds and waits for another wine process to appear before it exits, so this should be necessary only when things are really slow. This has indeed speed up builds for us in the past, but maybe this was because some other part didn’t work… No idea.
wineboot --init doesn’t seem to be required. The first wine invocation should init stuff automatically (and this is how I know wine…)

So I suggest to just remove all of this and pushed it as a fixup here: https://github.com/real-or-random/secp256k1/tree/230815-gha-msvc . CI still in my repo running, but I hope that it just works now…

hebasto commented at 8:54 am on August 17, 2023:

https://github.com/real-or-random/secp256k1/commit/76ecd47c42e320cab53ae30dce18710ad3a45ff1 has been cherry-picked and squashed with the last commit.

hebasto force-pushed on Aug 17, 2023

hebasto commented at 8:59 am on August 17, 2023: member

I’ve just realized that “C++ (public headers)” is a step in every matrix-generated MSVC job. Shouldn’t it be a separated job?

real-or-random commented at 9:03 am on August 17, 2023: contributor

I’ve just realized that “C++ (public headers)” is a step in every matrix-generated MSVC job. Shouldn’t it be a separated job?

Yes, I had the same thought earlier. I think it makes sense to move it to a separate job if you’re willing to make that change.

ci, gha: Run all MSVC tests on Windows natively 3545dc2b9b

ci: Remove Windows MSVC tasks from Cirrus CI

Co-authored-by: Tim Ruffing <crypto@timruffing.de>

d78bec7001

hebasto force-pushed on Aug 17, 2023

hebasto commented at 9:15 am on August 17, 2023: member

I’ve just realized that “C++ (public headers)” is a step in every matrix-generated MSVC job. Shouldn’t it be a separated job?

Yes, I had the same thought earlier. I think it makes sense to move it to a separate job if you’re willing to make that change.

Fixed.

real-or-random commented at 1:50 pm on August 17, 2023: contributor

ACK d78bec7001fe6f5ed8d5b215bf61e7b74e3369ca

real-or-random cross-referenced this on Aug 17, 2023 from issue ci: Future of CI after Cirrus pricing change by real-or-random

jonasnick commented at 4:50 pm on August 17, 2023: contributor

ACK d78bec7001fe6f5ed8d5b215bf61e7b74e3369ca

jonasnick merged this on Aug 17, 2023

jonasnick closed this on Aug 17, 2023

hebasto deleted the branch on Aug 17, 2023

ci, gha: Run all MSVC tests on Windows natively #1401