Bug 160659 - Do not check if WebKitLibraries folder exists on EFL and GTK
Summary: Do not check if WebKitLibraries folder exists on EFL and GTK
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-08 08:19 PDT by Gyuyoung Kim
Modified: 2016-08-09 00:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.21 KB, patch)
2016-08-08 08:21 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (1.21 KB, patch)
2016-08-08 17:49 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2016-08-08 08:19:33 PDT
As far as I know WebKitLibraries is only needed by mac and win ports though, EFL and GTK ports have checked whether it exists.
Comment 1 Gyuyoung Kim 2016-08-08 08:21:26 PDT
Created attachment 285566 [details]
Patch
Comment 2 Csaba Osztrogonác 2016-08-08 08:35:21 PDT
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")
Comment 3 Daniel Bates 2016-08-08 11:39:57 PDT
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.
Comment 4 Gyuyoung Kim 2016-08-08 17:49:12 PDT
Created attachment 285621 [details]
Patch
Comment 5 Gyuyoung Kim 2016-08-08 17:51:37 PDT
(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")
Comment 6 Csaba Osztrogonác 2016-08-08 23:40:17 PDT
Comment on attachment 285621 [details]
Patch

LGTM
Comment 7 WebKit Commit Bot 2016-08-09 00:26:56 PDT
Comment on attachment 285621 [details]
Patch

Clearing flags on attachment: 285621

Committed r204277: <http://trac.webkit.org/changeset/204277>
Comment 8 WebKit Commit Bot 2016-08-09 00:27:01 PDT
All reviewed patches have been landed.  Closing bug.