1564216 - Can't build with OSX 10.14 SDK with --enable-warnings-as-errors

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I can no longer build on OSX 10.13.6.

I get this error message (plus a number of similar ones):

0:21.03 /Users/amccreight/mc/widget/cocoa/nsCocoaUtils.mm:1174:19: error: 'AVAuthorizationStatusNotDetermined' is only available on macOS 10.14 or newer [-Werror,-Wunguarded-availability-new]
0:21.03 (int)AVAuthorizationStatusNotDetermined);
0:21.03 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I was able to build on this machine the last time I pulled, which was last week at some point. I'm not sure what has regressed. nsCocoaUtils.mm doesn't seem to have changed recently.

This particular chunk of code is guarded by
#if defined(MAC_OS_X_VERSION_10_14)
so I'm not sure why it is trying to compile it. I'm not sure where this is supposed to be defined.

Comment 1

[Bobby Holley (:bholley)]

You might try building with the 10.11 SDK: https://bugzilla.mozilla.org/show_bug.cgi?id=1522931#c27

Comment 2

[Stephen A Pohl [:spohl]]

This is bug 1494851.

Comment 3

[Andrew McCreight [:mccr8] (PTO until 12-2)]

If the #if defined(MAC_OS_X_VERSION_10_14) guards actually broke, there's likely going to be more than needs to be fixed than one API call. I guess we'll see.

Comment 4

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I'll start bisecting this. My good changeset is from July 3, and my bad changeset is from July 5, so hopefully it won't take long.

Comment 5

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I haven't gotten this quite bisected yet, but bug 1560044 is in the range and looks related. (Bug 1558924 is in the range and also mentions OSX, but I can't see how the patch would affect anything like this.)

Comment 6

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I'm going to undupe this. As far as I can tell, this is a new regression, where one of the symptoms of the regression is bug 1494851, so I'll mark it depending on that.

Anyways, my bisection points the blame to bug 1560044. My basic approach to bisection was to do mach build, let that run for awhile, then do mach build widget/

Glandium, can you take a look at this? Thanks.

Comment 7

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: log for mach build -v widget/cocoa

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: Unified_mm_widget_cocoa0.i from before the changeset landed

Comment 9

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: Unified_mm_widget_cocoa0.i with the changeset

Comment 10

[Mike Hommey [:glandium]]

So, the problem is with the code:

 #if defined(MAC_OS_X_VERSION_10_14)

MAC_OS_X_VERSION_10_x is not meant to be used this way. The SDK for version 10.x defines it, but the SDK can be used to target older versions, which is what happens when you use that version of the SDK on an older version of the OS.

Comment 11

[Mike Hommey [:glandium]]

At quick glance it looks like this is the only place misusing MAC_OS_X_VERSION_10_x

Comment 12

[Mike Hommey [:glandium]]

Attachment: Bug 1564216 - Fix macOS SDK conditions for 10.14.

Comment 13

[Mike Hommey [:glandium]]

Note there are other uses of MAC_OS_X_VERSION_MAX_ALLOWED that should probably be replaced with MAC_OS_X_VERSION_MIN_REQUIRED, but I'll leave that to Stephen or Haik to fix.

Comment 14

[Stephen A Pohl [:spohl]]

See code review feedback in phabricator.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Bug 1564216 - Fix warnings in nsCocoaUtils.mm with 10.14 SDK.

While there, transform the MOZ_ASSERTs into static_asserts, so that
discrepancies are caught at build time rather than runtime.

Note that I also tried to replace nsCocoaFeatures::OnMojaveOrLater with
@available, but that failed to link on older SDKs because of missing
___isOSVersionAtLeast symbol.

Comment 16

[Haik Aftandilian [:haik]]

Thanks for taking this on and improving the asserts. It looks like your patch addresses the problem on bug 1494851 and we can now close it as a dupe of this bug.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Bug 1564216 - Require clang 5 for OSX builds.

There is less incentive to keep things building with older versions of
clang for OSX builds, and we're going to require an objective-C feature
that was added in clang 5.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f82bce8aa27
Require clang 5 for OSX builds. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/991aad183871
Fix warnings in nsCocoaUtils.mm with 10.14 SDK. r=mstange

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:spohl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5f82bce8aa27
https://hg.mozilla.org/mozilla-central/rev/991aad183871

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is this something we wanted to backport to Beta and ESR68 to make life easier for developers?

Comment 22

[Mike Hommey [:glandium]]

People building with --enable-warnings-as-errors are asking for problems. Even more so when building esr68, which is likely to fail because of new warnings in newer versions of the rust compiler. So I'd say meh.

Comment 23

[:Gijs (out for now; he/him)]

(In reply to Mike Hommey [:glandium] from comment #22)

People building with --enable-warnings-as-errors are asking for problems. Even more so when building esr68, which is likely to fail because of new warnings in newer versions of the rust compiler. So I'd say meh.

I mean, building without it is also asking for problems because then your patches get backed out on infra because they cause warnings which are treated as errors there, soooo...

Comment 24

[Mike Hommey [:glandium]]

If you're really worried about warnings, you should at least do a try push with builds only, on all platforms. Because your local build with --enable-warnings-as-errors still has the possibility of making warnings on other platforms invisible to you.

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Mike Hommey [:glandium] from comment #24)

If you're really worried about warnings, you should at least do a try push with builds only, on all platforms. Because your local build with --enable-warnings-as-errors still has the possibility of making warnings on other platforms invisible to you.

Sure, but the possibility is much smaller, esp. now that we use clang everywhere, and because I effectively never write C++ code that (a) runs on multiple platforms and (b) has per-platform codepaths. So I run with --enable-warnings-as-errors locally in order to catch bustage before I push to try, as pushing to try takes much longer...

Comment 27

[Haik Aftandilian [:haik]]

(In reply to Mike Hommey [:glandium] from comment #22)

People building with --enable-warnings-as-errors are asking for problems. Even more so when building esr68, which is likely to fail because of new warnings in newer versions of the rust compiler. So I'd say meh.

FYI, when building release or beta, one should use rustup override to select the right rust version for the version of the tree being built.