build: Patch Qt to handle minimum macOS version properly

hebasto commented at 1:45 pm on November 2, 2023: member

This PR is:

required to switch to macOS 14 SDK (Xcode 15).
an alternative to #28732.

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

346cc125d0

DrahtBot commented at 1:45 pm on November 2, 2023: 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. 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.

DrahtBot added the label Build system on Nov 2, 2023

fanquake commented at 1:49 pm on November 2, 2023: member

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 commented at 1:59 pm on November 2, 2023: member

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:

0#if OS_LOG_TARGET_HAS_10_12_FEATURES
1#ifndef OS_ACTIVITY_OBJECT_API
2#define OS_ACTIVITY_OBJECT_API 1
3#endif // OS_ACTIVITY_OBJECT_API
4#else
5#if OS_ACTIVITY_OBJECT_API
6#error Please change your minimum OS requirements because OS_ACTIVITY_OBJECT_API is not available
7#endif // OS_ACTIVITY_OBJECT_API
8#define OS_ACTIVITY_OBJECT_API 0
9#endif // OS_LOG_TARGET_HAS_10_12_FEATURES

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

0#if __OS_LOG_OS_FEATURES_AT_LEAST(10_12, 10_0, 10_0, 3_0)
1#define OS_LOG_TARGET_HAS_10_12_FEATURES 1
2#else
3#define OS_LOG_TARGET_HAS_10_12_FEATURES 0
4#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.

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

fanquake commented at 2:13 pm on November 2, 2023: member

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 commented at 2:27 pm on November 2, 2023: member

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:

 0    It is also possible to use the *_VERSION_MIN_REQUIRED in source code to make one
 1    source base that can be compiled to target a range of OS versions.  It is best
 2    to not use the _MAC_* and __IPHONE_* macros for comparisons, but rather their values.
 3    That is because you might get compiled on an old OS that does not define a later
 4    OS version macro, and in the C preprocessor undefined values evaluate to zero
 5    in expresssions, which could cause the #if expression to evaluate in an unexpected
 6    way.
 7    
 8        #ifdef __MAC_OS_X_VERSION_MIN_REQUIRED
 9            // code only compiled when targeting Mac OS X and not iPhone
10            // note use of 1050 instead of __MAC_10_5
11            #if __MAC_OS_X_VERSION_MIN_REQUIRED < 1050
12                // code in here might run on pre-Leopard OS
13            #else
14                // code here can assume Leopard or later
15            #endif
16        #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 commented at 3:22 pm on November 2, 2023: member

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 commented at 9:26 pm on November 2, 2023: contributor

Guix builds (on x86_64)

File	commit 2e9454a633462604cfafdf685cc9b3ad7d0e56ef(master)	commit 909d035abb2a142b737c45343ff9f7a250479a65(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...`

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

hebasto commented at 8:13 am on November 3, 2023: member

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

0#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.

hebasto marked this as a draft on Nov 3, 2023

hebasto marked this as ready for review on Nov 3, 2023

fanquake commented at 10:18 am on November 9, 2023: member

Another thing to note, is that the code being patched here, has been deleted entirely upstream: https://github.com/qt/qtbase/commit/329db8b64f17c8ef013c586cea1f1c5b49c4a4b7.

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 commented at 5:16 pm on November 10, 2023: member

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 commented at 5:31 pm on November 10, 2023: member

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 commented at 0:01 am on November 11, 2023: member

Closing in favour of #28851.

hebasto closed this on Nov 11, 2023

fanquake referenced this in commit 160d23677a on Dec 1, 2023

hebasto deleted the branch on Dec 4, 2023

PastaPastaPasta referenced this in commit 51a8c92abc on Oct 24, 2024

PastaPastaPasta referenced this in commit fb33ef6771 on Oct 24, 2024

PastaPastaPasta referenced this in commit a77593e790 on Oct 24, 2024

PastaPastaPasta referenced this in commit 733a036219 on Oct 24, 2024

PastaPastaPasta referenced this in commit f670118cce on Oct 24, 2024

bitcoin locked this on Dec 3, 2024

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

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)