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

Move Windows build to GHC 9.4 #415

Merged
merged 8 commits into from
Oct 26, 2023
Merged

Move Windows build to GHC 9.4 #415

merged 8 commits into from
Oct 26, 2023

Conversation

elopez
Copy link
Contributor

@elopez elopez commented Oct 20, 2023

Description

This PR introduces several changes to the Windows CI in order to build with GHC 9.4

GHC 9.4 currently breaks on Windows because it now uses a Clang-based toolchain and the C++ standard library name does no longer match:

Additionally, GHC 9.4 now requires the CLANG64 environment in MSYS2 for native code linking:

GHC 9.4 introduces a generic way of linking against the system C++ standard library, build-depends: system-cxx-std-lib:

This can be used to reliably link against the system's C++ standard library implementation without having to use platform-dependent conditionals. Unfortunately it does not properly work on Nix, so we link manually on Darwin to work around the issue:

The GitHub runner also has many things preinstalled that tend to cause conflicts during building. This PR switches it to use the minimal MSYS2 path option and adds-in the Haskell tools we need instead.

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@elopez elopez force-pushed the windows-9.4 branch 2 times, most recently from affc06a to 35aa4da Compare October 20, 2023 21:33
Starting with GHC 9.4, this can be used to reliably link against the
system's C++ standard library implementation. GHC 9.4 currently
breaks on Windows because 9.4 uses a Clang-based toolchain.

  * https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.4#windows-uses-a-clang-based-toolchain
  * https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.4#link-against-system-cxx-std-lib-instead-of-stdc
Also update the Haskell setup action's name, the old repository is
being deprecated.
@elopez elopez force-pushed the windows-9.4 branch 5 times, most recently from e43f573 to 8fcc575 Compare October 21, 2023 00:46
GHC 9.4 introduces a generic way of linking against the system C++
standard library:

  https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.4#link-against-system-cxx-std-lib-instead-of-stdc

Unfortunately it does not properly work on Nix, so we link manually
on Darwin to work around the issue:

  https://gitlab.haskell.org/ghc/ghc/-/issues/23138
@elopez elopez marked this pull request as ready for review October 24, 2023 19:21
The GitHub runner has lots of things preinstalled that tend to cause
conflicts. Use the minimal MSYS2 path option and add-in the Haskell
tools we need instead.
    D:\a\_temp\msys64\clang64\include\stdio.h:171:18: error:
         warning: '__format__' attribute argument not supported: gnu_scanf [-Wignored-attributes]
          __attribute__((__format__ (gnu_scanf, 2, 0))) __MINGW_ATTRIB_NONNULL(2)
                         ^
        |
    171 |   __attribute__((__format__ (gnu_scanf, 2, 0))) __MINGW_ATTRIB_NONNULL(2)
        |                  ^
hevm.cabal Outdated
Comment on lines 369 to 370
if os(darwin)
extra-libraries: c++
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this up so the diff looks smaller?

hevm.cabal Outdated
Comment on lines 309 to 310
if os(windows)
buildable: False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this up so the diff looks smaller? Thanks!

hevm.cabal Outdated
Comment on lines 232 to 233
if os(darwin)
extra-libraries: c++
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this up so the diff looks smaller?

Copy link
Collaborator

@msooseth msooseth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! I'm only worried about the MacOS build, it can be finicky. Do you know if this successfully compiles & builds a static, running binary on MacOS, especially on Arm?

@@ -103,8 +104,17 @@ library
Paths_hevm
autogen-modules:
Paths_hevm
if os(linux) || os(windows)
if impl(ghc >= 9.4) && !os(darwin)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we need the ghc version condition here. I'm fine with only supporting 9.4+ unless you need older versions for echidna?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually just moved echidna to 9.4 😄 so if you'd prefer to only support 9.4+ that's fine by me. It's not a lot of code to keep the 9.2 build-ability though (just the else block after this bit)

if os(linux) || os(windows)
if impl(ghc >= 9.4) && !os(darwin)
-- darwin is skipped because it produces this error when building
-- > ghc: loadArchive: Neither an archive, nor a fat archive: `/nix/store/l3lkdfm7sg1wwc850451cikqds766h15-clang-wrapper-11.1.0/bin/clang++'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this error before, iirc I was able to fix by adding the config to the executables instead of the library (that's why we have extra-libaries: c++ duplicated a few times). Maybe that would work too for the system-cxx-std-lib stuff too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it does not 😞 if I apply this example diff on top of this PR, it stops building correctly. That's why I opted to generalize it for !darwin with the new build-depends: system-cxx-std-lib but keep the duplicated extra-libraries: c++ in the executables for darwin

diff --git a/hevm.cabal b/hevm.cabal
index 2ebc62b6..b66c901d 100644
--- a/hevm.cabal
+++ b/hevm.cabal
@@ -196,7 +196,7 @@ executable hevm
   other-modules:
     Paths_hevm
   if os(darwin)
-    extra-libraries: c++
+    build-depends: system-cxx-std-lib
   build-depends:
     QuickCheck,
     aeson,
       > [1 of 2] Compiling Paths_hevm       ( dist/build/hevm/autogen/Paths_hevm.hs, dist/build/hevm/hevm-tmp/Paths_hevm.o, dist/build/hevm/hevm-tmp/Paths_hevm.dyn_o )
       > [2 of 2] Compiling Main             ( cli/cli.hs, dist/build/hevm/hevm-tmp/Main.o, dist/build/hevm/hevm-tmp/Main.dyn_o )
       > ghc: loadArchive: Neither an archive, nor a fat archive: `/nix/store/l3lkdfm7sg1wwc850451cikqds766h15-clang-wrapper-11.1.0/bin/clang++'
       >
       > <no location info>: error:
       >     loadArchive "/nix/store/l3lkdfm7sg1wwc850451cikqds766h15-clang-wrapper-11.1.0/bin/clang++": failed
       > /nix/store/1k85b7faqm8pc0k3y553zr8bfhb2dv2z-ghc-9.4.6/bin/ghc returned
       > ExitFailure 1

@elopez
Copy link
Contributor Author

elopez commented Oct 25, 2023

Hi @msooseth! I tried building a static binary as follows on an M1 Mac and it built fine -- although the binary did not run, as it was not signed. I added an extra commit to implement signing now, and the resulting binary appears to execute fine at first glance. I don't have an Intel Mac to verify if it works over there.

hevm % nix  build .#redistributable --out-link hevmMacos
hevm % otool -L ./hevmMacos/bin/hevm
./hevmMacos/bin/hevm:
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.60.1)
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1.0.0)
        /usr/lib/libiconv.dylib (compatibility version 7.0.0, current version 7.0.0)
hevm % ./hevmMacos/bin/hevm version
0.51.3 [no git revision present]

I also moved those blocks around to reduce the diff noise as you requested.

Update: I tried the same process quoted above on Intel macOS and the resulting binary worked fine as well.

Copy link
Collaborator

@d-xo d-xo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh the codesigning change is awesome, thanks so much. This looks great in general.

@d-xo d-xo merged commit 90fabf0 into ethereum:main Oct 26, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants