Closed Bug 1283036 Opened 5 years ago Closed 5 years ago

Build failure on macOS 10.12 Sierra

Categories

(Core :: Widget: Cocoa, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox-esr45 - fixed
firefox50 --- fixed

People

(Reporter: ojab, Assigned: ojab)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Fix build on 10.12 (obsolete) — Splinter Review
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
Component: Untriaged → Widget: Cocoa
OS: Unspecified → Mac OS X
Product: Firefox → Core
Hardware: Unspecified → x86_64
Looks like that kVK_RightCommand is defined by SDK newly. I think that it should be gone if the SDK version is enough high.
Attachment #8766216 - Flags: review?(masayuki)
Attachment #8766216 - Attachment is patch: true
Attachment #8766216 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 8766216 [details] [diff] [review]
Fix build on 10.12

># HG changeset patch
># User ojab <ojab@ojab.ru>
># 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?
Attached patch sierra_v2.patch (obsolete) — Splinter Review
yeah, looks like MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12 is also needed.
Attachment #8766216 - Attachment is obsolete: true
Attachment #8766216 - Flags: review?(masayuki)
Attachment #8766381 - Flags: review?(masayuki)
Attachment #8766381 - Flags: review?(masayuki) → review+
Assignee: nobody → ojab
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Shouldn't you rather #ifdef out the redefinition in TextInputHandler.h, instead of the ambiguous use in TextInputHandler.mm?
(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.
I agree, but I don't think the patch is actually doing that.
(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.
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 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!
Attachment #8766381 - Flags: review+ → review-
Could you rewrite the patch as dropping the definition of kVK_RightCommand in the header file?
Flags: needinfo?(ojab)
Attached patch sierra_v3.patch (obsolete) — Splinter Review
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.
Attachment #8766381 - Attachment is obsolete: true
Flags: needinfo?(ojab)
Attachment #8767567 - Flags: review?(masayuki)
Comment on attachment 8767567 [details] [diff] [review]
sierra_v3.patch

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

Thanks!
Attachment #8767567 - Flags: review?(masayuki) → review+
Attached patch sierra_v3.5.patch (obsolete) — Splinter Review
…and looks like somehow I've messed up with mercurial.
Cleaned up patch w/ updated commit message.
Attachment #8767567 - Attachment is obsolete: true
Attachment #8767570 - Flags: review?(masayuki)
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.
Attachment #8767570 - Flags: review?(masayuki) → review+
Attached patch sierra_v4.patchSplinter Review
Hope that this is the last.
Attachment #8767570 - Attachment is obsolete: true
Attachment #8767580 - Flags: review?(masayuki)
Comment on attachment 8767580 [details] [diff] [review]
sierra_v4.patch

You can carry over the previous r+ ;-)
Attachment #8767580 - Flags: review?(masayuki) → review+
The patch has wrong bug number and landed in bug 1266369 comment 24.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Attachment #8767580 - Flags: approval-mozilla-beta?
Attachment #8767580 - Flags: approval-mozilla-aurora?
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.
Attachment #8767580 - Flags: approval-mozilla-beta?
Attachment #8767580 - Flags: approval-mozilla-beta-
Attachment #8767580 - Flags: approval-mozilla-aurora?
Attachment #8767580 - Flags: approval-mozilla-aurora+
[Tracking Requested - why for this release]: This build bustage impacts Firefox 45 ESR.  I'd appreciate a merge into that stable release branch, please.
(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 on attachment 8767580 [details] [diff] [review]
sierra_v4.patch

Fix for Mac 10.12 build for ESR.
Attachment #8767580 - Flags: approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.