Enabling Skia / Moterm to build with PNaCl Newlib toolchain

Enabling Skia / Moterm to build with PNaCl Newlib toolchain

To see moterm in action with PNaCl, run:
mojo/devtools/common/mojo_run --enable-multiprocess \
    "mojo:window_manager mojo:moterm_example_app.pexe"

Although moterm builds (and can run in a limited fashion),
the following fails due to a lack of mgl support.
./out/Debug/mojo_shell --enable-multiprocess \
    ./out/Debug/moterm_apptests.pexe.mojo

BUG=#431
R=mseaborn@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/473c8d00b0b052aa3eb6f734ab69a85eecd932c9

[Sean Klein, 2015-12-16 00:19:11]

Description was changed from

==========
Building Skia / Moterm with PNaCl Newlib toolchain.

mojo/devtools/common/mojo_run --enable-multiprocess \
    "mojo:window_manager mojo:moterm_example_app.pexe"

BUG=#431
==========

to

==========
Building Skia / Moterm with PNaCl Newlib toolchain.


mojo/devtools/common/mojo_run --enable-multiprocess \
    "mojo:window_manager mojo:moterm_example_app.pexe"

(This fails)
TEST=`./out/Debug/mojo_shell --enable-multiprocess
./out/Debug/moterm_apptests.pexe.mojo`
BUG=#431
==========

[Sean Klein, 2015-12-16 00:51:30]

Patchset #4 (id:40001) has been deleted

[Sean Klein, 2015-12-16 17:54:30]

Description was changed from

==========
Building Skia / Moterm with PNaCl Newlib toolchain.


mojo/devtools/common/mojo_run --enable-multiprocess \
    "mojo:window_manager mojo:moterm_example_app.pexe"

(This fails)
TEST=`./out/Debug/mojo_shell --enable-multiprocess
./out/Debug/moterm_apptests.pexe.mojo`
BUG=#431
==========

to

==========
Enabling Skia / Moterm to build with PNaCl Newlib toolchain

To see moterm in action with PNaCl, run:
mojo/devtools/common/mojo_run --enable-multiprocess \
    "mojo:window_manager mojo:moterm_example_app.pexe"

Although moterm builds (and can run in a limited fashion),
the following fails due to a lack of mgl support.
./out/Debug/mojo_shell --enable-multiprocess \
    ./out/Debug/moterm_apptests.pexe.mojo

BUG=#431
==========

[Sean Klein, 2015-12-16 17:59:52]

Patchset #5 (id:80001) has been deleted

[Sean Klein, 2015-12-16 18:01:45]

smklein@chromium.org changed reviewers:
+ mseaborn@chromium.org, phosek@chromium.org

[Mark Seaborn, 2015-12-16 21:20:37]

LGTM.  Good to see this working!

https://codereview.chromium.org/1525043002/diff/100001/apps/BUILD.gn
File apps/BUILD.gn (right):

