build: Patch Qt to handle minimum macOS version properly #28775

hebasto · 2023-11-02T13:45:24Z

This PR is:

required to switch to macOS 14 SDK (Xcode 15).
an alternative to build: Bump native_clang up to 17.0.2 #28732.

This change is required to switch to macOS 14 SDK (Xcode 15).

DrahtBot · 2023-11-02T13:45:27Z

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.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28622 (build: use macOS 14 SDK (Xcode 15.0) by fanquake)
#21778 (build: LLD based macOS toolchain by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake · 2023-11-02T13:49:30Z

Can you explain the patch? What is the actual problem, and how does this fix it?
Seems a bit odd to have to patch version detection checks inside Qt, can the correct values not be passed in from the outside?

hebasto · 2023-11-02T13:59:23Z

Can you explain the patch? What is the actual problem, and how does this fix it? Seems a bit odd to have to patch version detection checks inside Qt, can the correct values not be passed in from the outside?

In macOS SDK, the problematic os/activity.h header relies on the OS_LOG_TARGET_HAS_10_12_FEATURES macro:

#if OS_LOG_TARGET_HAS_10_12_FEATURES
#ifndef OS_ACTIVITY_OBJECT_API
#define OS_ACTIVITY_OBJECT_API 1
#endif // OS_ACTIVITY_OBJECT_API
#else
#if OS_ACTIVITY_OBJECT_API
#error Please change your minimum OS requirements because OS_ACTIVITY_OBJECT_API is not available
#endif // OS_ACTIVITY_OBJECT_API
#define OS_ACTIVITY_OBJECT_API 0
#endif // OS_LOG_TARGET_HAS_10_12_FEATURES

That macro in turn is defined in the os/trace_base.h header:

#if __OS_LOG_OS_FEATURES_AT_LEAST(10_12, 10_0, 10_0, 3_0)
#define OS_LOG_TARGET_HAS_10_12_FEATURES 1
#else
#define OS_LOG_TARGET_HAS_10_12_FEATURES 0
#endif

which depends on the __MAC_OS_X_VERSION_MIN_REQUIRED macro value.

The patch modifies the only code in the whole Qt codebase where the __MAC_OS_X_VERSION_MIN_REQUIRED macro is defined. The other MAC_OS_X_VERSION_MIN_REQUIRED macro is adjusted for consistency.

fanquake · 2023-11-02T14:13:41Z

the problematic os/activity.h header

What is the problem with the header?

The (availability.h) macros are set via the -mmacosx-version-min flag (which we set):

For these macros to function properly, a program must specify the OS version range
it is targeting. The min OS version is specified as an option to the compiler:
-mmacosx-version-min=10.x when building for Mac OS X,

Why do we need to hardcode them to the values that they should already be being set too?

hebasto · 2023-11-02T14:27:12Z

the problematic os/activity.h header

What is the problem with the header?

The OS_ACTIVITY_OBJECT_API is set to 0, which makes a bunch of code unavailable, which in turn causes compile errors.

The (availability.h) macros are set via the -mmacosx-version-min flag (which we set):

For these macros to function properly, a program must specify the OS version range
it is targeting. The min OS version is specified as an option to the compiler:
-mmacosx-version-min=10.x when building for Mac OS X,

I know this. But I can't see in the code how the -mmacosx-version-min flag affect the __MAC_OS_X_VERSION_MIN_REQUIRED macro, which is the one that is evaluated in the os/activity.h header.

Also, there is a comment in the Availability.h header:

    It is also possible to use the *_VERSION_MIN_REQUIRED in source code to make one
    source base that can be compiled to target a range of OS versions.  It is best
    to not use the _MAC_* and __IPHONE_* macros for comparisons, but rather their values.
    That is because you might get compiled on an old OS that does not define a later
    OS version macro, and in the C preprocessor undefined values evaluate to zero
    in expresssions, which could cause the #if expression to evaluate in an unexpected
    way.
    
        #ifdef __MAC_OS_X_VERSION_MIN_REQUIRED
            // code only compiled when targeting Mac OS X and not iPhone
            // note use of 1050 instead of __MAC_10_5
            #if __MAC_OS_X_VERSION_MIN_REQUIRED < 1050
                // code in here might run on pre-Leopard OS
            #else
                // code here can assume Leopard or later
            #endif
        #endif

