ci: Add MSVC builds #1084

pull real-or-random wants to merge 12 commits into bitcoin-core:master from real-or-random:202203-msvc changing 8 files +165 −47
  1. real-or-random commented at 7:12 pm on March 10, 2022: contributor

    This adds MSVC builds built on Linux using wine. This requires some settings of tools and flags because the autotools support for MSVC is naturally somewhat limited.

    The advantage of this approach is that it is compatible with our existing CI scripts, so there’s no need to write a Windows CI script (in PowerShell or similar). If we want to test building and running on Windows native (e.g., as supported by Cirrus CI) we could still do this in the future.

    Another advantage of this approach is that contributors can simply use the docker image if they need a MSVC installation in a non-Windows environment.

  2. real-or-random commented at 7:37 pm on March 10, 2022: contributor

    by the way, rather for fun:

     0Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)
 1
 232-bit MSVC -O2 (over wine):
 3ecdsa_verify                  ,   226.0       ,   227.0       ,   230.0
 4ecdsa_sign                    ,   133.0       ,   134.0       ,   134.0
 5ecdh                          ,   226.0       ,   227.0       ,   229.0
 6ecdsa_recover                 ,   230.0       ,   231.0       ,   233.0
 7schnorrsig_sign               ,   120.0       ,   120.0       ,   123.0
 8schnorrsig_verify             ,   227.0       ,   228.0       ,   232.0
 9
1032-bit MinGw -O2 (over wine):
11ecdsa_verify                  ,   195.0       ,   196.0       ,   198.0
12ecdsa_sign                    ,   115.0       ,   116.0       ,   117.0
13ecdh                          ,   199.0       ,   200.0       ,   204.0
14ecdsa_recover                 ,   199.0       ,   200.0       ,   204.0
15schnorrsig_sign               ,   101.0       ,   102.0       ,   103.0
16schnorrsig_verify             ,   196.0       ,   197.0       ,   200.0
17
1832-bit Linux GCC -O2:
19ecdsa_verify                  ,   195.0       ,   197.0       ,   200.0    
20ecdsa_sign                    ,   115.0       ,   116.0       ,   122.0    
21ecdh                          ,   199.0       ,   200.0       ,   202.0    
22ecdsa_recover                 ,   199.0       ,   201.0       ,   202.0    
23schnorrsig_sign               ,   101.0       ,   102.0       ,   104.0    
24schnorrsig_verify             ,   196.0       ,   197.0       ,   199.0

    edit: Fixed Linux figures, looks reasonable now.

  3. real-or-random force-pushed on Mar 10, 2022
  4. real-or-random force-pushed on Mar 10, 2022
  5. real-or-random force-pushed on Mar 12, 2022
  6. real-or-random force-pushed on Mar 12, 2022
  7. real-or-random force-pushed on Mar 12, 2022
  8. real-or-random commented at 10:55 am on March 12, 2022: contributor
    Ready for review (if CI passes…)
  9. real-or-random marked this as ready for review on Mar 12, 2022
  10. real-or-random force-pushed on Mar 12, 2022
  11. real-or-random commented at 10:09 am on March 13, 2022: contributor

    This will probably conflict with #1088. Let’s first try to merge the fix there.

    edit: rebased on top of #1088.

  12. real-or-random cross-referenced this on Mar 14, 2022 from issue configure: Use modern way to set AR by real-or-random
  13. real-or-random force-pushed on Mar 14, 2022
  14. real-or-random force-pushed on Mar 14, 2022
  15. real-or-random force-pushed on Mar 15, 2022
  16. real-or-random force-pushed on Mar 15, 2022
  17. real-or-random force-pushed on Mar 16, 2022
  18. real-or-random force-pushed on Mar 16, 2022
  19. real-or-random force-pushed on Mar 16, 2022
  20. bench: Make benchmarks compile on MSVC 1a6be5745f
  21. configure: Output message when checking for valgrind cca8cbbac8
  22. real-or-random force-pushed on Mar 16, 2022
  23. real-or-random force-pushed on Mar 16, 2022
  24. configure: Don't abort if the compiler does not define __STDC__ 
    This removes a check for $ac_cv_prog_cc_c89 which is set by AC_PROG_CC
if defined(__STDC__) in the preprocessor. (Standard compliant compilers
are supposed to define __STDC__ to 1 but the value is actually not
checked here.)

Unfortunately, MSVC doesn't define it, so configure fails for MSVC.

