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
ACK	fanquake
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: contributor

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: contributor

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

l0rinc force-pushed on Sep 3, 2024

DrahtBot removed the label Needs rebase on Sep 3, 2024

achow101 requested review from theuni on Oct 15, 2024

fanquake commented at 10:09 am on October 24, 2024: member

@l0rinc can you rebase this?

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

6c6b2442ed

l0rinc force-pushed on Oct 24, 2024

l0rinc commented at 10:33 am on October 24, 2024: contributor

@l0rinc can you rebase this?

Rebased, checked for leftover MAC_OSX (found none)

maflcko added the label DrahtBot Guix build requested on Oct 24, 2024

fanquake commented at 12:24 pm on October 24, 2024: member

Guix Build:

 0245051dc0f18c4da7060996df1ce9eb298b449e48b62cbad30017707c94faed2  guix-build-6c6b2442edab/output/aarch64-linux-gnu/SHA256SUMS.part
 155b4b8b790069826c0564c1d3372fada3c8e71dc1a172f2c360aa74c7c041ef1  guix-build-6c6b2442edab/output/aarch64-linux-gnu/bitcoin-6c6b2442edab-aarch64-linux-gnu-debug.tar.gz
 2133bdd966b70875c8aa6b62eed27235fc327dfeaa8e048db524ec9ccf6085602  guix-build-6c6b2442edab/output/aarch64-linux-gnu/bitcoin-6c6b2442edab-aarch64-linux-gnu.tar.gz
 3f34455a7dc5581bb876231671afa9c30df0376fe00bf2d12eed0b55f4b30c404  guix-build-6c6b2442edab/output/arm-linux-gnueabihf/SHA256SUMS.part
 4360f58a35ed523c259655e5b52ad3d93fc0d2571121ad2a922ef63b7c6c4b1a4  guix-build-6c6b2442edab/output/arm-linux-gnueabihf/bitcoin-6c6b2442edab-arm-linux-gnueabihf-debug.tar.gz
 5a6218bb1d3cb64fbd723edc363b00d7e61d548d611edae5228378329dc41c7e1  guix-build-6c6b2442edab/output/arm-linux-gnueabihf/bitcoin-6c6b2442edab-arm-linux-gnueabihf.tar.gz
 6187a912a2a1c1a815058c5a276c78cc07308ec011090b00f02cba5cae5f2b0ee  guix-build-6c6b2442edab/output/arm64-apple-darwin/SHA256SUMS.part
 7c0dcf923e7c3ff150aa44dac2e186d620ee90ac5fc741b22110576ac6629acd5  guix-build-6c6b2442edab/output/arm64-apple-darwin/bitcoin-6c6b2442edab-arm64-apple-darwin-unsigned.tar.gz
 8a25b06d388045b828f9a55c4feafa1c0bc59c7fd6836c89f5c4c0ff829246ced  guix-build-6c6b2442edab/output/arm64-apple-darwin/bitcoin-6c6b2442edab-arm64-apple-darwin-unsigned.zip
 94a1145119ed52441e3a363be4c8ab334fea6fd891da9b026c607f5484f70a826  guix-build-6c6b2442edab/output/arm64-apple-darwin/bitcoin-6c6b2442edab-arm64-apple-darwin.tar.gz
10f9506a3fe5890ce3a1827f523655f9f5c088c2dba94139f3750c322c40c5c81e  guix-build-6c6b2442edab/output/dist-archive/bitcoin-6c6b2442edab.tar.gz
11a83ade6be06651a80f95fa66df1a7a5aacf21eed0f05c7d7546fa4c8c67d78aa  guix-build-6c6b2442edab/output/powerpc64-linux-gnu/SHA256SUMS.part
12b395cfb458499059b13ff71b4dd8519a254dc21bec6155fe481a11342a0db2fe  guix-build-6c6b2442edab/output/powerpc64-linux-gnu/bitcoin-6c6b2442edab-powerpc64-linux-gnu-debug.tar.gz
1300e44a878f20cea0d265afc65d8f43febc4835f0e76db3e1e052bd3bab093279  guix-build-6c6b2442edab/output/powerpc64-linux-gnu/bitcoin-6c6b2442edab-powerpc64-linux-gnu.tar.gz
1464634931286dac6b138cec0ffb45fc7324c880ef7c5c3ce358991a6c89717249  guix-build-6c6b2442edab/output/riscv64-linux-gnu/SHA256SUMS.part
1511822a41c7fe8e754899be924b2d2d15987dddd74660a96400935bfc9a9d76c0  guix-build-6c6b2442edab/output/riscv64-linux-gnu/bitcoin-6c6b2442edab-riscv64-linux-gnu-debug.tar.gz
166d6a070afe184ef4e3c57179b629f36daa58735506f90c33cb0df10b02805315  guix-build-6c6b2442edab/output/riscv64-linux-gnu/bitcoin-6c6b2442edab-riscv64-linux-gnu.tar.gz
17752f60a689a036940a0a345626d8bdd1326790bc00c7bdaa9cefbba00b0d4a63  guix-build-6c6b2442edab/output/x86_64-apple-darwin/SHA256SUMS.part
1809ee27a6c6fb8fcf128c18443c6f073672651b73363e5c4fcec2fe000609dd6d  guix-build-6c6b2442edab/output/x86_64-apple-darwin/bitcoin-6c6b2442edab-x86_64-apple-darwin-unsigned.tar.gz
19e623227a1456a3ab5eec39a7b08918c5bae6a62e0d63d5a325da4193ab8bd3e9  guix-build-6c6b2442edab/output/x86_64-apple-darwin/bitcoin-6c6b2442edab-x86_64-apple-darwin-unsigned.zip
20933f64785d853e3818946688dd4356ffd0b2bf4f85e7cfaf335a4ef474d9f192  guix-build-6c6b2442edab/output/x86_64-apple-darwin/bitcoin-6c6b2442edab-x86_64-apple-darwin.tar.gz
21a2ce725d5e71b1622d9f5c33b264f5d67fc2f97d0dedbea2855dfaa4b44e4f28  guix-build-6c6b2442edab/output/x86_64-linux-gnu/SHA256SUMS.part
229f4f446611f2dd4f54ad0984483c5b7dd0b60d1f9fe1252ceea4ed1b07e9a868  guix-build-6c6b2442edab/output/x86_64-linux-gnu/bitcoin-6c6b2442edab-x86_64-linux-gnu-debug.tar.gz
239e83290babc2f15dfb19481dd33af31ac7ae6adc23df6887dcbae52457d463be  guix-build-6c6b2442edab/output/x86_64-linux-gnu/bitcoin-6c6b2442edab-x86_64-linux-gnu.tar.gz
24a45bc4958eae09a9a3fede0f64cd468c121d3d7fa1d5e614fe5f41cdd93ae653  guix-build-6c6b2442edab/output/x86_64-w64-mingw32/SHA256SUMS.part
25e0b5e0a8f3a3df304f608c2effc60bc954c1789924ce8bbc4ccc2b92c2ca0929  guix-build-6c6b2442edab/output/x86_64-w64-mingw32/bitcoin-6c6b2442edab-win64-debug.zip
26be64e323eb67131cc5194d346f73f8f3d3e4acf15d05eeb0f22140556743b058  guix-build-6c6b2442edab/output/x86_64-w64-mingw32/bitcoin-6c6b2442edab-win64-setup-unsigned.exe
279eb89a27a316c32a0ee89fa5f824605e347b5a2ee95cc70a454efdaf062d6708  guix-build-6c6b2442edab/output/x86_64-w64-mingw32/bitcoin-6c6b2442edab-win64-unsigned.tar.gz
28b5f40face90542333443b8a7851ac3424fa776e98a35d0dbf0afe7f04b72a023  guix-build-6c6b2442edab/output/x86_64-w64-mingw32/bitcoin-6c6b2442edab-win64.zip

