Bug 160659

Summary: Do not check if WebKitLibraries folder exists on EFL and GTK
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: Tools / TestsAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, lforschler, ossy
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

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.