Bug 209317

Summary:

Unable to build WebKit with iOS 13.4 SDK

Product:

WebKit

Reporter:

Ryan Haddad <ryanhaddad>

Component:

New Bugs

Assignee:

Tim Horton <thorton>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aestes, ap, dino, jbedard, megan_gardner, mitz, simon.fraser, webkit-bot-watchers-bugzilla, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Ryan Haddad

Reported 2020-03-19 15:38:55 PDT

Unable to build WebKit with iOS 13.4 SDK

Attachments
Patch (4.45 KB, patch) 2020-03-19 18:11 PDT, Tim Horton	no flags	Details Formatted Diff Diff
Patch (2.14 KB, patch) 2020-03-25 15:38 PDT, Tim Horton	no flags	Details Formatted Diff Diff
Patch (1.94 KB, patch) 2020-03-27 11:43 PDT, Tim Horton	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-03-19 15:39:37 PDT

<rdar://problem/60656534>

Tim Horton

Comment 2 2020-03-19 18:11:26 PDT

Created attachment 394051 [details] Patch

EWS

Comment 3 2020-03-19 21:41:30 PDT

Committed r258751: <https://trac.webkit.org/changeset/258751> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394051 [details].

Tim Horton

Comment 4 2020-03-25 15:35:30 PDT

Reopening for one follow-up

Tim Horton

Comment 5 2020-03-25 15:38:30 PDT

Created attachment 394552 [details] Patch

Tim Horton

Comment 6 2020-03-25 15:38:51 PDT

I disagree with all of the stylebot complaints in this case, so I'm gonna ignore it.

Alexey Proskuryakov

Comment 7 2020-03-25 15:59:07 PDT

Hmm, how is a bunch of numeric version checks throughout the code better than a HAVE macro with a descriptive name?

Tim Horton

Comment 8 2020-03-25 16:28:24 PDT

(In reply to Alexey Proskuryakov from comment #7) > Hmm, how is a bunch of numeric version checks throughout the code better > than a HAVE macro with a descriptive name? Because as we currently use it, the HAVE() macro would also be true before? We had it! It just was SPI! Now it's not! Maybe we'd want a bunch of them with names like HAVE(WHATEVER_AS_API)? I think that this is unprecedented, but could go either way.

Tim Horton

Comment 9 2020-03-25 16:29:02 PDT

(In reply to Tim Horton from comment #8) > (In reply to Alexey Proskuryakov from comment #7) > > Hmm, how is a bunch of numeric version checks throughout the code better > > than a HAVE macro with a descriptive name? > > Because as we currently use it, the HAVE() macro would also be true before? > We had it! It just was SPI! Now it's not! Maybe we'd want a bunch of them > with names like HAVE(WHATEVER_AS_API)? I think that this is unprecedented, > but could go either way. Also, I feel like SPI headers are a very special case, not "throughout the code"...

Alexey Proskuryakov

Comment 10 2020-03-25 16:49:51 PDT

> Also, I feel like SPI headers are a very special case, not "throughout the code"... There is some truth to that...

EWS

Comment 11 2020-03-25 17:32:06 PDT

Committed r259025: <https://trac.webkit.org/changeset/259025> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394552 [details].

mitz

Comment 12 2020-03-25 19:32:38 PDT

Comment on attachment 394552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394552&action=review > Source/WebKit/Platform/spi/ios/UIKitSPI.h:233 > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130400 This should me MAX_ALLOWED, since the presence of the definitions in the SDK depends on the SDK version used at build time, not the deployment target.

mitz

Comment 13 2020-03-25 19:34:53 PDT

(In reply to Tim Horton from comment #9) > Also, I feel like SPI headers are a very special case, not "throughout the > code"... Yes, since those headers mimic SDK content, as a rule they should be using SDK version conditionals.

Tim Horton

Comment 14 2020-03-25 20:18:38 PDT

(In reply to mitz from comment #13) > (In reply to Tim Horton from comment #9) > > > Also, I feel like SPI headers are a very special case, not "throughout the > > code"... > > Yes, since those headers mimic SDK content, as a rule they should be using > SDK version conditionals. Haha. (In reply to mitz from comment #12) > Comment on attachment 394552 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394552&action=review > > > Source/WebKit/Platform/spi/ios/UIKitSPI.h:233 > > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130400 > > This should me MAX_ALLOWED, since the presence of the definitions in the SDK > depends on the SDK version used at build time, not the deployment target. Yes, good point! I will fix.

Tim Horton

Comment 15 2020-03-27 11:12:27 PDT

(In reply to Tim Horton from comment #14) > (In reply to mitz from comment #13) > > > Source/WebKit/Platform/spi/ios/UIKitSPI.h:233 > > > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130400 > > > > This should me MAX_ALLOWED, since the presence of the definitions in the SDK > > depends on the SDK version used at build time, not the deployment target. > > Yes, good point! I will fix. Hmm, this is actually not working...

Tim Horton

Comment 16 2020-03-27 11:20:22 PDT

(In reply to Tim Horton from comment #15) > (In reply to Tim Horton from comment #14) > > (In reply to mitz from comment #13) > > > > Source/WebKit/Platform/spi/ios/UIKitSPI.h:233 > > > > +#if PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130400 > > > > > > This should me MAX_ALLOWED, since the presence of the definitions in the SDK > > > depends on the SDK version used at build time, not the deployment target. > > > > Yes, good point! I will fix. > > Hmm, this is actually not working... Looks like MAX_ALLOWED remains at 130000

Tim Horton

Comment 17 2020-03-27 11:43:01 PDT

Reopening to attach new patch.

Tim Horton

Comment 18 2020-03-27 11:43:02 PDT

Created attachment 394737 [details] Patch

EWS

Comment 19 2020-03-27 12:43:42 PDT

Committed r259133: <https://trac.webkit.org/changeset/259133> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394737 [details].

Note You need to log in before you can comment on or make changes to this bug.