Why do we need to hardcode them to the values that they should already be being set too?

We don't introduce hardcoded values. We rather are adjusting the current inappropriate values, no?

fanquake · 2023-11-02T15:22:32Z

We rather are adjusting the current inappropriate values, no?

The default values shouldn't need adjusting in the qt source code, if our version setting works. So this would point to that not being the case?

The OS_ACTIVITY_OBJECT_API is set to 0, which makes a bunch of code unavailable, which in turn causes compile errors.

In that case, maybe it would be easier to set something like -DOS_ACTIVITY_OBJECT_API=1 for the Qt build, and not have to patch any source?

However that would still be confusing, because this build failure only happens when cross-compiling? Native builds (using the latest Xcode) are not broken in this way, and the minimum version handling shouldn't differ between the two?

DrahtBot · 2023-11-02T21:26:06Z

Guix builds (on x86_64)

File	commit `2e9454a` (master)	commit `909d035` (master and this pull)
SHA256SUMS.part	`6f70ee2a3348199c...`	`3dc96312180e1760...`
*-aarch64-linux-gnu-debug.tar.gz	`171eccda523f4c39...`	`5f58b7a0abe4d126...`
*-aarch64-linux-gnu.tar.gz	`482eab25bba2cf21...`	`c7ff4bc137595fd8...`
*-arm-linux-gnueabihf-debug.tar.gz	`4645786ecdfdc1e6...`	`d71393c25d61aa70...`
*-arm-linux-gnueabihf.tar.gz	`63444c932c2ac892...`	`a74d2c7ac64505bb...`
*-arm64-apple-darwin-unsigned.tar.gz	`92fbf5298e848e32...`	`991fc338845ed2ee...`
*-arm64-apple-darwin-unsigned.zip	`da9b373e698cdf9c...`	`70d9a69deb1106ba...`
*-arm64-apple-darwin.tar.gz	`a667830ab1b89dfd...`	`6b9de461b138ed8f...`
*-powerpc64-linux-gnu-debug.tar.gz	`cb8999d51ecd2f8a...`	`0679c826ac6d2d01...`
*-powerpc64-linux-gnu.tar.gz	`c67a1ff303ac94e1...`	`b4939e62857e89af...`
*-powerpc64le-linux-gnu-debug.tar.gz	`7e1cb1f7d7a58cc8...`	`6eea2a5affc8809c...`
*-powerpc64le-linux-gnu.tar.gz	`7e619dd65842b3d5...`	`c92bbb561e484183...`
*-riscv64-linux-gnu-debug.tar.gz	`601e01ea41e4452d...`	`bab9ee1b3479e9fb...`
*-riscv64-linux-gnu.tar.gz	`c13a6d0c8bfd60b1...`	`accd8a56730d30a9...`
*-x86_64-apple-darwin-unsigned.tar.gz	`a0d269807c4854fd...`	`89dc2c2aafc88071...`
*-x86_64-apple-darwin-unsigned.zip	`72f5a017ab3543db...`	`a75d10fdd2579e4d...`
*-x86_64-apple-darwin.tar.gz	`dfa45a9d4b3201c8...`	`8074116efce3ec66...`
*-x86_64-linux-gnu-debug.tar.gz	`bb63037757faa1ac...`	`44b4116e472da440...`
*-x86_64-linux-gnu.tar.gz	`8021f0398f0756ee...`	`d8bed2411edb900e...`
*.tar.gz	`50d8bbd2ae74ca06...`	`c42ab57fa0a181a2...`
guix_build.log	`90a8c8f3508510cf...`	`08519d659303ab11...`
guix_build.log.diff		`fda2f4f334848d66...`

hebasto · 2023-11-03T08:13:02Z

In that case, maybe it would be easier to set something like -DOS_ACTIVITY_OBJECT_API=1 for the Qt build, and not have to patch any source?

I think it will hit

#error Please change your minimum OS requirements because OS_ACTIVITY_OBJECT_API is not available

Native builds (using the latest Xcode) are not broken in this way, and the minimum version handling shouldn't differ between the two?

I've verified the native build on macOS 14.1 without depends. Both macros are defined to 140000, which is the same as __MAC_14_0.

UPD. The Clang in depends does not define the __MAC_OS_X_VERSION_MIN_REQUIRED macro.

