[waffle] [PATCH v2 2/4] waffle: add support for building Waffle using NaCl toolchain

[Emil Velikov]

On 29/12/14 07:47, Tapani Pälli wrote:
> 
> 
> On 12/21/2014 03:53 PM, Emil Velikov wrote:
>> On 17 December 2014 at 10:17, Tapani Pälli <tapani.palli at intel.com>
>> wrote:
>>> Patch adds changes required to use NaCl compiler and libraries to
>>> build Waffle. Build can be configured to use specific version of
>>> the NaCl SDK, toolchain for the build needs to be selected with
>>> cmake variable CMAKE_TOOLCHAIN_FILE.
>>>
>>> Example command line to configure a build:
>>>
>>> cmake -Dwaffle_has_nacl=ON \
>>>        -Dnacl_sdk_path=/home/tpalli/nacl/nacl_sdk \
>>>        -Dnacl_version=pepper_39 \
>> Tbh I'm not a huge fan of having those piped via the top cmake into
>> the toolchain file.
>>
>> Quick look indicates that nacl_sdk as of Dec 2011 has/is defining
>> NACL_SDL_ROOT.
>> Perhaps we can reuse that one and error out if it's missing.
>> Considering we can use (have tested only) a single version we could
>> just hardcode it into the toolchain file ?
> 
> Problem is that this path is not in environment and user can choose to
> install to whatever location. I've tested 37, 38 and 39 which is the
> latest stable version. It seems there are new versions coming quite
> often (?) and there are unstable versions available for the cool guys so
> I would not hardcode it.
> 
> 
Wrt the nacl_version, I've assumed that you've tested it only against
39. That said I completely withdraw my comment on that one :)

