Bug 143084 – Support building WTF on Windows without Cygwin

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Support building WTF on Windows without Cygwin

[Myles C. Maxfield]

Created attachment 249476 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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!

[Brent Fulgham]

(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. :-)

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 249576 [details]
Patch

[Myles C. Maxfield]

Created attachment 249578 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 249578 [details]
Patch

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

> Source/WTF/ChangeLog:1
> [phone]  S. Litherum  <[e-mail]>

???

[Myles C. Maxfield]

Comment on attachment 249578 [details]
Patch

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

>> Source/WTF/ChangeLog:1
>> [phone]  S. Litherum  <[e-mail]>
> 
> ???

Whoops

[Brent Fulgham]

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!

[Myles C. Maxfield]

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.

[Brent Fulgham]

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.

[Myles C. Maxfield]

https://trac.webkit.org/r182089