-
Notifications
You must be signed in to change notification settings - Fork 37.8k
ci: Add "macOS 12 native arm64" task #25160
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
Conversation
A native macOS task does not aware of Linux container settings, and it does not use the `depends_built_cache`.
f0b6f07
to
8c17f08
Compare
Would this actually prevent #24958? Wouldn't it silently "fail" green? |
It will fail due to Lines 1751 to 1757 in f7a1e67
|
Concept ACK |
Not sure if it makes sense to run this check on every commit. There is some probability of breaking that is acceptable and unavoidable. I think we should focus on checks that are likely to catch errors often enough to be a net-positive considering the maintenance overhead and CPU overhead of the added CI task. So if #24958 is the only known bug this prevents, I tend toward NACK, even though the code looks good and correct. I expect that there are ppl running M1 locally, but if not, this could also be implemented as a nightly task to run outside this repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
sudo -H pip3 install --upgrade pip | ||
# shellcheck disable=SC2086 | ||
IN_GETOPT_BIN="/usr/local/opt/gnu-getopt/bin/getopt" ${CI_RETRY_EXE} pip3 install --user $PIP_PACKAGES | ||
IN_GETOPT_BIN="$(brew --prefix gnu-getopt)/bin/getopt" ${CI_RETRY_EXE} pip3 install --user $PIP_PACKAGES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 🤓
I don't think the M1 CPU time is shared with any other task. |
According to https://cirrus-ci.org/faq/#are-there-any-limits there is a limit of "12.0 CPUs macOS VM (1 VM)", so it will be shared with the other macos instance? |
It looks like docs are a bit inconsistent after introducing Apple Silicon support. They are correct about But M1 limits differ: |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
Ok, looks like it won't affect the limits for now. Though, I am still wondering how many bugs this will catch that are not found by other tasks. |
Updated f2e07bd -> 84cfbc7 (pr25160.02 -> pr25160.03, diff):
I hope every bug on the macOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK 84cfbc7
It only caught one bug for now, which doesn't seem enough to justify the maintenance burden? Note that in this repo, *bsd, s390x, various linux distros, valgrind, ... aren't tested either. |
I guess the 1st+3rd commit could still make sense. Then I could add it to my btc_nightly CI run, if you want. |
Done in #25444. |
0bb7a1f ci: Improve naming related to "macOS 12 native x86_64" task (Hennadii Stepanov) 8e017f3 ci, refactor: Add `MACOS_NATIVE_TASK_TEMPLATE` (Hennadii Stepanov) Pull request description: Split from bitcoin/bitcoin#25160 as [suggested](bitcoin/bitcoin#25160 (comment)). ACKs for top commit: MarcoFalke: ACK 0bb7a1f 🚘 Tree-SHA512: d50fe8a51a3364e76d1a5394f718e30bd2994ccdaa4bf73c017c5d81bff00539dcff1cd3879c8b4b6b442b7248b0aa6491489a27c6dd7ec1f3e976aa2a03c730
if amd64 is unsupported, we should probably remove it and just switch to arm64? |
0bb7a1f ci: Improve naming related to "macOS 12 native x86_64" task (Hennadii Stepanov) 8e017f3 ci, refactor: Add `MACOS_NATIVE_TASK_TEMPLATE` (Hennadii Stepanov) Pull request description: Split from bitcoin#25160 as [suggested](bitcoin#25160 (comment)). ACKs for top commit: MarcoFalke: ACK 0bb7a1f 🚘 Tree-SHA512: d50fe8a51a3364e76d1a5394f718e30bd2994ccdaa4bf73c017c5d81bff00539dcff1cd3879c8b4b6b442b7248b0aa6491489a27c6dd7ec1f3e976aa2a03c730
…ive" task da16893 ci: Use `macos-ventura-xcode:14.1` image for "macOS native" task (Hennadii Stepanov) 7028365 ci: Make `getopt` path architecture agnostic (Hennadii Stepanov) Pull request description: The "macOS native" CI task always uses the recent OS image. This PR updates it up to the recent macOS release. Cirrus Labs [stopped](#25160 (comment)) updating macOS images for `x86_64`, therefore, an `arm64` image been used. Also `make test-security-check` has been dropped as it ["isn't even expected to pass"](#26386 (comment)) on `arm64` in CI. ACKs for top commit: Sjors: utACK da16893 Tree-SHA512: 36785d33b7f11b3cdbc53bcfbf97d88bf821fad248c825982dd9f8e3413809a4ef11190eaf950e60fdf479b62ff66920c35d9ea42d534723f015742eec7e19b6
The release binaries for Apple Silicon macOS (
arm64-apple-darwin
) have been available since v23.0.It seems reasonable to add a task for such a platform to CI. Also it would prevent regressions like #24958.