Summary: | Do not check if WebKitLibraries folder exists on EFL and GTK | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||
Component: | Tools / Tests | Assignee: | 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
Gyuyoung Kim
2016-08-08 08:19:33 PDT
Created attachment 285566 [details]
Patch
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 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. Created attachment 285621 [details]
Patch
(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 on attachment 285621 [details]
Patch
LGTM
Comment on attachment 285621 [details] Patch Clearing flags on attachment: 285621 Committed r204277: <http://trac.webkit.org/changeset/204277> All reviewed patches have been landed. Closing bug. |