fanquake approved

fanquake commented at 12:30 pm on October 24, 2024: member

ACK 6c6b2442edabe9e3f77d29aacd6681bc3fa16d9e - at this point it seems unlikely that we’ll need to accomodate non-macOS Apple, so consolidating to __APPLE__ seems ok for now.

fanquake merged this on Oct 24, 2024

fanquake closed this on Oct 24, 2024

maflcko removed the label DrahtBot Guix build requested on Oct 24, 2024

TheCharlatan commented at 2:59 pm on October 24, 2024: contributor

Post-merge and Guix build reproduced ACK 6c6b2442edabe9e3f77d29aacd6681bc3fa16d9e

l0rinc deleted the branch on Oct 24, 2024

DrahtBot commented at 7:53 pm on October 24, 2024: contributor

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

File	commit d94adc7270ba615599eef1e4af849568c155694e(master)	commit 3bc025dc37779125f066ba51874712b3608584c3(master and this pull)
SHA256SUMS.part	`c713ef1f07ff0632...`	`9e91f78777fe09d3...`
*-aarch64-linux-gnu-debug.tar.gz	`a6f3e8470f058065...`	`c8943b2b757a3853...`
*-aarch64-linux-gnu.tar.gz	`5852fdab3138df32...`	`68fb3d2b2aa09603...`
*-arm-linux-gnueabihf-debug.tar.gz	`14b241555c922dab...`	`c7455e4004d4b447...`
*-arm-linux-gnueabihf.tar.gz	`f8e867278d03db30...`	`b261dbadc48b5b39...`
*-arm64-apple-darwin-unsigned.tar.gz	`c5a762db769a16ca...`	`931d31e3a07b94f1...`
*-arm64-apple-darwin-unsigned.zip	`595806da4d708a77...`	`f3f7b5eac02ef77e...`
*-arm64-apple-darwin.tar.gz	`ef7f4226f6f002f4...`	`e931bfe8b84b24f2...`
*-powerpc64-linux-gnu-debug.tar.gz	`d2559d77d79b0c80...`	`780ad5d229ce2222...`
*-powerpc64-linux-gnu.tar.gz	`a28e29c5ad291e20...`	`e64dc7ee30cf0ef7...`
*-riscv64-linux-gnu-debug.tar.gz	`a985c88eba5f1425...`	`c14ddce3034b23d9...`
*-riscv64-linux-gnu.tar.gz	`e856f86bc9053316...`	`11bf995666565f17...`
*-x86_64-apple-darwin-unsigned.tar.gz	`fd5767f5ce6dbacd...`	`1e45e38f5557087b...`
*-x86_64-apple-darwin-unsigned.zip	`a24038e482ed756b...`	`210f3e7fba5c6528...`
*-x86_64-apple-darwin.tar.gz	`9012e956a46e2f64...`	`50ff9c08daf4d290...`
*-x86_64-linux-gnu-debug.tar.gz	`b32a3afd06d33771...`	`a57cf0c7a2640ffc...`
*-x86_64-linux-gnu.tar.gz	`67fa3d26ab7957a0...`	`24a2685e31142e51...`
*.tar.gz	`aff6b4dba5453528...`	`8e6975d22c1df446...`
guix_build.log	`6f5e9f32addd55da...`	`3ae4df0c35281ac1...`
guix_build.log.diff		`f94da22e4347cf98...`

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]

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