Bug 209317 - Unable to build WebKit with iOS 13.4 SDK
Summary: Unable to build WebKit with iOS 13.4 SDK
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-19 15:38 PDT by Ryan Haddad
Modified: 2020-03-27 12:43 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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].