>>>       
>>> -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain-nacl-linux-glibc-x86_64.cmake \
>>>        -DCMAKE_BUILD_TYPE=Release \
>>>        .
>>>
>>> v2: cmake fixes, create toolchain files for nacl (Emil Velikov)
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> ---
>>>   Options.cmake                                 |  5 +++++
>>>   cmake/Modules/WaffleDefineCompilerFlags.cmake |  4 ++++
>>>   cmake/Modules/WaffleValidateOptions.cmake     | 31
>>> +++++++++++++++++++++++++--
>>>   cmake/toolchain-nacl-linux-glibc-x86_32.cmake | 29
>>> +++++++++++++++++++++++++
>>>   cmake/toolchain-nacl-linux-glibc-x86_64.cmake | 29
>>> +++++++++++++++++++++++++
>>>   examples/CMakeLists.txt                       |  4 ++++
>>>   src/utils/CMakeLists.txt                      |  4 ++++
>>>   src/waffle/CMakeLists.txt                     |  6 ++++++
>>>   8 files changed, 110 insertions(+), 2 deletions(-)
>>>   create mode 100644 cmake/toolchain-nacl-linux-glibc-x86_32.cmake
>>>   create mode 100644 cmake/toolchain-nacl-linux-glibc-x86_64.cmake
>>>
>>> diff --git a/Options.cmake b/Options.cmake
>>> index c316070..4f097a0 100644
>>> --- a/Options.cmake
>>> +++ b/Options.cmake
>>> @@ -28,6 +28,11 @@ if(waffle_on_linux)
>>>       option(waffle_has_wayland "Build support for Wayland"
>>> ${wayland_default})
>>>       option(waffle_has_x11_egl "Build support for X11/EGL"
>>> ${x11_egl_default})
>>>       option(waffle_has_gbm "Build support for GBM" ${gbm_default})
>>> +    option(waffle_has_nacl "Build support for NaCl" OFF)
>>> +
>>> +    # NaCl specific settings.
>>> +    set(nacl_sdk_path "" CACHE STRING "Set nacl_sdk path here")
>>> +    set(nacl_version "pepper_39" CACHE STRING "Set NaCl bundle here")
>>>   endif()
>>>
>>>   option(waffle_build_tests "Build tests" ON)
>>> diff --git a/cmake/Modules/WaffleDefineCompilerFlags.cmake
>>> b/cmake/Modules/WaffleDefineCompilerFlags.cmake
>>> index 710b7e0..4bdc0df 100644
>>> --- a/cmake/Modules/WaffleDefineCompilerFlags.cmake
>>> +++ b/cmake/Modules/WaffleDefineCompilerFlags.cmake
>>> @@ -122,6 +122,10 @@ if(waffle_on_linux)
>>>           add_definitions(-DWAFFLE_HAS_TLS_MODEL_INITIAL_EXEC)
>>>       endif()
>>>
>>> +    if(waffle_has_nacl)
>>> +        add_definitions(-DWAFFLE_HAS_NACL)
>>> +    endif()
>>> +
>>>       add_definitions(-D_XOPEN_SOURCE=600)
>>>   endif()
>>>
>>> diff --git a/cmake/Modules/WaffleValidateOptions.cmake
>>> b/cmake/Modules/WaffleValidateOptions.cmake
>>> index ea60b0e..1275463 100644
>>> --- a/cmake/Modules/WaffleValidateOptions.cmake
>>> +++ b/cmake/Modules/WaffleValidateOptions.cmake
>>> @@ -46,11 +46,38 @@ endif()
>>>
>>>   if(waffle_on_linux)
>>>       if(NOT waffle_has_glx AND NOT waffle_has_wayland AND
>>> -       NOT waffle_has_x11_egl AND NOT waffle_has_gbm)
>>> +       NOT waffle_has_x11_egl AND NOT waffle_has_gbm AND
>>> +       NOT waffle_has_nacl)
>>>           message(FATAL_ERROR
>>>                   "Must enable at least one of: "
>>>                   "waffle_has_glx, waffle_has_wayland, "
>>> -                "waffle_has_x11_egl, waffle_has_gbm.")
>>> +                "waffle_has_x11_egl, waffle_has_gbm, "
>>> +                "waffle_has_nacl.")
>>> +    endif()
>>> +    if(waffle_has_nacl)
>>> +        if(NOT EXISTS ${nacl_sdk_path})
>>> +            message(FATAL_ERROR "NaCl SDK path not found :
>>> ${nacl_sdk_path}")
>>> +        endif()
>>> +
>> In the future as we add a proper build check, we can add it into the
>> auto-detection. Can you add a single line of comment ?
> 
> I don't believe auto-detection would be good idea in this case.
> 
Care to elaborate as to why ? The idea (check for a header/lib)
mentioned into the other email sounds OK but maybe I've missed something.

>>> +        if(NOT EXISTS ${CMAKE_TOOLCHAIN_FILE})
>>> +            message(FATAL_ERROR "Toolchain for Nacl not found. This
>>> must be "
>>> +                                "configured using
>>> CMAKE_TOOLCHAIN_FILE.")
>>> +        endif()
>>> +
>> Don't think we need this check, but I'll leave it up-to Chad to be the
>> final judge.
> 
> This is fine for me, I left it there to have a bit more verbose output
> for the user as without it things blow up and it may not be clear for
> user that the file should be specified for compilation to work.
> 
>>> +        # Warn the user that building tests is disabled.
>>> +        if(waffle_build_tests)
>>> +            message(WARNING "Building the tests with the NaCl backend "
>>> +                            "is not supported, skipping tests.")
>>> +            set(waffle_build_tests OFF)
>>> +        endif()
>>> +
>>> +        # When building for NaCl, disable incompatible backends.
>>> +        set(waffle_has_gbm OFF)
>>> +        set(waffle_has_egl OFF)
>>> +        set(waffle_has_glx OFF)
>>> +        set(waffle_has_x11 OFF)
>>> +        set(waffle_has_x11_egl OFF)
>>> +        set(waffle_has_wayland OFF)
>> The platform supports dlopen/dlsym, so I'm quite confident that we can
>> get it working.
>> That will obviously can be looked at a later stage. Can you add a
>> small TODO line ?
> 
> Only the glibc version supports dlopen, pnacl does not. Also I don't see
> the use case of calling waffle from javascript to open a Wayland window?
> I don't think Chrome security wise would allow such to happen. The
> realistic use case is to show content on the browser window. Also the
> resulted waffle binary can not be expected to work in any other system
> than within chrome.
> 
I was under the impression that nacl and pnacl will be separate
targets/build options. Plus by going the dlopen route (in the long term)
we can build a *single* library that depends only on
lib{c,dl,pthread}.so and can be used with either - wayland/chrome..

Yet it seems that the nacl library needs a few extra libs (ppapi_cpp,
ppapi) which are required by/part of the SDK thus the above hope goes
down in flames :'(


-Emil