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

fixing the MacOS build #2586

Merged
merged 23 commits into from
May 31, 2024
Merged

fixing the MacOS build #2586

merged 23 commits into from
May 31, 2024

Conversation

winitzki
Copy link
Collaborator

@winitzki winitzki commented May 12, 2024

The goal of this PR is to fix any problems with the build of dhall-haskell.

Current issues:

  • Mac OS build failing on Sonoma 14.4.1 (Intel x86_64) due to failures with building saltine after stack build.
  • Mac OS build failing due to libsodium in the PR optimize Natural/fold in the strict case #2585 (but builds are successful on other OSes)

The error is in installing libsodium.

tried: '/opt/homebrew/lib/libsodium.dylib' (mach-o file, but is an incompatible architecture (have 'arm64', need 'x86_64'))

This discussion https://stackoverflow.com/questions/72970363 suggests using the command: arch -x86_64 /usr/local/bin/brew install libsodium

@winitzki winitzki changed the title no-op change just to test the build fixing the MacOS build May 14, 2024
@winitzki
Copy link
Collaborator Author

Currently the macos build seems to be broken. I've been trying randomly poking around to fix it but I don't really know what I'm doing in this build system. #2586 Anyone has any ideas? The failure is around libsodium and has to do with macos architectures (x86 vs arm).

The error seems to be confusingly worded. The error message says that it found libsodium with arm64 architecture but it requires libsodium with x86 architecture. However, it is wrong that it should have looked for libsodium with x86 architecture, because the build is for macos-latest which is arm64. See here for the list. https://github.com/actions/runner-images?tab=readme-ov-file#available-images

It seems that macos-latest is configured to build arm64. However, I am also not able to change that into building x86.

What I have tried so far:

  • Adding a command to install libsodium (brew install libsodium). This did not help because libsodium was already installed by another command somewhere.
  • Adding a command to install libsodium on mac (brew reinstall libsodium) with x86 architecture specifically. This fails because brew is installed for arm64 and refuses to install x86 artifacts. I don't know how I can reinstall brew correctly.
  • Changing the build in main.yaml to use a different macos runner: macos-latest-large, macos-14-large and so on. This simply fails even to start the build, with no error messages.

@winitzki
Copy link
Collaborator Author