fanquake · 2023-11-09T10:18:11Z

Another thing to note, is that the code being patched here, has been deleted entirely upstream: qt/qtbase@329db8b.

macOS: Remove fallback defines for MAC_OS_X_VERSION_MIN_REQUIRED
Availability.h from the platform SDK should take care of this these days.

So this points to there still being an underlying issue, and workaround will "break" if/when we start using newer versions of Qt.

hebasto · 2023-11-10T17:16:39Z

Another thing to note, is that the code being patched here, has been deleted entirely upstream: qt/qtbase@329db8b.

macOS: Remove fallback defines for MAC_OS_X_VERSION_MIN_REQUIRED
Availability.h from the platform SDK should take care of this these days.

So this points to there still being an underlying issue...

The fact is that the __MAC_OS_X_VERSION_MIN_REQUIRED is not defined. At least, when using the current Clang version in depends.

... and workaround will "break" if/when we start using newer versions of Qt.

Agree. We can handle it in its time.

fanquake · 2023-11-10T17:31:06Z

At least, when using the current Clang version in depends.

Right, but why is it suddenly defined when using a newer compiler? Can you point to where in the LLVM/Clang source code this starts happening in versions 16+? It just irks me that updating to a new compiler "magically" fixes things, without explanation.

I think we will likely take this patch as a temporary workaround, just to unblock C++20, but we should probably add some more context to the patch itself. I also dislike having to add comments about "keeping things in sync", rather than enforcing what needs to be done via actual code.

hebasto · 2023-11-11T00:01:22Z

Closing in favour of #28851.