https://codereview.chromium.org/1525043002/diff/100001/apps/BUILD.gn#newcode7
apps/BUILD.gn:7: group("portable_apps") {
Maybe add a comment to explain that "portable" means "builds OK with PNaCl"?

The other file has the comment "# Collection of examples which are currently
capable of being built as pexes."

https://codereview.chromium.org/1525043002/diff/100001/apps/BUILD.gn#newcode30
apps/BUILD.gn:30: if (mojo_use_nacl && (is_linux || is_android) && !is_asan) {
Can you drop "&& (is_linux || is_android) && !is_asan", given that mojo_use_nacl
implies those?

That's based on the definitions in build/config/nacl/config.gni:
  mojo_use_nacl_sfi = is_linux && !is_asan && !is_fnl
  mojo_use_nacl_nonsfi = (is_linux || is_android) && !is_asan && !is_fnl
  mojo_use_nacl = mojo_use_nacl_sfi || mojo_use_nacl_nonsfi

https://codereview.chromium.org/1525043002/diff/100001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/1525043002/diff/100001/skia/BUILD.gn#newcode506
skia/BUILD.gn:506: cflags += [ "-fomit-frame-pointer" ]
Why this option?  I'm certain you don't need it because it has no significance
when building bitcode.

https://codereview.chromium.org/1525043002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1525043002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:38: #if defined(SK_BUILD_FOR_WIN32)
Nit: you might need to #include include/core/SkPreConfig.h to ensure these are
#defined (to #include-what-you-use).

Or using OS_BLAH rather than SK_BUILD_FOR_BLAH might be preferred for this
non-Skia-repo code.

[Sean Klein, 2015-12-16 22:34:03]

https://codereview.chromium.org/1525043002/diff/100001/apps/BUILD.gn
File apps/BUILD.gn (right):

https://codereview.chromium.org/1525043002/diff/100001/apps/BUILD.gn#newcode7
apps/BUILD.gn:7: group("portable_apps") {
On 2015/12/16 21:20:37, Mark Seaborn wrote:
> Maybe add a comment to explain that "portable" means "builds OK with PNaCl"?
> 
> The other file has the comment "# Collection of examples which are currently
> capable of being built as pexes."

Done.

https://codereview.chromium.org/1525043002/diff/100001/apps/BUILD.gn#newcode30
apps/BUILD.gn:30: if (mojo_use_nacl && (is_linux || is_android) && !is_asan) {
On 2015/12/16 21:20:37, Mark Seaborn wrote:
> Can you drop "&& (is_linux || is_android) && !is_asan", given that
mojo_use_nacl
> implies those?
> 
> That's based on the definitions in build/config/nacl/config.gni:
>   mojo_use_nacl_sfi = is_linux && !is_asan && !is_fnl
>   mojo_use_nacl_nonsfi = (is_linux || is_android) && !is_asan && !is_fnl
>   mojo_use_nacl = mojo_use_nacl_sfi || mojo_use_nacl_nonsfi

Done.

https://codereview.chromium.org/1525043002/diff/100001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/1525043002/diff/100001/skia/BUILD.gn#newcode506
skia/BUILD.gn:506: cflags += [ "-fomit-frame-pointer" ]
On 2015/12/16 21:20:37, Mark Seaborn wrote:
> Why this option?  I'm certain you don't need it because it has no significance
> when building bitcode.

Removed. I naively figured it would improve performance, but I suppose it would
only be relevant at translation time.

https://codereview.chromium.org/1525043002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1525043002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:38: #if defined(SK_BUILD_FOR_WIN32)
On 2015/12/16 21:20:37, Mark Seaborn wrote:
> Nit: you might need to #include include/core/SkPreConfig.h to ensure these are
> #defined (to #include-what-you-use).
> 
> Or using OS_BLAH rather than SK_BUILD_FOR_BLAH might be preferred for this
> non-Skia-repo code.

Going with the "#include" option for now.

[Sean Klein, 2015-12-16 22:45:47]

Description was changed from

==========
Enabling Skia / Moterm to build with PNaCl Newlib toolchain

To see moterm in action with PNaCl, run:
mojo/devtools/common/mojo_run --enable-multiprocess \
    "mojo:window_manager mojo:moterm_example_app.pexe"

Although moterm builds (and can run in a limited fashion),
the following fails due to a lack of mgl support.
./out/Debug/mojo_shell --enable-multiprocess \
    ./out/Debug/moterm_apptests.pexe.mojo

BUG=#431
==========

to

==========
Enabling Skia / Moterm to build with PNaCl Newlib toolchain

To see moterm in action with PNaCl, run:
mojo/devtools/common/mojo_run --enable-multiprocess \
    "mojo:window_manager mojo:moterm_example_app.pexe"

Although moterm builds (and can run in a limited fashion),
the following fails due to a lack of mgl support.
./out/Debug/mojo_shell --enable-multiprocess \
    ./out/Debug/moterm_apptests.pexe.mojo

BUG=#431
R=mseaborn@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/473c8d00b0b052aa3eb6f734ab6...
==========

[Sean Klein, 2015-12-16 22:45:48]

Committed patchset #6 (id:120001) manually as
473c8d00b0b052aa3eb6f734ab69a85eecd932c9 (presubmit successful).