Skip to content

CI: add macos-15-large #63

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.

Sign up for GitHub

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

Merged
jandubois merged 1 commit into from
Nov 25, 2024
Merged

CI: add macos-15-large #63

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,24 @@ permissions:
contents: read

jobs:
integration:
name: Integration tests
build:
name: Build
strategy:
fail-fast: false
matrix:
# macos-13-large is used as macos-13 seems too flaky.
# macos-14 (ARM) and later cannot be used for the most part of the job
# due to the lack of the support for nested virt.
#
# TODO: add macos-15-large https://github.com/lima-vm/socket_vmnet/pull/63
platform: [macos-13-large, macos-14-large]
platform: [macos-13, macos-14, macos-15]
Copy link
Copy Markdown
Member

@nirs nirs Nov 23, 2024

Choose a reason for hiding this comment

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

This build only on aarch64, previously we built only on x86_64. We don't wan to test build on both?

Copy link
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda Nov 25, 2024

Choose a reason for hiding this comment

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

Intel is covered in the integration test

Copy link
Copy Markdown
Member

@nirs nirs Nov 25, 2024

Choose a reason for hiding this comment

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

This will be easier to maintain if we use:

platform:
- macos-13
- macos-14
- macos-15

runs-on: ${{ matrix.platform }}
timeout-minutes: 10
steps:
- uses: actions/checkout@eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871 # v4.2.1
with:
fetch-depth: 1
- name: Build
run: make
integration:
name: Integration tests
# Only a single version of macOS is used for running integration tests, due to the flakiness of the CI.
Copy link
Copy Markdown
Member

@nirs nirs Nov 23, 2024

Choose a reason for hiding this comment

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

Good idea but better to add macos-15 and change the build in separate commits.

Copy link
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda Nov 25, 2024

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Member

@nirs nirs Nov 25, 2024

Choose a reason for hiding this comment

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

This change is doing 2 logical changes - adding new plafrom for testing and removing older platform. We can do this separately but it works as is.

runs-on: macos-15-large # Intel (supports nested virtualization)
timeout-minutes: 40
Copy link
Copy Markdown
Member

@nirs nirs Nov 23, 2024

Choose a reason for hiding this comment

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

The build takes 9 minutes now (with #84) - so we can use much smaller timeout now, maybe 15 minutes?

steps:
- uses: actions/checkout@eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871 # v4.2.1
Copy link
Copy Markdown
Member

@nirs nirs Nov 23, 2024

Choose a reason for hiding this comment

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

This compiles both for x86_64 and aarch64, but we test only on x86_64 - any reason to build twice when we test the build in the build job?

Copy link
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda Nov 25, 2024

Choose a reason for hiding this comment

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

For testing reproducibility on Intel

Copy link
Copy Markdown
Member

@nirs nirs Nov 25, 2024

Choose a reason for hiding this comment

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

OK, makes sense, this is also very quick.

Expand Down