Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 17, 2022

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.

A native macOS task does not aware of Linux container settings, and it
does not use the `depends_built_cache`.
@hebasto hebasto force-pushed the 220517-ci branch 3 times, most recently from f0b6f07 to 8c17f08 Compare May 17, 2022 19:22
@DrahtBot DrahtBot added the Tests label May 17, 2022
@maflcko
Copy link
Member

maflcko commented May 18, 2022

Would this actually prevent #24958? Wouldn't it silently "fail" green?

@hebasto
Copy link
Member Author

hebasto commented May 18, 2022

Would this actually prevent #24958? Wouldn't it silently "fail" green?

export BITCOIN_CONFIG="--with-gui --with-miniupnpc --with-natpmp --enable-reduce-exports"

It will fail due to

bitcoin/configure.ac

Lines 1751 to 1757 in f7a1e67

dnl Enable NAT-PMP support.
AC_MSG_CHECKING([whether to build with support for NAT-PMP])
if test "$have_natpmp" = "no"; then
if test "$use_natpmp" = "yes"; then
AC_MSG_ERROR([NAT-PMP requested but cannot be built. Use --without-natpmp])
fi
AC_MSG_RESULT([no])

@Zero-1729
Copy link
Contributor

Concept ACK

@maflcko
Copy link
Member

maflcko commented May 18, 2022

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.

Copy link
Contributor

@promag promag left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice one 🤓

@hebasto
Copy link
Member Author

hebasto commented May 20, 2022

CPU overhead of the added CI task

I don't think the M1 CPU time is shared with any other task.

@maflcko
Copy link
Member

maflcko commented May 20, 2022

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?

@hebasto
Copy link
Member Author

hebasto commented May 20, 2022

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 x86_64:
image

But M1 limits differ:
image

@DrahtBot
Copy link
Contributor

DrahtBot commented May 23, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@maflcko
Copy link
Member

maflcko commented May 24, 2022

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.

@hebasto hebasto changed the title ci: Add "macOS 12 native Apple Silicon" task ci: Add "macOS 12 native arm64" task May 24, 2022
@hebasto
Copy link
Member Author

hebasto commented May 24, 2022

Updated f2e07bd -> 84cfbc7 (pr25160.02 -> pr25160.03, diff):

I am still wondering how many bugs this will catch that are not found by other tasks.

I hope every bug on the macOS arm64 platform, which has a steadily growing number of users, will be caught before release.

Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

crACK 84cfbc7

@maflcko
Copy link
Member

maflcko commented Jun 21, 2022

I hope every bug on the macOS arm64 platform, which has a steadily growing number of users, will be caught before release.

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.

@hebasto hebasto closed this Jun 21, 2022
@maflcko
Copy link
Member

maflcko commented Jun 22, 2022

I guess the 1st+3rd commit could still make sense. Then I could add it to my btc_nightly CI run, if you want.

@hebasto
Copy link
Member Author

hebasto commented Jun 22, 2022

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.

@hebasto
Copy link
Member Author

hebasto commented Jun 22, 2022

Btw, it seems Cirrus Labs stopped updating macOS images for x86_64. At least, image: monterey-xcode-13.4 or image: monterey-xcode-13.4.1 fail, although, they are available for arm64.

My question remained unanswered.

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jun 22, 2022
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
@maflcko
Copy link
Member

maflcko commented Jun 22, 2022

if amd64 is unsupported, we should probably remove it and just switch to arm64?

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 22, 2022
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
maflcko pushed a commit that referenced this pull request Oct 27, 2022
…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
@hebasto hebasto deleted the 220517-ci branch December 3, 2022 18:19
@bitcoin bitcoin locked and limited conversation to collaborators Dec 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants