Bug 209317

Summary: Unable to build WebKit with iOS 13.4 SDK
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: 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: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ryan Haddad 2020-03-19 15:38:55 PDT 
Unable to build WebKit with iOS 13.4 SDK
Comment 1 Radar WebKit Bug Importer 2020-03-19 15:39:37 PDT 
<rdar://problem/60656534>
Comment 2 Tim Horton 2020-03-19 18:11:26 PDT 
Created attachment 394051 [details]
Patch
Comment 3 EWS 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].
Comment 4 Tim Horton 2020-03-25 15:35:30 PDT 
Reopening for one follow-up
Comment 5 Tim Horton 2020-03-25 15:38:30 PDT 
Created attachment 394552 [details]
Patch
Comment 6 Tim Horton 2020-03-25 15:38:51 PDT 
I disagree with all of the stylebot complaints in this case, so I'm gonna ignore it.
Comment 7 Alexey Proskuryakov 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?
Comment 8 Tim Horton 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.
Comment 9 Tim Horton 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"...
Comment 10 Alexey Proskuryakov 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...
Comment 11 EWS 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].
Comment 12 mitz 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.
Comment 13 mitz 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.
Comment 14 Tim Horton 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.
Comment 15 Tim Horton 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...
Comment 16 Tim Horton 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
Comment 17 Tim Horton 2020-03-27 11:43:01 PDT 
Reopening to attach new patch.
Comment 18 Tim Horton 2020-03-27 11:43:02 PDT 
Created attachment 394737 [details]
Patch
Comment 19 EWS 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].