This check is not very useful in practice. Over 30 years after C89 has
been released, there are no C compilers out there that are not
sufficiently compliant with C89 for the project. The only practically
relevant case was that the check rejected C++ compilers. A different
method to reject C++ compilers will be introduced in a later commit.
    1cc0941414
  25. build: Reject C++ compilers in the preprocessor 3b4f3d0d46
  26. configure: Add a few CFLAGS for MSVC 09f3d71c51
  27. real-or-random force-pushed on Mar 17, 2022
  28. real-or-random force-pushed on Mar 18, 2022
  29. sipa commented at 6:18 pm on June 8, 2022: contributor
    utACK c16afd001ac441c7729475fa18bcea60ef2bad2f
  30. in .cirrus.yml:260 in c16afd001a outdated 
    259     EXPERIMENTAL: yes
260     SCHNORRSIG: yes
261     CTIMETEST: no
262+  matrix:
263+    - name: "x86_64 (mingw32-w64): Windows (Debian stable, Wine)"
264+      env:
    jonasnick commented at 9:24 am on June 23, 2022:
    There are a few trailing whitespaces in cirrus.yml and cirrus.sh.
    real-or-random commented at 8:48 am on June 29, 2022:
    fixed
  31. in src/modules/schnorrsig/bench_impl.h:99 in c16afd001a outdated 
     96-    free(data.msgs);
 97-    free(data.sigs);
 98+    free((void *)data.keypairs);
 99+    free((void *)data.pk);
100+    free((void *)data.msgs);
101+    free((void *)data.sigs);
    jonasnick commented at 9:25 am on June 23, 2022:
    Perhaps add comment so this isn’t changed back at the next best opportunity?
    real-or-random commented at 9:12 am on June 29, 2022:
    done
  32. in src/secp256k1.c:16 in c16afd001a outdated 
    11+ * because it is still good to know if this will lead to real trouble, so
12+ * there is a possibility to override the check. But be warned that
13+ * compiling with a C++ compiler is not supported. */
14+#if defined(__cplusplus) && !defined(SECP256K1_CPLUSPLUS_TEST_OVERRIDE)
15+#error Trying to compile a C project with a C++ compiler.
16+#endif
    jonasnick commented at 9:28 am on June 23, 2022:
    Is it intended that I don’t see this error with g++ unless I also compile with -fpermissive? This is the case because g++ aborts earlier due to other errors. So if someone would accidentally use g++ (without -fpermissive) then they may just be confused vs. having a rather clear error message as before. Not saying that this is terrible, but seems like a change from the previous behavior.
    real-or-random commented at 4:35 pm on June 28, 2022:
    Hm, I see the error with g++ though it’s buried within hundreds of others errors. I think it’s fine as long it won’t compile. And this check is better because it’s independent of autotools (and will work when compiling manually, etc.)
    jonasnick commented at 5:42 pm on June 28, 2022:

    is better because it’s independent of autotools

    Right, ok.

  33. in configure.ac:119 in c16afd001a outdated 
    131+
132+    # MSVC
133+    # Assume MSVC if we're building for Windows but not with GCC or compatible;
134+    # libtool makes the same assumption internally.
135+    # Note that "/opt" and "-opt" are equivalent for MSVC; we use "-opt" because "/opt" looks like a path.
136+    if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then
    jonasnick commented at 9:31 am on June 23, 2022:

    Instead of

    0if test "x$GCC" = "xyes"; then ...
1if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then ...

    why not set up the conditions like this

    0if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then ...
1else ...

    or

    0if test "x$GCC" = "xyes"; then ...
1if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then
2else error("something is wrong")
    real-or-random commented at 4:49 pm on June 28, 2022:

    why not set up the conditions like this

    0if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then ...
1else ...

    That’s certainly closer to what we have now. When I wrote this, I thought it’s a good idea not to fiddle with the CFLAGS at all if this not GCC or compatible (for example, your random compiler X may not support any of these flags but also not error out and just spit out warning, so we’ll add all of them wrongly).

    or

    0if test "x$GCC" = "xyes"; then ...
1if test x"$GCC" != x"yes" && test x"$build_windows" = x"yes"; then
2else error("something is wrong")

    That would always error out on platforms other than Windows (if I’m not mistaken).

    jonasnick commented at 5:42 pm on June 28, 2022:

    thought it’s a good idea not to fiddle with the CFLAGS at all if this not GCC or compatible

    Yeah that makes sense. Just wanted to make sure this wasn’t by accident.

    That would always error out on platforms other than Windows

    I believe I intended the second if to be an else if. Anyway, erroring out is unnecessary.

  34. in .cirrus.yml:284 in c16afd001a outdated 
    283+    SCHNORRSIG: yes
