[waffle] [PATCH 1/2] waffle: add support for building Waffle using NaCl toolchain
Tapani
tapani.palli at intel.com
Mon Dec 15 22:34:15 PST 2014
On 12/15/2014 10:23 PM, Emil Velikov wrote:
> On 15/12/14 12:59, Tapani Pälli 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 but currently toolchain is fixed to glibc and target
>> architecture hardcoded as 64bit x86.
>>
> If I understand things correctly, you're hard-coding within the cmake
> what is essentially the toolchain file. I would kindly ask you to not do
> that.
Huh, I see I've missed this concept. It looks like toolchain file makes
a lot of sense here, I will move into using that. Then we can avoid
having a if/else mess when selecting between arm/x86/x86_64 by having
separate toolchain files.
Do you think I should add CMAKE_TOOLCHAIN_FILE selection somehow
available in cmake ncurses ui or should I rather use ${waffle_has_nacl}
to setup CMAKE_TOOLCHAIN_FILE point to the correct file during make?
>> Example command line to configure a build:
>>
>> cmake -Dwaffle_has_nacl=ON \
>> -Dnacl_sdk_path=/home/tpalli/nacl/nacl_sdk \
>> -DCMAKE_BUILD_TYPE=Release \
>> .
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>> Options.cmake | 13 +++++++++++++
>> cmake/Modules/WaffleDefineCompilerFlags.cmake | 23 +++++++++++++++++++++++
>> cmake/Modules/WaffleDefineInternalOptions.cmake | 4 ++++
>> cmake/Modules/WaffleValidateOptions.cmake | 11 +++++++++--
>> examples/CMakeLists.txt | 18 ++++++++++--------
>> src/CMakeLists.txt | 7 +++++--
>> src/waffle/CMakeLists.txt | 10 ++++++++++
>> 7 files changed, 74 insertions(+), 12 deletions(-)
>>
>> diff --git a/Options.cmake b/Options.cmake
>> index c316070..a7f4c26 100644
>> --- a/Options.cmake
>> +++ b/Options.cmake
>> @@ -28,6 +28,19 @@ 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()
>> +
>> +# When building for NaCl, disable uncompatible backends.
>> +if(waffle_has_nacl)
>> + set(waffle_has_glx OFF)
>> + set(waffle_has_x11_egl OFF)
>> + set(waffle_has_gbm OFF)
>> + set(waffle_has_wayland OFF)
>> endif()
> s/uncompatible/incompatible/
>
> Is that incompatibility a temporary or a permanent one ?
> I was under the impression by replacing the dynamic linking with
> dlopen/dlsym we essentially make waffle more versatile.
I consider this permanent. By compiling with the nacl toolchain your
executable will only work when running within native client environment
and there is no x11 or Wayland there, it's just the interfaces with browser.
> Afaict you will need a check in WaffleValidateOptions.cmake as the user
> can override the values you set in here.
I believe I had these there but there they did not get *forced* OFF. I
will try again, I'm quite new with cmake so I was not sure in which
order these files get read.
>>
>> option(waffle_build_tests "Build tests" ON)
>> diff --git a/cmake/Modules/WaffleDefineCompilerFlags.cmake b/cmake/Modules/WaffleDefineCompilerFlags.cmake
>> index 710b7e0..79d214c 100644
>> --- a/cmake/Modules/WaffleDefineCompilerFlags.cmake
>> +++ b/cmake/Modules/WaffleDefineCompilerFlags.cmake
>> @@ -122,6 +122,29 @@ if(waffle_on_linux)
>> add_definitions(-DWAFFLE_HAS_TLS_MODEL_INITIAL_EXEC)
>> endif()
>>
>> + if(waffle_has_nacl)
>> + add_definitions(-DWAFFLE_HAS_NACL)
>> +
>> + # setup architecture and toolchain to use, note that this is
>> + # currently hardcoded for 64bit x86 with glibc toolchain
>> + set(nacl_target_arch "x86_64")
>> + set(nacl_ports "glibc_x86_64")
>> + set(nacl_toolchain "linux_x86_glibc")
>> +
>> + # setup paths for nacl
>> + set(nacl_root "${nacl_sdk_path}/${nacl_version}")
>> + set(nacl_toolpath "${nacl_root}/toolchain/${nacl_toolchain}/bin")
>> +
>> + # setup compilers from toolchain
>> + set(CMAKE_C_COMPILER "${nacl_toolpath}/${nacl_target_arch}-nacl-gcc")
>> + set(CMAKE_CXX_COMPILER "${nacl_toolpath}/${nacl_target_arch}-nacl-g++")
>> +
>> + # setup nacl include and library paths
>> + set(nacl_INCLUDE_DIRS "${nacl_INCLUDE_DIRS} ${nacl_sdk_path}/${nacl_version}/include")
>> + set(nacl_LIBS "${nacl_sdk_path}/${nacl_version}/lib/${nacl_ports}/${CMAKE_BUILD_TYPE}")
>> + set(nacl_LDFLAGS "-L${nacl_LIBS} -lppapi_cpp -lppapi -lpthread -ldl")
>> + endif()
>> +
> See the toolchain comment above. Imho adding a toolchain file to git,
> similar to the Archlinux/Debian build scripts, won't be a bad idea.
Yep, I'll follow this approach!
>> add_definitions(-D_XOPEN_SOURCE=600)
>> endif()
>>
>> diff --git a/cmake/Modules/WaffleDefineInternalOptions.cmake b/cmake/Modules/WaffleDefineInternalOptions.cmake
>> index 3ef7a25..34dbb61 100644
>> --- a/cmake/Modules/WaffleDefineInternalOptions.cmake
>> +++ b/cmake/Modules/WaffleDefineInternalOptions.cmake
>> @@ -9,3 +9,7 @@ if(waffle_has_glx OR waffle_has_x11_egl)
>> else()
>> set(waffle_has_x11 FALSE)
>> endif()
>> +
>> +if(waffle_has_nacl)
>> + set(waffle_build_tests FALSE)
>> +endif()
> Move this to Options.cmake.
OK
>> diff --git a/cmake/Modules/WaffleValidateOptions.cmake b/cmake/Modules/WaffleValidateOptions.cmake
>> index ea60b0e..69eb34a 100644
>> --- a/cmake/Modules/WaffleValidateOptions.cmake
>> +++ b/cmake/Modules/WaffleValidateOptions.cmake
>> @@ -46,11 +46,13 @@ 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_gbm)
>> if(NOT gbm_FOUND)
>> @@ -122,6 +124,11 @@ if(waffle_on_linux)
>> message(FATAL_ERROR "x11_egl dependency is missing: ${x11_egl_missing_deps}")
>> endif()
>> endif()
>> + if(waffle_has_nacl)
>> + if(NOT EXISTS ${nacl_sdk_path})
>> + message(FATAL_ERROR "NaCl SDK path not found : ${nacl_sdk_path}")
>> + endif()
>> + endif()
> Perhaps you can check a header/library's presence and error out.
> Otherwise thing will blow up, if one makes a typo while setting the
> variable.
But if wanted header is missing we need to blow up anyway? I tested
having a invalid path and as user experience it felt ok because I'm
printing there the path and it's easy to see what went wrong. We really
do need this path to be able to point to different directories under it.
> Imho you can add an extra check here:
> if(waffle_build_tests)
> message(FATAL_ERROR "Building the tests with the NaCl backend is not
> supported")
> endif()
I'll add this.
>
>> elseif(waffle_on_mac)
>> if(waffle_has_gbm)
>> message(FATAL_ERROR "Option is not supported on Darwin: waffle_has_gbm.")
>> diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt
>> index 281ef47..ede3797 100644
>> --- a/examples/CMakeLists.txt
>> +++ b/examples/CMakeLists.txt
>> @@ -11,7 +11,7 @@ install(
>> # Target: simple-x11-egl (executable)
>> # ----------------------------------------------------------------------------
>>
>> -if(waffle_on_linux)
>> +if((waffle_on_linux) AND (waffle_has_x11_egl))
> That's a nice independent fix. Maybe split it out ?
Sure, it can be done separately.
>> add_executable(simple-x11-egl simple-x11-egl.c)
>> target_link_libraries(simple-x11-egl ${waffle_libname})
>> endif()
>> @@ -20,12 +20,14 @@ endif()
>> # Target: gl_basic (executable)
>> # ----------------------------------------------------------------------------
>>
>> -add_executable(gl_basic gl_basic.c)
>> -target_link_libraries(gl_basic ${waffle_libname} ${GETOPT_LIBRARIES})
>> +if(!waffle_has_nacl)
>> + add_executable(gl_basic gl_basic.c)
>> + target_link_libraries(gl_basic ${waffle_libname} ${GETOPT_LIBRARIES})
> Adding the following seems shorter/cleaner, imho.
>
> if(waffle_has_nacl)
> return()
> endif()
Yes, this looks cleaner.
>>
>> -if(waffle_on_mac)
>> - set_target_properties(gl_basic
>> - PROPERTIES
>> - COMPILE_FLAGS "-ObjC"
>> - )
>> + if(waffle_on_mac)
>> + set_target_properties(gl_basic
>> + PROPERTIES
>> + COMPILE_FLAGS "-ObjC"
>> + )
>> + endif()
>> endif()
>> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
>> index aa6d3c3..a47ef10 100644
>> --- a/src/CMakeLists.txt
>> +++ b/src/CMakeLists.txt
>> @@ -1,6 +1,9 @@
>> -if(waffle_build_tests)
>> +if((waffle_build_tests) AND (!waffle_has_nacl))
>> add_subdirectory(waffle_test)
>> endif()
>>
> With the WaffleValidateOptions.cmake change you can drop this.
I'm currently setting waffle_build_tests OFF in
WaffleDefineInternalOptions.cmake but I noticed it did not really turn
it OFF for this check here, that is why I added extra check :) I will
try WaffleValidateOptions.cmake instead.
>> -add_subdirectory(utils)
>> +if(!waffle_has_nacl)
>> + add_subdirectory(utils)
>> +endif()
>> +
> Replace ! with NOT. Optionally add the following in utils/CMakefile.txt
>
> if(waffle_has_nacl)
> return()
> endif()
This seems cleaner approach, I'll add this.
>
>> add_subdirectory(waffle)
>> diff --git a/src/waffle/CMakeLists.txt b/src/waffle/CMakeLists.txt
>> index d76e029..b4dd63f 100644
>> --- a/src/waffle/CMakeLists.txt
>> +++ b/src/waffle/CMakeLists.txt
>> @@ -25,6 +25,7 @@ include_directories(
>> ${gl_INCLUDE_DIRS}
>> ${GLEXT_INCLUDE_DIR}
>> ${libudev_INCLUDE_DIRS}
>> + ${nacl_INCLUDE_DIRS}
>> ${wayland-client_INCLUDE_DIRS}
>> ${wayland-egl_INCLUDE_DIRS}
>> ${x11-xcb_INCLUDE_DIRS}
>> @@ -55,6 +56,11 @@ if(waffle_on_linux)
>> ${libudev_LDFLAGS}
>> )
>> endif()
>> + if(waffle_has_nacl)
>> + list(APPEND waffle_libdeps
>> + ${nacl_LDFLAGS}
>> + )
>> + endif()
>> endif()
>>
>> set(waffle_sources
>> @@ -278,6 +284,10 @@ function(add_unittest unittest_name)
>> return()
>> endif()
>>
>> + if(waffle_has_nacl)
>> + return()
>> + endif()
>> +
> With the WaffleValidateOptions.cmake change you can drop this hunk.
>
> Nicely done Tapani. Must admit that you're work with cmake is way better
> than my first attempts :P
Thanks for the review!
> -Emil
>
// Tapani
More information about the waffle
mailing list