Bug 187133 – Switch OS(FUCHSIA) to using JSCOnly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Switch OS(FUCHSIA) to using JSCOnly

[Adam Barth]

Created attachment 343792 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 343792 [details]
Patch

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

> ChangeLog:9
> +        Rather than creating a Fuchsia port, OS(FUCHSIA) now uses the JSCOnly
> +        port.

OK, then, I think we should remove Fuchsia related things in webkitdir.pm. We can pass cmakeargs by using `--cmakeargs`.
So basically, `Tools/Scripts/build-jsc --jsc-only --cmakeargs="-DCMAKE_TOOLCHAIN_FILE=XXX"` should work.

> Tools/Scripts/webkitdirs.pm:152
> +my $targetingFuchsia;

If we use JSCOnly port in Fuchsia OS, I think we do not need to check Fuchsia related things in webkitdirs.pm. We can just use `--jsc-only`, and we can remove Fuchsia related things in webkitdirs.pm.

> Tools/Scripts/webkitdirs.pm:2212
> +    my $toolchainFile = $ENV{'CMAKE_TOOLCHAIN_FILE'};
> +    if ($toolchainFile) {
> +        push @args, "-DCMAKE_TOOLCHAIN_FILE=$toolchainFile";
> +    }

Why not passing it with `cmakeargs`? `Tools/Scripts/build-jsc --cmakeargs="-DCMAKE_TOOLCHAIN_FILE=XXX"`

[Daniel Bates]

Comment on attachment 343792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343792&action=review
> Tools/Scripts/webkitdirs.pm:2209
> +    my $toolchainFile = $ENV{'CMAKE_TOOLCHAIN_FILE'};

Minor: ‘ (single quote) => “ (double quote)

Although unwritten, we prefer double-quoted string literals over single quoted ones unless we explicitly do not want string interpolation or using single quotes reduces the need to escape characters in the string.

> Tools/Scripts/webkitdirs.pm:2211
> +        push @args, "-DCMAKE_TOOLCHAIN_FILE=$toolchainFile";

This will not work correctly if the path has spaces in its name. We should take a similar approach as done for -DCMAKE_INSTALL_PREFIX and surround $toolchainFile in quotes. Is there a better way to write this code to handle a path that has embedded space characters?

[Adam Barth]

Created attachment 343797 [details]
Patch

[Adam Barth]

> OK, then, I think we should remove Fuchsia related things in webkitdir.pm.
> We can pass cmakeargs by using `--cmakeargs`.
> So basically, `Tools/Scripts/build-jsc --jsc-only
> --cmakeargs="-DCMAKE_TOOLCHAIN_FILE=XXX"` should work.

Neat!

> If we use JSCOnly port in Fuchsia OS, I think we do not need to check
> Fuchsia related things in webkitdirs.pm. We can just use `--jsc-only`, and
> we can remove Fuchsia related things in webkitdirs.pm.

Yes, that seems to work.  Done.

> Why not passing it with `cmakeargs`? `Tools/Scripts/build-jsc
> --cmakeargs="-DCMAKE_TOOLCHAIN_FILE=XXX"`

Good idea.

> Minor: ‘ (single quote) => “ (double quote)

Removed.

> Although unwritten, we prefer double-quoted string literals over single
> quoted ones unless we explicitly do not want string interpolation or using
> single quotes reduces the need to escape characters in the string.

Got it.  I've removed this code, in any case.

> > Tools/Scripts/webkitdirs.pm:2211
> > +        push @args, "-DCMAKE_TOOLCHAIN_FILE=$toolchainFile";
> 
> This will not work correctly if the path has spaces in its name. We should
> take a similar approach as done for -DCMAKE_INSTALL_PREFIX and surround
> $toolchainFile in quotes. Is there a better way to write this code to handle
> a path that has embedded space characters?

Apparently the best way to write this code is to delete it.  :)

[Yusuke Suzuki]

Comment on attachment 343797 [details]
Patch

r=me, nice!

[Daniel Bates]

Comment on attachment 343797 [details]
Patch

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

> Tools/ChangeLog:11
> +        * Scripts/webkitdirs.pm:

Please update this ChangeLog as this function listing is out of date. It would be good to explain that we are removing code from webkitdir.pm (and why) as well as explain how to build by passing --cmakeargs “...” to build-webkit.

[Adam Barth]

> Please update this ChangeLog as this function listing is out of date.

Done.

> It would be good to explain that we are removing code from webkitdir.pm (and
> why) as well as explain how to build by passing --cmakeargs “...” to
> build-webkit.

I've been putting that information on the wiki:

https://trac.webkit.org/wiki/Fuchsia

[Adam Barth]

Created attachment 343799 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 343799 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 343799 [details]
Patch

Clearing flags on attachment: 343799

Committed r233303: <https://trac.webkit.org/changeset/233303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41567155>