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

Sun Dec 28 23:28:56 PST 2014

On 12/23/2014 02:06 AM, Chad Versace wrote:
> On 12/17/2014 04:17 AM, 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, 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 \
>>        -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain-nacl-linux-glibc-x86_64.cmake \
>>        -DCMAKE_BUILD_TYPE=Release \
>
> I tried really hard to simplify the CMake cmdline, but I failed. I'm convinced
> that this is simple as it gets.
>
>> 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/cmake/toolchain-nacl-linux-glibc-x86_32.cmake b/cmake/toolchain-nacl-linux-glibc-x86_32.cmake
>
> If you rename this file toolchain-nacl.cmake, and make a few changes, then I think the same
> file can be used for every host OS (Linux, Mac, Windows) and target architecture.

OK, this sounds ideal. I'm not able to test all these though so not sure 
if everything would work.

>> new file mode 100644
>> index 0000000..3c13bb6
>> --- /dev/null
>> +++ b/cmake/toolchain-nacl-linux-glibc-x86_32.cmake
>
> Nothing about this file is specific to Linux. I think you can rename this
> file to toolchain-nacl-glibc-x86_32.cmake and, with a small change, have
> it work on Mac (and possibly Windows) too.
>
> Also, I think the architecture is more significant, and should therefore precede,
> the C library variant. That is,
>
>      toolchain-nacl-{NACL_ARCH}[-{NACL_C_LIBRARY}].cmake
>
> instead of
>
>      toolchain-nacl-{NACL_C_LIBRARY}-{NACL_ARCH}.cmake
>
>
> Otherwise, for PNaCl you end up with the weird name
>
>      toolchain-nacl-newlibc-pnacl.cmake
>
> instead of the simple name
>
>      toolchain-nacl-pnacl.cmake
>
> (The first is weird because newlibc is the only option for PNaCl. PNaCl
> doesn't support glibc).

Yes, this makes sense. I will change the naming scheme.

>> @@ -0,0 +1,29 @@
>> +#
>> +# NaCl toolchain file for 32bit x86 using glibc C library
>> +#
>> +
>> +set(CMAKE_SYSTEM_NAME Linux)
>
> Why did you choose to set CMAKE_SYSTEM_NAME to "Linux"? I expected it to be set to "NaCl"
> or left at its default value. Also, does setting it to "Linux" risk breaking NaCl builds
> done on Mac?

Leaving it to default (not setting it) produces 'Error running link 
command: No such file or directory'. I think it leaves many needed cmake 
variables undefined so it needs to be there. Using 'unknown' system such 
as 'nacl' produces lots of warnings to console that I should consult 
with cmake at www.cmake.org to add 'nacl' as one of the options, it works 
though (with little addition to WaffleDefineOS.cmake) so it can be used. 
I can make this change if such warnings are ok.

>> +set(nacl_target_arch "i686")
>> +set(nacl_ports "glibc_x86_32")
>
> You can make this file generic for Linux, Mac, and Windows by replacing
>
>> +set(nacl_toolchain "linux_x86_glibc")
>
> with
>
>      if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux)
>          set(nacl_host_os "linux")
>      else()
>          message(FATAL_ERROR "TODO: NaCl support on ${CMAKE_HOST_SYSTEM_NAME}")
>      endif()
>
>      set(nacl_toolchain "${nacl_host_os}_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++)
>> +
>> +set(CMAKE_FIND_ROOT_PATH ${nacl_root})
>> +
>> +# for FIND_PROGRAM|LIBRARY|INCLUDE use ${nacl_root} only
>> +set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM ONLY)
>> +set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
>> +set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)
>
> I think you should set CMAKE_FIND_ROOT_PATH_MODE_PROGRAM to NEVER and
> update the comment. For precedent, see the Piglit ebuild [1] in Chromium OS.
> If in the future Waffle needs to use a program (such as Python or the Go compiler
> or simply sed) to modify or generate sources at build time, then that program
> needs to be compatible with the build host's architecture and should be taken
> from the build host's PATH, not from the NaCl SDK. If Waffle does need to use
> some specific tool found in ${nacl_root}, then that tool's path should be
> set explicitly in a new variable (such as, nacl_whizbang_tool) rather than
> relying the search path defined by CMAKE_FIND_ROOT.

OK, I thought it a bit scary to mix host tools when cross compilation 
environment is in use as the environment can choose to override whatever 
tools it wants and I'm not sure what the search preference is (first 
host, then cross env?). But I can change this, it should work fine with 
nacl SDK.

> [1] https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/piglit/piglit-2014.07.23.ebuild
>
>> +# setup nacl includes and required libraries
>> +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)
>
>