Switch OS(FUCHSIA) to using JSCOnly
Created attachment 343792 [details] Patch
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"`
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?
Created attachment 343797 [details] Patch
> 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. :)
Comment on attachment 343797 [details] Patch r=me, nice!
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.
> 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
Created attachment 343799 [details] Patch
Comment on attachment 343799 [details] Patch r=me
Comment on attachment 343799 [details] Patch Clearing flags on attachment: 343799 Committed r233303: <https://trac.webkit.org/changeset/233303>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41567155>