Bug 160659 – Do not check if WebKitLibraries folder exists on EFL and GTK

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gyuyoung Kim]

As far as I know WebKitLibraries is only needed by mac and win ports though, EFL and GTK ports have checked whether it exists.

[Gyuyoung Kim]

Created attachment 285566 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 285566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285566&action=review

> Tools/Scripts/build-webkit:161
> -if (!isIOSWebKit() && !-d "WebKitLibraries") {
> +if ((!isIOSWebKit() && !isEfl() && !isGtk()) && !-d "WebKitLibraries") {
>      die "Error: No WebKitLibraries directory found. Please do a fresh checkout.\n";
>  }

AFAIK WebKitLibraries is used by Apple pots: Mac, iOS and Win too. There are *IOS* files in 
WebKitLibraries directory, so I don't understand this !isIOSWebKit() check. It was added 
by https://trac.webkit.org/changeset/161685 , we should as Daniel if we still need it for iOS.

I prefer whitelisting these platforms instead of blacklisting others. But we 
can be conservative until get answer from Apple, and leave iOS unchanged.

- if (!isIOSWebKit() && !-d "WebKitLibraries")
+ if (!isIOSWebKit() && isAppleWebKit() && !-d "WebKitLibraries")

[Daniel Bates]

Comment on attachment 285566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285566&action=review

> Tools/Scripts/build-webkit:159
> +if ((!isIOSWebKit() && !isEfl() && !isGtk()) && !-d "WebKitLibraries") {

We should remove the !isIOSWebKit() from this line. As Csaba pointed out in comment #2, we expect the directory WebKitLibraries to exist when building the Apple WebKit ports.

For completeness, originally the iOS port did not make use of files in directory WebKitLibraries. So, we skipped the directory existence check for this directory when building for iOS.

>> Tools/Scripts/build-webkit:161
>>  }
> 
> AFAIK WebKitLibraries is used by Apple pots: Mac, iOS and Win too. There are *IOS* files in 
> WebKitLibraries directory, so I don't understand this !isIOSWebKit() check. It was added 
> by https://trac.webkit.org/changeset/161685 , we should as Daniel if we still need it for iOS.
> 
> I prefer whitelisting these platforms instead of blacklisting others. But we 
> can be conservative until get answer from Apple, and leave iOS unchanged.
> 
> - if (!isIOSWebKit() && !-d "WebKitLibraries")
> + if (!isIOSWebKit() && isAppleWebKit() && !-d "WebKitLibraries")

From my understanding the WinCairo port also makes use of WebKitLibraries.

[Gyuyoung Kim]

Created attachment 285621 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #3)
> Comment on attachment 285566 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285566&action=review
> 
> > Tools/Scripts/build-webkit:159
> > +if ((!isIOSWebKit() && !isEfl() && !isGtk()) && !-d "WebKitLibraries") {
> 
> We should remove the !isIOSWebKit() from this line. As Csaba pointed out in
> comment #2, we expect the directory WebKitLibraries to exist when building
> the Apple WebKit ports.

Daniel, I remove the !isIOSWebKit() in the line.

> For completeness, originally the iOS port did not make use of files in
> directory WebKitLibraries. So, we skipped the directory existence check for
> this directory when building for iOS.
> 
> >> Tools/Scripts/build-webkit:161
> >>  }
> > 
> > AFAIK WebKitLibraries is used by Apple pots: Mac, iOS and Win too. There are *IOS* files in 
> > WebKitLibraries directory, so I don't understand this !isIOSWebKit() check. It was added 
> > by https://trac.webkit.org/changeset/161685 , we should as Daniel if we still need it for iOS.
> > 
> > I prefer whitelisting these platforms instead of blacklisting others. But we 
> > can be conservative until get answer from Apple, and leave iOS unchanged.
> > 
> > - if (!isIOSWebKit() && !-d "WebKitLibraries")
> > + if (!isIOSWebKit() && isAppleWebKit() && !-d "WebKitLibraries")
> 
> From my understanding the WinCairo port also makes use of WebKitLibraries.

ok, I modify the condition which checks if port is isAppleWebKit() or isWinCairo(). Is it fine ?

if ((isAppleWebKit() || isWinCairo()) && !-d "WebKitLibraries")

[Csaba Osztrogonác]

Comment on attachment 285621 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 285621 [details]
Patch

Clearing flags on attachment: 285621

Committed r204277: <http://trac.webkit.org/changeset/204277>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.