Bug 233906 – Deployment target for macOS 11+ does not follow minor version bumps

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Williams]

Deployment target for macOS 11+ does not follow minor version bumps

[Elliott Williams]

Created attachment 446108 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 446108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446108&action=review

> Source/JavaScriptCore/ChangeLog:8
> +				Change the deployment target for macOS 11 and above to use the default

Please use spaces in ChangeLog too (style checker said that, but it's not necessarily super visible).


The ChangeLog would benefit from mentioning that <rdar://problem/70185899> is now fixed in all versions of Xcode that we use, so you are removing the workaround while at it.

> Source/WTF/ChangeLog:8
> +				Change the deployment target for macOS 11 and above to use the default

Not entirely sure if committing will merge the explanations; could make for a terrible commit message if not. I usually only put a long explanation in WebCore ChangeLog, but not sure if that's right or necessary.

Just saying it out loud, you don't have to investigate or change this.

> Source/JavaScriptCore/Configurations/DebugRelease.xcconfig:34
>  MACOSX_DEPLOYMENT_TARGET_101400 = 10.14;

10.14 can/should be removed at this point.

> Source/JavaScriptCore/Configurations/DebugRelease.xcconfig:37
> +MACOSX_DEPLOYMENT_TARGET_120000 = $(inherited);

How does this work, what is this inherited from? It seems like it ends up being empty, and can be removed. Also unclear to me why 11.0 and 12.0 are handled differently.

[Alexey Proskuryakov]

Comment on attachment 446108 [details]
Patch

Marking r-, as the patch needs to be updated at least to remove tabs, possibly more.

[Elliott Williams]

rdar://86041002

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #2)

> > Source/JavaScriptCore/Configurations/DebugRelease.xcconfig:37
> > +MACOSX_DEPLOYMENT_TARGET_120000 = $(inherited);
> 
> How does this work, what is this inherited from? It seems like it ends up
> being empty, and can be removed. Also unclear to me why 11.0 and 12.0 are
> handled differently.

They inherit from the SDK default, but you're right that they can be removed -- this setting isn't overriding anything in the pbxprojs.

My hesitation with changing 11.0 is that you can currently force a macOS 11.0 deployment target by setting

    TARGET_MAC_OS_X_VERSION_MAJOR = 110000

which is long-standing functionality from https://commits.webkit.org/47162@main. AFAICT, we can't continue to support TARGET_MAC_OS_X_VERSION_MAJOR with minor-versioned deployment targets without hardcoding every known deployment target we support. But I left it in place, in case anyone depends on it for 11.0 and below. What do you think about that tradeoff? Could we stop letting TARGET_MAC_OS_X_VERSION_MAJOR influence the deployment target altogether?

[Alexey Proskuryakov]

I don't think that we have any tools that set TARGET_MAC_OS_X_VERSION_MAJOR (I checked open source and internal scripts), and engineers wouldn't set it manually. So if there is any code that supports setting it externally, that seems like dead code to remove. Looks like this functionality was only needed for WebKit nightlies.

Production builds can of course be built on a mismatching host OS, but that doesn't work via changing TARGET_MAC_OS_X_VERSION_MAJOR.

I'll admit that I'm always confused by relationships between TARGET_MAC_OS_X_VERSION_MAJOR, MAC_OS_X_VERSION_MAJOR, and MACOSX_DEPLOYMENT_TARGET.

[Elliott Williams]

Created attachment 446286 [details]
Patch

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #6)
> I don't think that we have any tools that set TARGET_MAC_OS_X_VERSION_MAJOR
> (I checked open source and internal scripts), and engineers wouldn't set it
> manually. So if there is any code that supports setting it externally, that
> seems like dead code to remove.

The latest patch does that, which makes this change nice and all-red. I'd like to test on a supported versions of 10.x and 11.x, but assuming its Xcode behaves the same way there and targets the OS version corresponding to the SDK, we shouldn't need any of this.

[Alexey Proskuryakov]

mac-AS-debug-wk2 EWS is unhappy. We do have a mismatch between host and SDK version on this bot, but I'm not sure how it explains the difference. I haven't looked deeply though.

[Elliott Williams]

There are some additional places we need to unset MACOSX_DEPLOYMENT_TARGET. I think this is causing problems because the core targets are targeting 11.3 (the SDK version on those builders) and the test targets are still targeting 11.0.

[Elliott Williams]

Created attachment 446407 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Alexey Proskuryakov]

Comment on attachment 446407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446407&action=review

> Source/JavaScriptCore/Configurations/DebugRelease.xcconfig:-32
> -// FIXME: Once <rdar://problem/70185899> is fixed, replace the following with
> -// TARGET_MAC_OS_X_VERSION_MAJOR = $(MAC_OS_X_VERSION_MAJOR)