…version properly 05aca09 build: Patch Qt to handle minimum macOS version properly (Hennadii Stepanov) Pull request description: This PR is: - required to [switch](bitcoin/bitcoin#28622) to macOS 14 SDK (Xcode 15). - an alternative to bitcoin/bitcoin#28732 and bitcoin/bitcoin#28775. Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option. Xcode 12 SDK expects the OS-specific `__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__` macro: ```c++ #ifndef __MAC_OS_X_VERSION_MIN_REQUIRED #ifdef __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ /* compiler for Mac OS X sets __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ */ #define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ #endif #endif /* __MAC_OS_X_VERSION_MIN_REQUIRED*/ ``` In the other hand, Xcode 15 SDK expects a general `__ENVIRONMENT_OS_VERSION_MIN_REQUIRED__` macro: ```c++ #ifndef __MAC_OS_X_VERSION_MIN_REQUIRED #if defined(__has_builtin) && __has_builtin(__is_target_os) #if __is_target_os(macos) #define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_OS_VERSION_MIN_REQUIRED__ #define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0 #endif #elif __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ #define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ #define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0 #endif /* __has_builtin(__is_target_os) && __is_target_os(macos) */ #endif /* __MAC_OS_X_VERSION_MIN_REQUIRED */ ``` The latter macro is not provided by LLVM Clang until llvm/llvm-project@c8e2dd8, which is available in Clang 17. The suggested patch makes Qt "borrow" the `__MAC_OS_X_VERSION_MIN_REQUIRED` value from `MAC_OS_X_VERSION_MIN_REQUIRED`, which is set in the `AvailabilityMacros.h` SDK header. ACKs for top commit: maflcko: lgtm ACK 05aca09 Tree-SHA512: 8891aefde4b8a48885abf0648f4ec71a22f7fcfca1e17ebb8c70ce1ef44751ea5db6b8b652de6ee8a716ca5f96f720fef01600bc23986162d0146c946e2e8743

…properly 05aca09 build: Patch Qt to handle minimum macOS version properly (Hennadii Stepanov) Pull request description: This PR is: - required to [switch](bitcoin#28622) to macOS 14 SDK (Xcode 15). - an alternative to bitcoin#28732 and bitcoin#28775. Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option. Xcode 12 SDK expects the OS-specific `__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__` macro: ```c++ #ifndef __MAC_OS_X_VERSION_MIN_REQUIRED #ifdef __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ /* compiler for Mac OS X sets __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ */ #define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ #endif #endif /* __MAC_OS_X_VERSION_MIN_REQUIRED*/ ``` In the other hand, Xcode 15 SDK expects a general `__ENVIRONMENT_OS_VERSION_MIN_REQUIRED__` macro: ```c++ #ifndef __MAC_OS_X_VERSION_MIN_REQUIRED #if defined(__has_builtin) && __has_builtin(__is_target_os) #if __is_target_os(macos) #define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_OS_VERSION_MIN_REQUIRED__ #define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0 #endif #elif __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ #define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ #define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0 #endif /* __has_builtin(__is_target_os) && __is_target_os(macos) */ #endif /* __MAC_OS_X_VERSION_MIN_REQUIRED */ ``` The latter macro is not provided by LLVM Clang until llvm/llvm-project@c8e2dd8, which is available in Clang 17. The suggested patch makes Qt "borrow" the `__MAC_OS_X_VERSION_MIN_REQUIRED` value from `MAC_OS_X_VERSION_MIN_REQUIRED`, which is set in the `AvailabilityMacros.h` SDK header. ACKs for top commit: maflcko: lgtm ACK 05aca09 Tree-SHA512: 8891aefde4b8a48885abf0648f4ec71a22f7fcfca1e17ebb8c70ce1ef44751ea5db6b8b652de6ee8a716ca5f96f720fef01600bc23986162d0146c946e2e8743

build: Patch Qt to handle minimum macOS version properly

346cc12

This change is required to switch to macOS 14 SDK (Xcode 15).

DrahtBot added the Build system label Nov 2, 2023

hebasto mentioned this pull request Nov 2, 2023

build: Bump native_clang up to 17.0.2 #28732

Closed

maflcko added the DrahtBot Guix build requested label Nov 2, 2023

DrahtBot mentioned this pull request Nov 2, 2023

build: use macOS 14 SDK (Xcode 15.0) #28622

Merged

DrahtBot mentioned this pull request Nov 2, 2023

build: LLD based macOS toolchain #21778

Merged

DrahtBot removed the DrahtBot Guix build requested label Nov 2, 2023

hebasto marked this pull request as draft November 3, 2023 08:26

hebasto marked this pull request as ready for review November 3, 2023 10:14

hebasto mentioned this pull request Nov 11, 2023

build: Patch Qt to handle minimum macOS version properly #28851

Merged

hebasto closed this Nov 11, 2023

hebasto deleted the 231102-qt-macos branch December 4, 2023 11:29

bitcoin locked and limited conversation to collaborators Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: Patch Qt to handle minimum macOS version properly #28775

build: Patch Qt to handle minimum macOS version properly #28775

Uh oh!

hebasto commented Nov 2, 2023

Uh oh!

DrahtBot commented Nov 2, 2023 •

edited

Loading

Uh oh!

fanquake commented Nov 2, 2023 •

edited

Loading

Uh oh!

hebasto commented Nov 2, 2023

Uh oh!

fanquake commented Nov 2, 2023

Uh oh!

hebasto commented Nov 2, 2023

Uh oh!

fanquake commented Nov 2, 2023

Uh oh!

DrahtBot commented Nov 2, 2023

Uh oh!

hebasto commented Nov 3, 2023 •

edited

Loading

Uh oh!

fanquake commented Nov 9, 2023

Uh oh!

hebasto commented Nov 10, 2023

Uh oh!

fanquake commented Nov 10, 2023

Uh oh!

hebasto commented Nov 11, 2023

Uh oh!

Uh oh!

build: Patch Qt to handle minimum macOS version properly #28775

build: Patch Qt to handle minimum macOS version properly #28775

Uh oh!

Conversation

hebasto commented Nov 2, 2023

Uh oh!

DrahtBot commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

fanquake commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Nov 2, 2023

Uh oh!

fanquake commented Nov 2, 2023

Uh oh!

hebasto commented Nov 2, 2023

Uh oh!

fanquake commented Nov 2, 2023

Uh oh!

DrahtBot commented Nov 2, 2023

Guix builds (on x86_64)

Uh oh!

hebasto commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Nov 9, 2023

Uh oh!

hebasto commented Nov 10, 2023

Uh oh!

fanquake commented Nov 10, 2023

Uh oh!

hebasto commented Nov 11, 2023

Uh oh!

Uh oh!

DrahtBot commented Nov 2, 2023 •

edited

Loading

fanquake commented Nov 2, 2023 •

edited

Loading

hebasto commented Nov 3, 2023 •

edited

Loading