Bug 143084 - Support building WTF on Windows without Cygwin
Summary: Support building WTF on Windows without Cygwin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-25 23:14 PDT by Myles C. Maxfield
Modified: 2015-03-27 16:09 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.27 KB, patch)
2015-03-25 23:16 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (13.31 KB, patch)
2015-03-27 10:08 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (13.33 KB, patch)
2015-03-27 10:13 PDT, Myles C. Maxfield
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-03-25 23:14:27 PDT
Support building WTF on Windows without Cygwin
Comment 1 Myles C. Maxfield 2015-03-25 23:16:33 PDT
Created attachment 249476 [details]
Patch
Comment 2 Brent Fulgham 2015-03-25 23:49:03 PDT
Comment on attachment 249476 [details]
Patch

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

This looks really good. I want to see these various paths changed to use File::Spec so we can avoid problems flipping between UNIX and Windows path styles.

> Source/WTF/WTF.vcxproj/WTFGenerated.make:2
> +    type NUL > "%ConfigurationBuildDir%\buildfailed"

Please precede the line with '@' so we don't get output in the build log.

> Tools/Scripts/update-webkit-support-libs:-47
> -my $zipDirectory = toUnixPath($ENV{'WEBKITSUPPORTLIBRARIESZIPDIR'}) || $sourceDir;

We should use File::Spec on these paths so that they are Unix-y on Unix, windows on windows, and cygwin-y on cygwin.

We should make that same change elsewhere, too.

> Tools/Scripts/update-webkit-support-libs:52
> +my $webkitLibrariesDir = $ENV{'WEBKIT_LIBRARIES'} || "$sourceDir/WebKitLibraries/win";

Ditto. We should have something like:

File::

> Tools/Scripts/update-webkit-support-libs:53
>  my $versionFile = $file . "Version";

... File::Spec->catdir($source, 'WebKitLibraries', 'win').

Sorry for the bad formatting, I'm doing this via iPad!

> Tools/Scripts/update-webkit-support-libs:93
> +    copy($_, $destination);

Nice!

> Tools/Scripts/update-webkit-support-libs:121
> +    chomp(my $zipFileVersion = $zip->contents("$file/win/$versionFile"));

Nice! But let's switch to File::Spec.

> Tools/Scripts/webkitdirs.pm:1466
> +        }

This is no longer true. We should remove this warning.
Comment 3 Brent Fulgham 2015-03-25 23:52:59 PDT
EWS says it hates this patch. :)

Let me try using it on my machine tomorrow and I'll see if I using the File::Spec changed makes it happy again.

Thanks for working on this!
Comment 4 Brent Fulgham 2015-03-26 20:35:30 PDT
(In reply to comment #3)
> EWS says it hates this patch. :)

Wrong! EWS is the problem. It appears to be using an old support library. I'll fix that tomorrow.

I still stand behind my other suggestions, but I think the patch is fundamentally good. :-)
Comment 5 Myles C. Maxfield 2015-03-27 09:34:42 PDT
Comment on attachment 249476 [details]
Patch

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

Overall comments

>> Tools/Scripts/update-webkit-support-libs:121
>> +    chomp(my $zipFileVersion = $zip->contents("$file/win/$versionFile"));
> 
> Nice! But let's switch to File::Spec.

This path is actually inside the .zip file, where paths are defined to be using / instead of whatever the platform uses. Changing this to File::Spec actually breaks it.
Comment 6 Myles C. Maxfield 2015-03-27 10:08:21 PDT
Created attachment 249576 [details]
Patch
Comment 7 Myles C. Maxfield 2015-03-27 10:13:18 PDT
Created attachment 249578 [details]
Patch
Comment 8 Csaba Osztrogonác 2015-03-27 10:40:23 PDT
Comment on attachment 249578 [details]
Patch

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

> Source/WTF/ChangeLog:1
> +2015-03-27  S. Litherum  <litherum@gmail.com>

???
Comment 9 Myles C. Maxfield 2015-03-27 10:46:21 PDT
Comment on attachment 249578 [details]
Patch

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

>> Source/WTF/ChangeLog:1
>> +2015-03-27  S. Litherum  <litherum@gmail.com>
> 
> ???

Whoops
Comment 10 Brent Fulgham 2015-03-27 13:52:36 PDT
Comment on attachment 249578 [details]
Patch

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

Looks great! I think you should change the catfile->catdir when dealing with directories just to future-proof things, and to deal with cases where the shell variables are empty strings (catdir handles that properly, catfile may or may not).

> Tools/Scripts/update-webkit-dependency:73
> +my $webkitLibrariesDir = $ENV{'WEBKIT_LIBRARIES'} || File::Spec->catfile($sourceDir, "WebKitLibraries", "win");

Nit: This should be "catdir" since it's not a file. But I don't think it matters in practice.

> Tools/Scripts/update-webkit-support-libs:85
> +    my $relativeName = File::Spec->abs2rel($File::Find::name, File::Spec->catfile($tmpAbsDir, "$file", "win"));

This should be catdir.

> Tools/Scripts/update-webkit-support-libs:121
> +    chomp(my $zipFileVersion = $zip->contents("$file/win/$versionFile"));

This should be a File::Spec->catfile.

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

These should be catdir.

> Tools/Scripts/webkitdirs.pm:154
> +    $sourceDir = File::Spec->catfile($sourceDir, "OpenSource") if -d File::Spec->catfile($sourceDir, "OpenSource");

these should be catdir.

> Tools/Scripts/webkitdirs.pm:1436
> +    return File::Spec->catfile(windowsSourceDir(), "Source");

catdir

> Tools/Scripts/webkitdirs.pm:1441
> +    return File::Spec->catfile(windowsSourceDir(), "WebKitLibraries", "win");

catdir

> Tools/Scripts/webkitdirs.pm:1446
> +    return File::Spec->catfile(windowsSourceDir(), "WebKitBuild");

catdir

> Tools/Scripts/webkitdirs.pm:-1466
> -

Yay!
Comment 11 Myles C. Maxfield 2015-03-27 14:12:28 PDT
Comment on attachment 249578 [details]
Patch

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

>> Tools/Scripts/update-webkit-support-libs:121
>> +    chomp(my $zipFileVersion = $zip->contents("$file/win/$versionFile"));
> 
> This should be a File::Spec->catfile.

See my reply to you in the previous version of this patch.
Comment 12 Brent Fulgham 2015-03-27 14:54:34 PDT
Comment on attachment 249578 [details]
Patch

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

>>> Tools/Scripts/update-webkit-support-libs:121
>>> +    chomp(my $zipFileVersion = $zip->contents("$file/win/$versionFile"));
>> 
>> This should be a File::Spec->catfile.
> 
> See my reply to you in the previous version of this patch.

Ah! Ok. Sounds good.
Comment 13 Myles C. Maxfield 2015-03-27 16:09:24 PDT
https://trac.webkit.org/r182089