EFL and GTK ports only use WebKit2 port, build-webkit script has been checking if WebKit1 directory exists. Remove the check.
Created attachment 285639 [details] Patch
Created attachment 285640 [details] Patch
Created attachment 285641 [details] Patch
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?
Created attachment 285693 [details] Patch
(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*.
Alex, ping ?
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.
Downstream ports will need to patch this.
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?
(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.
Created attachment 285828 [details] Patch
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.
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() ?
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()
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.
Created attachment 285901 [details] Patch
LGTM
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.
Committed r204444: <http://trac.webkit.org/changeset/204444>
(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.