The build fails immediately if the runner is set to macos-14, macos-14-large, macos-latest-large. (Although those runners are supposed to be working now. https://github.com/actions/runner-images?tab=readme-ov-file#available-images )

The previous working version was macos-13 and I'm trying it right now. However, the github settings for this repo require macOS-latest build to succeed.

@winitzki winitzki requested a review from mmhat May 27, 2024 07:02
@winitzki
Copy link
Collaborator Author

winitzki commented May 27, 2024

I found this page:
https://docs.github.com/en/actions/using-github-hosted-runners/about-larger-runners/about-larger-runners#about-macos-larger-runners
which says:

Larger runners are only available for organizations and enterprises using the GitHub Team or GitHub Enterprise Cloud plans.
Probably, this is why the CI job simply fails immediately when I specified macos-14-large or other "large" runners.

So, we have a choice:

  • Keep the macos build at version 13 (this still works) and change the github repository settings for dhall-lang/dhall-haskell to require macos-13 build to pass instead of macos-latest.
  • Somehow fix the macOS ARM64 build. Currently, the ARM64 build fails because of some dependency problems with hnix-store that depends on libsodium and appears to be misconfigured: when building hnix-store on ARM64 architecture, the build expects libsodium to be for the x86 architecture. I don't know how to fix that. There seems to be some dependency issues for hnix-store.

@winitzki winitzki requested a review from Gabriella439 May 27, 2024 07:28
@mmhat
Copy link
Collaborator

mmhat commented May 30, 2024

@winitzki Thank's for your efforts; I am trying to understand why this happens and here is what I have found so far:

I suspect the reason is that GH changed the runners to arm64 only: actions/runner-images#9741 (comment)
Their issue tracker has a bunch of interesting issues in this regard, involving caching issues
(e.g. actions/runner-images#9741 (comment)), wrong uname -m output (actions/runner-images#9755 (comment)).
Apparently Homebrew choked on that change as well (actions/runner-images#9760), and there are some more interesting issues here:
actions/runner-images#9766
actions/runner-images#9676

I also re-run an old GH action of this PR with debug logging enabled:
https://github.com/dhall-lang/dhall-haskell/actions/runs/9049961867/job/25621271094
There is something weird going on: For example https://github.com/dhall-lang/dhall-haskell/actions/runs/9049961867/job/25621271094#step:3:73 indicates that we are running on arm64 architecture (which is correct), and then a few lines later (e.g. https://github.com/dhall-lang/dhall-haskell/actions/runs/9049961867/job/25621271094#step:3:117) ghcup fetches and installs an x86_64 bindist of GHC.
Later, stack appears to build x86_64 objects as well (See e.g. https://github.com/dhall-lang/dhall-haskell/actions/runs/9049961867/job/25621271094#step:6:2309).

Copy link
Collaborator

@mmhat mmhat left a comment

Choose a reason for hiding this comment

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

I think we should pin the MacOS runner first in order to get working mac builds again, but #2587 should stay open and the subject should be adjusted. Thought?

@@ -49,7 +49,7 @@ jobs:
${{ matrix.os.runner }}-
path: |
${{ steps.setup-haskell-cabal.outputs.stack-root }}
- name: Install libsodium
- name: Install libsodium x86_64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- name: Install libsodium x86_64
- name: Install libsodium

@mmhat
Copy link
Collaborator

mmhat commented May 30, 2024

@winitzki It might be worth trying to use the latest haskell setup action -- Which is haskell-actions/setup@v2.7.

The various "Downloading ..." debug messages (e.g. https://github.com/dhall-lang/dhall-haskell/actions/runs/9049961867/job/25621271094#step:3:465) in the GH Actions run linked previously originate from the haskell setup action: See https://github.com/haskell-actions/setup/blob/33585e1a16afa5875e124b0ebc89dd0c2f872c21/dist/index.js#L4523 in the definition and https://github.com/haskell-actions/setup/blob/33585e1a16afa5875e124b0ebc89dd0c2f872c21/src/installer.ts#L343 for the usage of the downloadTool function.

@winitzki
Copy link
Collaborator Author

I think we should pin the MacOS runner first in order to get working mac builds again, but #2587 should stay open and the subject should be adjusted. Thought?

In my view, the root cause is that dhall-haskell cannot build under macos-arm64 due to some dependency problems in hnix-store and/or elsewhere. The macos-arm64 build was actually never tested in dhall-haskell, and in fact there were never any macos-arm64 executables released for dhall. When github changed the macos-latest runners to arm64, all builds started failing.

To resolve this issue, I propose the following path forward:

  1. Use macos-13 instead of macos-latest (as in this PR). Change the repo settings so that macos-latest is no longer a requirement for passing the CI builds. (I don't have permissions to do this.) Mention in the README that macos-arm64 is currently unsupported.
  2. Fix the dependency problems so that macos-arm64 build becomes successful. Enable macos-13 and macos-latest builds and publish both dhall-darwin-x86 and dhall-darwin-arm64 executables. Mention in the README that macos-arm64 is now supported.

@winitzki
Copy link
Collaborator Author

winitzki commented May 31, 2024

The macos-latest build passed with haskell actions 2.7. Thank you for suggesting that!

Let me add a macos-13 build as well.

@winitzki
Copy link
Collaborator Author

winitzki commented May 31, 2024

Let me know if this is OK to merge. This will unblock other builds.

What needs to be done in order to publish both macos-x86 and macos-arm64 executables when a release is made?

@mmhat
Copy link
Collaborator

mmhat commented May 31, 2024

I think we should pin the MacOS runner first in order to get working mac builds again, but #2587 should stay open and the subject should be adjusted. Thought?

In my view, the root cause is that dhall-haskell cannot build under macos-arm64 due to some dependency problems in hnix-store and/or elsewhere. The macos-arm64 build was actually never tested in dhall-haskell, and in fact there were never any macos-arm64 executables released for dhall. When github changed the macos-latest runners to arm64, all builds started failing.

Indeed.

To resolve this issue, I propose the following path forward:

  1. Use macos-13 instead of macos-latest (as in this PR). Change the repo settings so that macos-latest is no longer a requirement for passing the CI builds. (I don't have permissions to do this.) Mention in the README that macos-arm64 is currently unsupported.

Would you mind adding that comment to the README?

  1. Fix the dependency problems so that macos-arm64 build becomes successful. Enable macos-13 and macos-latest builds and publish both dhall-darwin-x86 and dhall-darwin-arm64 executables. Mention in the README that macos-arm64 is now supported.

👍

Let me know if this is OK to merge. This will unblock other builds.

LGTM, merge it when you're done. If you include bigger changes, don't hesitate to ask for another review!

What needs to be done in order to publish both macos-x86 and macos-arm64 executables when a release is made?

I'd proceed as follows:

  1. Parameterize the objects of the workflow with runner.arch from https://docs.github.com/en/actions/learn-github-actions/contexts#runner-context. In particular
    name: ${{ matrix.os.runner }} - ${{ matrix.stack-yaml }}
    ,
    key: ${{ matrix.os.runner }}-${{ hashFiles(matrix.stack-yaml) }}-${{ env.cache_generation }}
    (maybe the restore-key: too) and
    name: 'dhall-${{runner.os}}.${{matrix.os.file-extension}}'
  2. Also use runner.arch in the conditional executions here:
    if [ '${{matrix.os.runner}}' == 'windows-latest' ] && [ "${package}" == 'dhall-nix' ]; then
    ,
    if [ '${{matrix.os.runner}}' == 'windows-latest' ] && [ "${package}" == 'dhall-nix' ]; then
    ,
    if [ '${{matrix.os.runner}}' != 'windows-latest' ]; then
  3. I think it is good to get rid of
    architecture="$(uname -m)"
    and use runner.arch in
    local package_file="${package}-${version}-${architecture}-${{runner.os}}.${{ matrix.os.file-extension }}"
    .

@winitzki
Copy link
Collaborator Author

@mmhat The current build with macOS-latest is an arm64 build, correct?

https://github.com/dhall-lang/dhall-haskell/actions/runs/9316129247/job/25643704112?pr=2586

Current runner version: '2.316.1'
Operating System
  macOS
  14.5
  23F79
Runner Image
  Image: macos-14-arm64
  Version: 20240526.2
  Included Software: https://github.com/actions/runner-images/blob/macos-14-arm64/20240526.2/images/macos/macos-14-arm64-Readme.md
  Image Release: [https://g](https://github.com/actions/runner-images/releases/tag/macos-14-arm64%2F20240526.2)

So, it looks like macos-arm64 build is working and passing all tests. I don't think we need to say in the readme that it is unsupported. Should we?

@winitzki winitzki merged commit 1088e3c into dhall-lang:main May 31, 2024
7 checks passed
@winitzki winitzki deleted the feature/test-build branch May 31, 2024 12:45
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