Bug 160691 – Skip to check directories existence in build-webkit

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

[Gyuyoung Kim]

EFL and GTK ports only use WebKit2 port, build-webkit script has been checking if WebKit1 directory exists. Remove the check.

[Gyuyoung Kim]

Created attachment 285639 [details]
Patch

[Gyuyoung Kim]

Created attachment 285640 [details]
Patch

[Gyuyoung Kim]

Created attachment 285641 [details]
Patch

[Alex Christensen]

Comment on attachment 285641 [details]
Patch

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

> Tools/Scripts/build-webkit:155
> +push @projects, "Source/WebKit2" if isEfl() or isGtk();

WebKit2 is needed for Mac and iOS, too.

> Tools/Scripts/webkitdirs.pm:-171
> -    until ((-d File::Spec->catdir($sourceDir, "Source") && -d File::Spec->catdir($sourceDir, "Source", "WebCore") && -d File::Spec->catdir($sourceDir, "Source", "WebKit")) || (-d File::Spec->catdir($sourceDir, "Internal") && -d File::Spec->catdir($sourceDir, "OpenSource")))

Why not just change this to WebKit2 instead of removing it?

[Gyuyoung Kim]

Created attachment 285693 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #4)
> Comment on attachment 285641 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285641&action=review
> 
> > Tools/Scripts/build-webkit:155
> > +push @projects, "Source/WebKit2" if isEfl() or isGtk();
> 
> WebKit2 is needed for Mac and iOS, too.

Missed it. Fixed.

> > Tools/Scripts/webkitdirs.pm:-171
> > -    until ((-d File::Spec->catdir($sourceDir, "Source") && -d File::Spec->catdir($sourceDir, "Source", "WebCore") && -d File::Spec->catdir($sourceDir, "Source", "WebKit")) || (-d File::Spec->catdir($sourceDir, "Internal") && -d File::Spec->catdir($sourceDir, "OpenSource")))
> 
> Why not just change this to WebKit2 instead of removing it?

Because I thought that *WebKit2* may influence on only WK1 ports. If not, it would be great to change it with *WebKit2*.

[Gyuyoung Kim]

Alex, ping ?

[Michael Catanzaro]

Comment on attachment 285693 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:171
> +    until ((-d File::Spec->catdir($sourceDir, "Source") && -d File::Spec->catdir($sourceDir, "Source", "WebCore") && -d File::Spec->catdir($sourceDir, "Source", "WebKit2")) || (-d File::Spec->catdir($sourceDir, "Internal") && -d File::Spec->catdir($sourceDir, "OpenSource")))

So this makes it possible to run build-webkit from a tarball that doesn't contain the WebKit directory... but makes it impossible to run it from a tarball that doesn't contain WebKit2. I would just remove it. Or replace it with JavaScriptCore; all ports will surely have that.

[Michael Catanzaro]

Downstream ports will need to patch this.

[Konstantin Tokarev]

CMake checks presense of all source files in configuration time, so it will bail out if e.g. WebKit2 directory is missing when build requires it. Shouldn't we just omit this check if isCMakeBuild() is true?

[Gyuyoung Kim]

(In reply to comment #10)
> CMake checks presense of all source files in configuration time, so it will
> bail out if e.g. WebKit2 directory is missing when build requires it.
> Shouldn't we just omit this check if isCMakeBuild() is true?

As we talked on irc, let's postpone to land this patch for a while. Let me find better  solutions as you guys suggest.

[Gyuyoung Kim]

Created attachment 285828 [details]
Patch

[Gyuyoung Kim]

I apply annulen's suggestion to the patch for downstream ports. CMake ports don't need to check directories because CMake checks whether directories are there again.

[Gyuyoung Kim]

Comment on attachment 285828 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2076
> +    return 1 unless isAppleMacWebKit() || isIOSWebKit() || isWinCairo();

Please let me know other ports should be added to this list. e.g. isAppleWinWebKit() ?

[Konstantin Tokarev]

Comment on attachment 285828 [details]
Patch

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

>> Tools/Scripts/webkitdirs.pm:2076
>> +    return 1 unless isAppleMacWebKit() || isIOSWebKit() || isWinCairo();
> 
> Please let me know other ports should be added to this list. e.g. isAppleWinWebKit() ?

This change is wrong, it makes WinCairo non-cmake. Also isIOSWebKit() is already covered by isAppleMacWebKit(), which is defined as (portName() eq Mac) || isIOSWebKit()

[Gyuyoung Kim]

Comment on attachment 285828 [details]
Patch

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

>>> Tools/Scripts/webkitdirs.pm:2076
>>> +    return 1 unless isAppleMacWebKit() || isIOSWebKit() || isWinCairo();
>> 
>> Please let me know other ports should be added to this list. e.g. isAppleWinWebKit() ?
> 
> This change is wrong, it makes WinCairo non-cmake. Also isIOSWebKit() is already covered by isAppleMacWebKit(), which is defined as (portName() eq Mac) || isIOSWebKit()

Did win-cairo port adopt cmake ? I didn't know that. Thanks.

[Gyuyoung Kim]

Created attachment 285901 [details]
Patch

[Konstantin Tokarev]

LGTM

[Darin Adler]

Comment on attachment 285901 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:170
>      # defined by containing Sources, WebCore, and WebKit

Should fix this comment.

[Gyuyoung Kim]

Committed r204444: <http://trac.webkit.org/changeset/204444>

[Gyuyoung Kim]

(In reply to comment #19)
> Comment on attachment 285901 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285901&action=review
> 
> > Tools/Scripts/webkitdirs.pm:170
> >      # defined by containing Sources, WebCore, and WebKit
> 
> Should fix this comment.

Fixed.