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

Bump macOS GitHub CI runner version #6006

Merged
merged 3 commits into from
May 22, 2024

Conversation

knelli2
Copy link
Contributor

@knelli2 knelli2 commented May 17, 2024

Proposed changes

Currently just using this draft PR to see if this works since macos-11 will be deprecated at the end of next month. macos-13 is the last version still on intel chips. macos-14 and macos-latest are on M1 chips

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@nilsvu
Copy link
Member

nilsvu commented May 18, 2024

See also commits in #5973

Old versions can be hard to install because pip has to
build them from source.
macos-11 is deprecated by GitHub. It's not supported by Homebrew anymore
either, so fetching dependencies ran into issues.
@knelli2 knelli2 marked this pull request as ready for review May 22, 2024 15:25
@knelli2
Copy link
Contributor Author

knelli2 commented May 22, 2024

Ok @sxs-collaboration/spectre-core-devs I finally got this to work. These are the minimal changes to get macOS CI back up and running. I'd like to do this first before we do anything more fancy like in #5973.

Copy link
Member

@nilsdeppe nilsdeppe left a comment

Choose a reason for hiding this comment

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

LGTM. I think macOS has been doing weird things with signals. E.g. FPEs raise sigill instead.

Separate: we should use 4 cores on Linux too since they seem to have increased what's available

Comment on lines +24 to +28
// For some reason on the newer macOS-13 CI runners, this test never prints out
// "Segmentation fault!". Somehow, the CHECK_THROWS_WITH must catch the signal
// and exit cleanly if the test is working. Because of this, the test fails
// because this is an output test. So we manually print out the message so the
// output test is happy.
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like the test might actually work as intended on Mac but not Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think mac does something different when handling signals vs exceptions. Because in the catch docs it says all the CHECK_(NO)THROW* macros handle exceptions and doesn't mention signals. So my best guess is that on linux this test works as intended because CATCH_THROWS_WITH doesn't catch the SIGSEGV, so the test "fails" and our output test catches that and reports that the test actually passes. However, on mac, somehow CHECK_THROWS_WITH catches SIGSEGV so the test "succeeds" and then our output test actually fails because no message was printed.

Copy link
Member

Choose a reason for hiding this comment

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

We throw an exception in the signal handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... Then I have no clue 🤷🏻

@wthrowe wthrowe merged commit b27d027 into sxs-collaboration:develop May 22, 2024
22 checks passed
@nilsvu nilsvu added the ci/cd Continuous integration & deployment label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd Continuous integration & deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants