1283036 - Build failure on macOS 10.12 Sierra

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[ojab]

Attachment: Fix build on 10.12

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160628004053

Steps to reproduce:

./mach build


Actual results:

build failed
 4:44.08 In file included from /Users/ojab/Documents/repos/mozilla-central/obj-x86_64-apple-darwin16.0.0/widget/cocoa/Unified_mm_widget_cocoa0.mm:47:
 4:44.08 /Users/ojab/Documents/repos/mozilla-central/widget/cocoa/TextInputHandler.mm:50:10: error: reference to 'kVK_RightCommand' is ambiguous
 4:44.08     case kVK_RightCommand:        return "Right-Command";
 4:44.08          ^
 4:44.08 /System/Library/Frameworks/Carbon.framework/Frameworks/HIToolbox.framework/Headers/Events.h:276:3: note: candidate found by name lookup is 'kVK_RightCommand'
 4:44.08   kVK_RightCommand              = 0x36,
 4:44.08   ^
 4:44.08 /Users/ojab/Documents/repos/mozilla-central/widget/cocoa/TextInputHandler.h:30:3: note: candidate found by name lookup is 'mozilla::widget::kVK_RightCommand'
 4:44.08   kVK_RightCommand    = 0x36, // right command key
 4:44.09   ^
 4:44.11 Layers.o
 4:44.21    clang++	 ...  /Users/ojab/Documents/repos/mozilla-central/intl/icu/source/i18n/quantityformatter.cpp
 4:44.22 In file included from /Users/ojab/Documents/repos/mozilla-central/obj-x86_64-apple-darwin16.0.0/widget/cocoa/Unified_mm_widget_cocoa0.mm:74:
 4:44.23 Warning: -Wmismatched-parameter-types in /Users/ojab/Documents/repos/mozilla-central/widget/cocoa/nsAppShell.mm: conflicting parameter types in implementation of 'nextEventMatchingMask:untilDate:inMode:dequeue:': 'NSEventMask' (aka 'unsigned long long') vs 'NSUInteger' (aka 'unsigned long')
 4:44.23 /Users/ojab/Documents/repos/mozilla-central/widget/cocoa/nsAppShell.mm:113:47: warning: conflicting parameter types in implementation of 'nextEventMatchingMask:untilDate:inMode:dequeue:': 'NSEventMask' (aka 'unsigned long long') vs 'NSUInteger' (aka 'unsigned long') [-Wmismatched-parameter-types]
 4:44.23 - (NSEvent*)nextEventMatchingMask:(NSUInteger)mask
 4:44.23                                    ~~~~~~~~~~ ^
 4:44.23 /System/Library/Frameworks/AppKit.framework/Headers/NSApplication.h:280:58: note: previous definition is here
 4:44.23 - (nullable NSEvent *)nextEventMatchingMask:(NSEventMask)mask untilDate:(nullable NSDate *)expiration inMode:(NSString *)mode dequeue:(BOOL)deqFlag;
 4:44.23                                              ~~~~~~~~~~~ ^
 4:44.62    clang++	 ...  /Users/ojab/Documents/repos/mozilla-central/intl/icu/source/i18n/measunit.cpp
 4:44.95 1 warning and 1 error generated.


Expected results:

build succeed

kVK_RightCommand define was added to the Carbon.framework in 10.12: https://developer.apple.com/library/prerelease/content/releasenotes/Miscellaneous/APIDiffsMacOS10_12/Objective-C/Carbon.html

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that kVK_RightCommand is defined by SDK newly. I think that it should be gone if the SDK version is enough high.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8766216 [details] [diff] [review]
Fix build on 10.12

># HG changeset patch
># User ojab <[e-mail]>
># Parent  e6d0c56847ced14f0b7500b12a852defd83d5a7a
>Don't define kVK_RightCommand when building against the macOS 10.12 SDK.

You need to add bug number at the start of this line. See other commits for understanding the commit message rules.

>+#if !defined(MAC_OS_X_VERSION_10_12)


Shouldn't this be:

#if !defined(MAC_OS_X_VERSION_10_12) || \
    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12

no?

Comment 3

[ojab]

Attachment: sierra_v2.patch

yeah, looks like MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12 is also needed.

Comment 4

[Markus Stange [:mstange]]

Shouldn't you rather #ifdef out the redefinition in TextInputHandler.h, instead of the ambiguous use in TextInputHandler.mm?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Markus Stange [:mstange] from comment #4)
> Shouldn't you rather #ifdef out the redefinition in TextInputHandler.h,
> instead of the ambiguous use in TextInputHandler.mm?

I don't think so because I defined kVK_RightCommand which was defined for exactly same purpose of the new SDK. So, if it's defined by SDK, we should use it.

Comment 6

[Markus Stange [:mstange]]

I agree, but I don't think the patch is actually doing that.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Markus Stange [:mstange] from comment #6)
> I agree, but I don't think the patch is actually doing that.

Why? The patch drops the our definition of kVK_RightCommand if the SDK is for 10.12 or later. So, with such SDK, it's defined only by SDK, otherwise, defined only by ourselves.

Comment 8

[Markus Stange [:mstange]]

Am I looking at the right patch? The patch that I see does not drop our definition of kVK_RightCommand. It only drops the switch case for kVK_RightCommand in the GetKeyNameForNativeKeyCode function. Our definition of kVK_RightCommand is in an enum in TextInputHandler.h, and the patch does not touch that.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8766381 [details] [diff] [review]
sierra_v2.patch

Oh, you're right. I misunderstood the patch. I believed that changes the definition of kVK_RightCommand. So, this patch does really wrong thing!

Thank you for your check!

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Could you rewrite the patch as dropping the definition of kVK_RightCommand in the header file?

Comment 11

[ojab]

Attachment: sierra_v3.patch

Whooops, I've actually intended to change header definition, dunno how I've messed it.
Please note that there is another build issue (presumably due to bug956899): 10.12 has no pthread_condattr_setclock like android, so I've added check for __APPLE__ there.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8767567 [details] [diff] [review]
sierra_v3.patch

Please remove gfx/layers/composite/TextureHost.cpp part before landing.

Thanks!

Comment 13

[ojab]

Attachment: sierra_v3.5.patch

…and looks like somehow I've messed up with mercurial.
Cleaned up patch w/ updated commit message.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8767570 [details] [diff] [review]
sierra_v3.5.patch

Please remove js/src/threading/posix/ConditionVariable.cpp part from this patch. I cannot review this and it's not good to include non-related fixes into a patch.

If you'll land only TextInputHandler.h part, r=masayuki.

Comment 15

[ojab]

Attachment: sierra_v4.patch

Hope that this is the last.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8767580 [details] [diff] [review]
sierra_v4.patch

You can carry over the previous r+ ;-)

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

The patch has wrong bug number and landed in bug 1266369 comment 24.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8767580 [details] [diff] [review]
sierra_v4.patch

Approval Request Comment
[Feature/regressing bug #]: macOS 10.12 Sierra
[User impact if declined]: Developers who work on 10.12 will not be able to compile Firefox, for example when they want to test their fixes for uplift on release branches
[Describe test coverage new/current, TreeHerder]: none, we don't have automatic builders on 10.12 yet
[Risks and why]: extremely low, the patch just removes a duplicate definition of a symbol. (If the patch were wrong, we wouldn't be able to build.)
[String/UUID change made/needed]: none

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8767580 [details] [diff] [review]
sierra_v4.patch

Review of attachment 8767580 [details] [diff] [review]:
-----------------------------------------------------------------

It's too late for 48 beta. Let's let it ride the train on 49 and not fix in 48 to mitigate the risks on the release.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2fc57c35067e

Comment 21

[Alex Vincent [:WeirdAl]]

[Tracking Requested - why for this release]: This build bustage impacts Firefox 45 ESR.  I'd appreciate a merge into that stable release branch, please.

Comment 22

[Alex Vincent [:WeirdAl]]

(In reply to ojab from comment #15)
> Created attachment 8767580 [details] [diff] [review]
> sierra_v4.patch
> 
> Hope that this is the last.

This patch applies cleanly to ESR 45 code.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8767580 [details] [diff] [review]
sierra_v4.patch

Fix for Mac 10.12 build for ESR.

Comment 24

[Carsten Book [:Tomcat]]

  https://hg.mozilla.org/releases/mozilla-esr45/rev/e406b951f4a62b6890ab5479873e7fc7222f2642