Closed Bug 1283036 Opened 5 years ago Closed 5 years ago
Build failure on mac
OS 10 .12 Sierra
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.
Comment on attachment 8766216 [details] [diff] [review] Fix build on 10.12 ># HG changeset patch ># User ojab <email@example.com> ># 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?
yeah, looks like MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_12 is also needed.
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?
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 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+
…and looks like somehow I've messed up with mercurial. Cleaned up patch w/ updated commit message.
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+
Hope that this is the last.
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.
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 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.
[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.