build: replace custom `MAC_OSX` macro with existing `__APPLE_

l0rinc commented at 1:18 pm on February 19, 2024: contributor

This PR aims to standardize and simplify macOS-specific checks within our codebase by replacing the custom-defined MAC_OSX macro with the existing __APPLE__macro, defined in e.g. https://sourceforge.net/p/predef/wiki/OperatingSystems/#macos

We already use __APPLE__ in our own codebase for e.g. https://github.com/bitcoin/bitcoin/blob/master/src/crypto/sha256.cpp#L22

Local Verification confirms that MAC_OSX isn’t defined, but __APPLE__ is:

0% echo | cpp -dM | egrep 'MAC_OSX|__MACOS__|__APPLE__'
1#define __APPLE__ 1

DrahtBot commented at 1:18 pm on February 19, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

maflcko commented at 1:24 pm on February 19, 2024: member

Missing build: prefix in title?

l0rinc renamed this:
~~Replace Custom `MAC_OSX` Macro with Standard `__APPLE__` for Consistent macOS Detection~~
build: Replace Custom `MAC_OSX` Macro with Standard `__APPLE__` for Consistent macOS Detection
on Feb 19, 2024

DrahtBot added the label Build system on Feb 19, 2024

maflcko added the label DrahtBot Guix build requested on Feb 19, 2024

DrahtBot commented at 6:55 am on February 20, 2024: contributor

Guix builds (on x86_64)

File	commit c265aad5b52bf7b1b1e3cc38d04812caa001ba76(master)	commit e40066f96bc7331c32fa69c273299dcb831b5239(master and this pull)
SHA256SUMS.part	`6f821214728e3547...`	`ab75f4bbc3127423...`
*-aarch64-linux-gnu-debug.tar.gz	`1be67882ac17b9d8...`	`adf3f4e7dd456df9...`
*-aarch64-linux-gnu.tar.gz	`3b63efdbdbfebebc...`	`3651d345f6582ae2...`
*-arm-linux-gnueabihf-debug.tar.gz	`de9aed501d377fa8...`	`824943a6a673b74d...`
*-arm-linux-gnueabihf.tar.gz	`aaa813eed959c112...`	`886badd580265254...`
*-arm64-apple-darwin-unsigned.tar.gz	`36b16123ece970b8...`	`5b29c119f906f7c3...`
*-arm64-apple-darwin-unsigned.zip	`b8de9375e9ea3ff4...`	`88304a71e6bf1256...`
*-arm64-apple-darwin.tar.gz	`bbb0336cf64eca66...`	`60959eff9ec8b89a...`
*-powerpc64-linux-gnu-debug.tar.gz	`cda7885c51e49cad...`	`d6953c662907579c...`
*-powerpc64-linux-gnu.tar.gz	`f351af5d4966e5a7...`	`d6873a7608f3c7ac...`
*-powerpc64le-linux-gnu-debug.tar.gz	`1237e5cd283def85...`	`10711c142e8b24ba...`
*-powerpc64le-linux-gnu.tar.gz	`5fa6b8a31a28c43b...`	`a80465957dd19cc3...`
*-riscv64-linux-gnu-debug.tar.gz	`fcf0051066f424f3...`	`b2c2e7864ba648f8...`
*-riscv64-linux-gnu.tar.gz	`effb85105b1b6614...`	`3e74191c0d576c21...`
*-x86_64-apple-darwin-unsigned.tar.gz	`36c31f5c2a780e02...`	`0f4ed2e4c58f31b1...`
*-x86_64-apple-darwin-unsigned.zip	`17a933ddd37b77b2...`	`2b66bdc582f65a48...`
*-x86_64-apple-darwin.tar.gz	`5e12c8265fb50774...`	`1f810b70d7fc9360...`
*-x86_64-linux-gnu-debug.tar.gz	`f41057edc7b8fd88...`	`1b71b8c5e89a5a46...`
*-x86_64-linux-gnu.tar.gz	`179c0b93c0e2c0d8...`	`7dc2d66ce3aeb99a...`
*.tar.gz	`a7ecb9c3494c0b3c...`	`5751750368d1c91c...`
guix_build.log	`983a04a8e5e518d2...`	`ab792a118d54d4be...`
guix_build.log.diff		`bad60b7da3bc5af7...`

DrahtBot removed the label DrahtBot Guix build requested on Feb 20, 2024

l0rinc renamed this:
~~build: Replace Custom `MAC_OSX` Macro with Standard `__APPLE__` for Consistent macOS Detection~~
build: replace Custom `MAC_OSX` Macro with Standard `__APPLE__` for Consistent macOS Detection
on Feb 20, 2024

l0rinc renamed this:
~~build: replace Custom `MAC_OSX` Macro with Standard `__APPLE__` for Consistent macOS Detection~~
build: replace custom `MAC_OSX` macro with standard `__APPLE__` for consistent macOS detection
on Feb 20, 2024

fanquake commented at 3:27 pm on February 20, 2024: member

This is MAC_OSX because that is what we actually mean, and using our own define gives us more control. We produce release binaries for macOS running on darwin hardware, not just any “__APPLE__ target”, which, for example, would include iOS running on darwin hardware (i.e arm64e-apple-ios).

Swapping to just use __APPLE__ would mean it’s no-longer as easy to differentiate the two. At this point, that may no-longer matter, but there is code in this codebase that is relevant for macOS, but not for iOS, for example: https://github.com/bitcoin/bitcoin/pull/29450/files#diff-820710709ad533b4feb014bcc62b8cef5158a52b7e9690af995380a23499b5e3R185-R191.

l0rinc commented at 3:58 pm on February 20, 2024: contributor

Hey @fanquake, thanks for your input. How do you suggest we unify this in the codebase, since it’s not used consistently currently? This change could level the field before branching off to properly differentiate between the cases.

Empact commented at 6:52 am on February 27, 2024: member

Fyi the current uses of __APPLE__ in the codebase are from external libs included directly into the codebase as subtrees, see get_subtrees for a list of the related dirs. tinyformat.h is a special case which is included as a single file. @paplorinc

Which is not to say this change is incorrect (I’m not sure the distinction is meaningful in our case), but that the apparent inconsistency could be misleading.

l0rinc commented at 10:19 am on February 27, 2024: contributor

Thanks @Empact, I think we can ignore the internal consistency argument, and the change would still make the build more maintainable - do you agree?

Empact commented at 6:01 pm on February 27, 2024: member

I’m +0 on this change but not particularly familiar with the scope and variety of our Apple-related builds.

DrahtBot added the label Needs rebase on Mar 1, 2024

l0rinc force-pushed on Mar 9, 2024

DrahtBot removed the label Needs rebase on Mar 9, 2024

theuni commented at 5:43 pm on April 5, 2024: member

I recently did the same thing in one of my branches while working to remove the remaining autoconf vars from crypto/: https://github.com/theuni/bitcoin/commit/3646f76688e289774cce45e8f7a9967ee570ddda .

I think this is the right thing to do. I just checked locally and this applies to iOS as well: clang -target arm64-apple-ios test.cpp --sysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.4.sdk -o testing

Where test.cpp is:

 0#include <sys/types.h>
 1#include <sys/sysctl.h>
 2int main()
 3{
 4    int val = 0;
 5    int have_arm_shani;
 6    size_t len = sizeof(val);
 7    if (sysctlbyname("hw.optional.arm.FEAT_SHA256", &val, &len, nullptr, 0) == 0) {
 8        have_arm_shani = val != 0;
 9    }
10    return have_arm_shani;
11}

And it compiles fine.

So in this case, we didn’t actually mean macOS, we meant apple.

theuni commented at 5:44 pm on April 5, 2024: member

Whoops, sorry. I spoke too fast. This is the right thing to do for sha256.cpp. I’d have to look into the rest in detail, but I’d ACK that small change.

theuni commented at 8:12 am on April 9, 2024: member

@paplorinc I agree with @fanquake that some of these are macos specific.

Would you be interested in opening a new PR for just the crypto changes? Otherwise I will.

l0rinc commented at 9:25 am on April 9, 2024: contributor

@theuni, I’ve split it out the crypto package to #29834, thanks for checking it!

fanquake referenced this in commit 0b4218e43c on Apr 9, 2024

l0rinc force-pushed on Apr 9, 2024

l0rinc commented at 3:12 pm on April 9, 2024: contributor

Rebased after the #29834 merge, please let me know what to change or verify here.

DrahtBot added the label Needs rebase on Apr 25, 2024

l0rinc force-pushed on Apr 26, 2024

DrahtBot removed the label Needs rebase on Apr 26, 2024

l0rinc force-pushed on May 11, 2024

l0rinc force-pushed on May 29, 2024

hebasto added the label Needs CMake port on Aug 16, 2024

l0rinc force-pushed on Aug 30, 2024

l0rinc renamed this:
~~build: replace custom `MAC_OSX` macro with standard `__APPLE__` for consistent macOS detection~~
build: replace custom `MAC_OSX` macro with existing `__APPLE__`
on Aug 30, 2024

l0rinc commented at 10:37 am on August 30, 2024: contributor

Rebased and adjusted to the CMake build following @hebasto’s Needs CMake port label.

maflcko removed the label Needs CMake port on Aug 30, 2024

maflcko added the label DrahtBot Guix build requested on Aug 30, 2024

DrahtBot commented at 11:36 am on August 31, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 96b0a8f858ab24f3672360b8c830553b963de726(master)	commit d26232ebafc9176bf4a9a284b8abaac6103119c1(master and this pull)
SHA256SUMS.part	`27365d9d56b05cce...`	`23f769565a1f6705...`
*-aarch64-linux-gnu-debug.tar.gz	`04a5bb232114f782...`	`043b8ad9a8e48904...`
*-aarch64-linux-gnu.tar.gz	`bd5d23a64ab7d779...`	`712d59c593efe503...`
*-arm-linux-gnueabihf-debug.tar.gz	`0021ca776f7dcc8b...`	`41afb2208357ebc2...`
*-arm-linux-gnueabihf.tar.gz	`2340d1119a1ec632...`	`6c43dcca66104a1c...`
*-arm64-apple-darwin-unsigned.tar.gz	`ab00b80f00cd09b8...`	`592c93b2efca91b3...`
*-arm64-apple-darwin-unsigned.zip	`cb84320f60e26fed...`	`bcf00da332858f45...`
*-arm64-apple-darwin.tar.gz	`a1a2786cfcecd350...`	`c1923d35df593f9e...`
*-powerpc64-linux-gnu-debug.tar.gz	`9544adef5060dfab...`	`44ef4a95bc0df38b...`
*-powerpc64-linux-gnu.tar.gz	`faf390f38f51cc68...`	`673b88440299f599...`
*-riscv64-linux-gnu-debug.tar.gz	`67b921646742efc8...`	`a5b5a3c189862f92...`
*-riscv64-linux-gnu.tar.gz	`c4759b761c171f92...`	`baa1c008c6062281...`
*-x86_64-apple-darwin-unsigned.tar.gz	`459ba0d98d89d678...`	`d9c610e3f99c506c...`
*-x86_64-apple-darwin-unsigned.zip	`d19b0e188ac78eb6...`	`7daca194a3fda375...`
*-x86_64-apple-darwin.tar.gz	`7b3f41e90375e361...`	`83926aee65beafdb...`
*-x86_64-linux-gnu-debug.tar.gz	`a1c953af5210e0d6...`	`0a3774107b8134f1...`
*-x86_64-linux-gnu.tar.gz	`5d74858c83c440f2...`	`f97331986c3bc9c9...`
*.tar.gz	`b3128d336e5c9680...`	`3b91c398989b85b2...`
guix_build.log	`3e54d141b4a84788...`	`0ba2e46142bdfc2a...`
guix_build.log.diff		`0f1baed7d663ef4a...`

DrahtBot removed the label DrahtBot Guix build requested on Aug 31, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

build: Replace MAC_OSX macro with existing __APPLE__

Adopting `__APPLE__` aligns our project with broader industry practices, including those in prominent projects such as the Linux kernel (and even our own code).

See: https://sourceforge.net/p/predef/wiki/OperatingSystems/#macos

e775765b28

l0rinc force-pushed on Sep 3, 2024

DrahtBot removed the label Needs rebase on Sep 3, 2024

build: replace custom MAC_OSX macro with existing __APPLE__ #29450

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

build: replace custom `MAC_OSX` macro with existing `APPLE` #29450