284+    CTIMETEST: no
285+    # Set non-essential options that affect the CLI messages here.
286+    # (They depend on the user's taste, so we don't want to set them automatically in configure.ac.)
287+    CFLAGS: -nologo -diagnostics:caret
288+    LDFLAGS: -XCClinker -nologo -XCClinker -diagnostics:caret
    jonasnick commented at 9:32 am on June 23, 2022:
    Do you think -WX (treat warnings as errors) would be too fragile?
    real-or-random commented at 3:49 pm on June 28, 2022:

    This is done a few lines above in WERROR_CFLAGS.

    IIRC, the reason for a separate variable is that we don’t want to set it in CFLAGS, at least for gcc and compatible, because that would override the default CFLAGS of -O2 -g. This will make testing the defaults difficult and then we’d need to pass -g manually. Not this PR but I can add a comment that clarifies this.

    jonasnick commented at 5:42 pm on June 28, 2022:
    Ah sorry I missed this. There’s a comment in configure.ac which is sufficient imo.
    real-or-random commented at 9:15 am on June 29, 2022:
    I added a commit that improves the comment. It took me a while to reconstruct the rationale when answering your question here, though I wrote that code.
  35. in configure.ac:40 in c16afd001a outdated 
    31@@ -32,17 +32,19 @@ AM_INIT_AUTOMAKE([1.11.2 foreign subdir-objects])
32 # Make the compilation flags quiet unless V=1 is used.
33 m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])])
34 
35-PKG_PROG_PKG_CONFIG
36-
37 AC_PROG_CC
38-if test x"$ac_cv_prog_cc_c89" = x"no"; then
39-  AC_MSG_ERROR([c89 compiler support required])
40-fi
    jonasnick commented at 9:35 am on June 23, 2022:
    Did you try the MSVC -Za flag which according to the docs should set __STDC__? (https://docs.microsoft.com/en-us/cpp/build/reference/za-ze-disable-language-extensions?view=msvc-170)
    real-or-random commented at 4:33 pm on June 28, 2022:

    Yeah, -Za is stupid because even windows headers don’t adhere it to it and then the compilation of the examples fail…

     0\opt\msvc\kits\10\include\10.0.19041.0\um\winnt.h(12657,14): error C2467: illegal declaration of anonymous 'union'
 1            };
 2             ^
 3\opt\msvc\kits\10\include\10.0.19041.0\um\winnt.h(12874,10): error C2467: illegal declaration of anonymous 'struct'
 4        };
 5         ^
 6\opt\msvc\kits\10\include\10.0.19041.0\um\winnt.h(12875,6): error C2467: illegal declaration of anonymous 'union'
 7    };
 8     ^
 9\opt\msvc\kits\10\include\10.0.19041.0\um\winnt.h(18837,6): error C2467: illegal declaration of anonymous 'struct'
10    };
11     ^
12\opt\msvc\kits\10\include\10.0.19041.0\um\winnt.h(21055,24): error C2133: 'Policies': unknown size
13    IMAGE_POLICY_ENTRY Policies[];
14                       ^
15\opt\msvc\kits\10\include\10.0.19041.0\um\winioctl.h(4337,10): error C2467: illegal declaration of anonymous 'struct'
16        };
17         ^
18\opt\msvc\kits\10\include\10.0.19041.0\um\winioctl.h(4338,6): error C2467: illegal declaration of anonymous 'union'
19    };

    We could use different flags for the examples but this will be more complex, I think.

    jonasnick commented at 5:43 pm on June 28, 2022:
    Ok, let’s not make it more complex.
  36. real-or-random force-pushed on Jun 29, 2022
  37. schnorrsig bench: Suppress a stupid warning in MSVC bd81f4140a
  38. configure: Convince autotools to work with MSVC's archiver lib.exe 2be6ba0fed
  39. ci: Add MSVC builds 
    This adds MSVC builds built on Linux using wine. This requires some
settings of tools and flags because the autotools support for MSVC is
naturally somewhat limited.

The advantage of this approach is that it is compatible with our
existing CI scripts, so there's no need to write a Windows CI script
(in PowerShell or similar). If we want to test building and running on
Windows native (e.g., as supported by Cirrus CI) we could still do this
in the future.

Another advantage of this approach is that contributors can simply use
the docker image if they need a MSVC installation in a non-Windows
environment.

This commit also improves the Dockerfile by grouping RUN commands
according to Docker docs:
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run
    9efc2e5221
  40. ci: Add 32-bit MinGW64 build 
    This commit also raises the TEST_ITERS for wine tasks to the default.