This removes TARGET_MAC_OS_X_VERSION_MAJOR completely instead, and I don't see how this corresponds to ChangeLog saying that it should be considered read-only. Is it also set elsewhere?

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #13)
> This removes TARGET_MAC_OS_X_VERSION_MAJOR completely instead, and I don't
> see how this corresponds to ChangeLog saying that it should be considered
> read-only. Is it also set elsewhere?

It's also set in each module's `Base.xcconfig`. It's defined a few different ways in different places, but the main modules all use:

   TARGET_MAC_OS_X_VERSION_MAJOR = $(TARGET_MAC_OS_X_VERSION_MAJOR_$(TARGET_MACOS_LEGACY_VERSION_IDENTIFIER))

I confirmed that for projects like WebCore, TARGET_MAC_OS_X_VERSION_MAJOR is still correctly computed as 120000 for 12.x, 101500 for 10.15, etc.

[Elliott Williams]

Spent some time down the rabbit hole, and I think I understand why the `ApplePayButton.html` layout test is failing:

1. The test calls `ApplePaySession.supportsVersion()` to determine whether it can check new -apple-pay-button-type values added in ApplePaySession v10.

2. This is implemented in WebCore::PaymentAPIVersion::current(), which uses platform macros to determine what version number to return: https://github.com/WebKit/WebKit/blob/413e73561f8da3a1c9cfafe74ea1810f37e5786f/Source/WebCore/Modules/applepay/cocoa/PaymentAPIVersionCocoa.mm#L35

3. The macro for v11 (`ENABLE(APPLE_PAY_SESSION_V11)`) is defined in PlatformEnableCocoa.h as

    (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 110300)

which is now true for this builder, because its deployment target is changed to 11.3 :-)

4. So the ApplePaySession API reports that it supports v11, but it doesn't actually support the button types added in v10. The button type implementation is controlled by HAVE_PASSKIT_NEW_BUTTON_TYPES. AFAICT, HAVE_PASSKIT_NEW_BUTTON_TYPES is not part of any production SDK.

Confirmed via disassembly that PaymentAPIVersion::current() now returns 11, and that on trunk builds it returns one of the versions below 10.

What should we do about this? Perhaps the best thing to do would be to make the v11 macro depend on the same things the v10 macro does, but I don't know if that would have downstream consequences.

[Elliott Williams]

Created attachment 446584 [details]
Patch

[Elliott Williams]

Created attachment 446620 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 446620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446620&action=review

Looks right to me, r=ews.

> Source/WTF/wtf/PlatformHave.h:636
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 110000) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)

The iOS version check is not needed, we don't support anything older than iOS 15. So just (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 110000) || PLATFORM(IOS).

It is a little unclear to me if this was added in 11.0 or 11.3, but in practice, there is no difference, building or running on 11.0 isn't supported either.

[Alexey Proskuryakov]

Seeing the build failures, pretty sure the new types need to be added to PassKitSPI.h.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #19)
> Seeing the build failures, pretty sure the new types need to be added to
> PassKitSPI.h.

PKPaymentButtonType is public API starting in 11.0: https://developer.apple.com/documentation/passkit/pkpaymentbuttontype

Would it be better to change our PKPaymentButtonType redeclaring to only compile below 11.0, so that we aren't overshadowing public API?

[Elliott Williams]

Created attachment 446832 [details]
Patch

[Alexey Proskuryakov]

> Would it be better to change our PKPaymentButtonType redeclaring to only compile below 11.0, so that we aren't overshadowing public API?

Nice catch! Not quite sure about how the latest patch implements this neat idea - I would expect that we need to add an include for open source builds. Typedefs don't shadow; we must not be including the API header now.

Would also point out that checking iOS version is still not needed:

> #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 110000) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
> #define HAVE_PASSKIT_NEW_BUTTON_TYPES 1
> #endif

[Elliott Williams]

Created attachment 446855 [details]
Patch

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #22)
> > Would it be better to change our PKPaymentButtonType redeclaring to only compile below 11.0, so that we aren't overshadowing public API?
> 
> Nice catch! Not quite sure about how the latest patch implements this neat
> idea - I would expect that we need to add an include for open source builds.
> Typedefs don't shadow; we must not be including the API header now.

You're right, we aren't including it on Mac. I can play with this some more next week, but I think it's a bit out of scope for this change. My attempt to clean this up didn't help much, since we still need to include the SPI definitions for 10.15.

> Would also point out that checking iOS version is still not needed:

Thank you for the reminder, the latest patch has this fixed :)

[EWS]

Committed r286958 (245182@main): <https://commits.webkit.org/245182@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446855 [details].