The overhead of wine is negligible, so we can certainly afford the same
number of iterations as for native Linux tests.
    3fb3269c22
  41. ci: Run persistent wineserver to speed up wine 51f296a46c
  42. ci: Add a C++ job that compiles the public headers without -fpermissive 8dc4b03341
  43. real-or-random force-pushed on Jun 29, 2022
  44. configure: Improve rationale for WERROR_CFLAGS 49e2acd927
  45. real-or-random commented at 9:16 am on June 29, 2022: contributor
    addressed all comments
  46. jonasnick commented at 9:30 am on June 29, 2022: contributor
    ACK 49e2acd927ce9eb806cc10f3a1fd89a9ddd081e2
  47. jonasnick merged this on Jun 29, 2022
  48. jonasnick closed this on Jun 29, 2022
  49. real-or-random cross-referenced this on Jul 2, 2022 from issue build: Add CMake-based build system by hebasto
  50. dhruv referenced this in commit 2dfd7005d9 on Jul 19, 2022
  51. dhruv referenced this in commit 21e2acb595 on Jul 20, 2022
  52. dhruv referenced this in commit f70e7d8108 on Jul 20, 2022
  53. dhruv referenced this in commit 49704ff497 on Jul 20, 2022
  54. dhruv referenced this in commit d564c6266f on Jul 20, 2022
  55. dhruv referenced this in commit 908c171261 on Jul 20, 2022
  56. dhruv referenced this in commit a1ac8e1b7f on Jul 20, 2022
  57. dhruv referenced this in commit b02a78d8df on Jul 20, 2022
  58. dhruv cross-referenced this on Jul 20, 2022 from issue Bitcoin Core `configure` fails after #1084 was merged by dhruv
  59. hebasto referenced this in commit 07695f6c2d on Jul 21, 2022
  60. real-or-random cross-referenced this on Jul 21, 2022 from issue Revert "build: remove some no-longer-needed var unexporting from conf… by hebasto
  61. real-or-random referenced this in commit 7597a3d5e7 on Jul 21, 2022
  62. real-or-random cross-referenced this on Jul 21, 2022 from issue configure: Remove pkgconfig macros again (reintroduced by mismerge) by real-or-random
  63. real-or-random referenced this in commit cabe085bb4 on Jul 21, 2022
  64. real-or-random referenced this in commit 9f8a13dc8e on Jul 21, 2022
  65. dhruv referenced this in commit e5166959a4 on Jul 21, 2022
  66. dhruv referenced this in commit 726cbfe06c on Jul 21, 2022
  67. dhruv referenced this in commit c354ccd3e6 on Jul 21, 2022
  68. dhruv referenced this in commit 296cb3807d on Jul 21, 2022
  69. dhruv referenced this in commit a7efff1c21 on Jul 22, 2022
  70. dhruv referenced this in commit 5667aa958a on Aug 12, 2022
  71. dhruv referenced this in commit 06823cfe29 on Aug 24, 2022
  72. dhruv referenced this in commit 6eca30d4bd on Sep 2, 2022
  73. dhruv referenced this in commit c3ed192dda on Sep 2, 2022
  74. dhruv referenced this in commit 89ebab0601 on Oct 1, 2022
  75. dhruv referenced this in commit d6bcb105c3 on Oct 20, 2022
  76. dhruv referenced this in commit c27eb1e66a on Oct 20, 2022
  77. dhruv referenced this in commit a2e91d2816 on Oct 20, 2022
  78. dhruv referenced this in commit 0b21533c10 on Oct 21, 2022
  79. dhruv referenced this in commit 01dddb4cf6 on Oct 21, 2022
  80. dhruv referenced this in commit a0bb5b6946 on Nov 17, 2022
  81. dhruv referenced this in commit 2e4c03dd67 on Nov 17, 2022
  82. dhruv referenced this in commit 388c9b1b55 on Nov 21, 2022
  83. dhruv referenced this in commit 244eb87643 on Dec 7, 2022
  84. dhruv referenced this in commit 92cddabc43 on Dec 8, 2022
  85. sipa referenced this in commit 9d47e7b71b on Dec 13, 2022
  86. dhruv referenced this in commit 55ffd47cc6 on Dec 14, 2022
  87. dhruv referenced this in commit 967c65b158 on Dec 14, 2022
  88. dhruv referenced this in commit 78b5ddf28b on Jan 11, 2023
  89. dhruv referenced this in commit 215394a1d5 on Jan 11, 2023
  90. div72 referenced this in commit 945b094575 on Mar 14, 2023
  91. str4d referenced this in commit 0df7b459f6 on Apr 21, 2023
  92. dderjoel referenced this in commit 3768744290 on May 23, 2023
  93. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  94. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  95. jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115 by jonasnick
  96. jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115 by jonasnick
  97. jonasnick cross-referenced this on Jul 17, 2023 from issue Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115, 1116, 1120, 1122, 1121, 1128, 1131, 1144, 1150, 1146 by jonasnick


real-or-random sipa jonasnick

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 12:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me