[waffle] [PATCH] waffle: make gl_basic to work with nacl

[Chad Versace]

On 12/21/2014 08:06 AM, Emil Velikov wrote:
> On 17 December 2014 at 13:12, Tapani Pälli <tapani.palli at intel.com> wrote:
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>  .gitignore              |  2 ++
>>  examples/CMakeLists.txt | 30 ++++++++++++++++++++++++++++++
>>  examples/gl_basic.c     | 17 +++++++++++++++++
>>  examples/index.html     | 39 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 88 insertions(+)
>>  create mode 100644 examples/index.html
>>
>> diff --git a/.gitignore b/.gitignore
>> index 6981cd7..a1161e3 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -34,6 +34,8 @@ Makefile
>>
>>  /bin/gl_basic
>>  /bin/gl_basic_test
>> +/bin/gl_basic_nacl.nexe
>> +/bin/gl_basic_nacl.nmf
>>  /bin/simple-x11-egl
>>  /bin/wflinfo
>>  /doc/html/man/waffle.7.html
>> diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt
>> index 75e1c0b..cf75165 100644
>> --- a/examples/CMakeLists.txt
>> +++ b/examples/CMakeLists.txt
>> @@ -17,6 +17,36 @@ if((waffle_on_linux) AND (waffle_has_x11_egl))
>>  endif()
>>
>>  # ----------------------------------------------------------------------------
>> +# Target: gl_basic_nacl (executable + JSON manifest file)
>> +# ----------------------------------------------------------------------------
>> +if (waffle_has_nacl)
>> +    add_executable(gl_basic_nacl.nexe gl_basic.c)
>> +    include_directories(${nacl_INCLUDE_DIRS})
>> +
>> +    # this is done to get rid of -Werror=implicit-function-declaration
>> +    # which results in error when compiling against ppapi_simple
>> +    set(CMAKE_C_FLAGS "--std=c99")

> Hmm I was under the impression that the we've already have it set in
> the "top" cmake file ?
> I.e. within cmake/Modules/WaffleDefineCompilerFlags.cmake

Yes, but it looks like Tapani is using a quick hack to strip -Werror=implicit-function-declaration.
His hack is to clobber any previously set value for CMAKE_C_FLAGS.

Tapani, I think it's cleaner to change WaffleDefineCompilerFlags.cmake to
not add the problematic flag if waffle_has_nacl==true.

>> +
>> +    target_link_libraries(gl_basic_nacl.nexe ${waffle_libname} ${nacl_LDFLAGS} -lppapi_simple -lnacl_io -lppapi_gles2)
>> +
> Please split it into separate lines, and double check for overlinking.

Yes, please split.

> I would suspect that we don't need -lppapi_gles2, and maybe more.
> 
>> +    # create .nmf file that contains JSON manifest for the .nexe required by NaCl
>> +    add_custom_command(
>> +        OUTPUT gl_basic_nacl.nmf
>> +        COMMAND ${nacl_root}/tools/create_nmf.py -L${CMAKE_LIBRARY_OUTPUT_DIRECTORY} ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/gl_basic_nacl.nexe > ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/gl_basic_nacl.nmf
>> +        DEPENDS gl_basic_nacl.nexe
>> +        COMMENT "Create JSON manifest"
>> +        VERBATIM
>> +    )
>> +
>> +    add_custom_target(CreateJSONManifest ALL
>> +                      DEPENDS gl_basic_nacl.nmf)

Please follow the convention elsewhere in Waffle and use unix-hacker-style or
unix_hacker_style for target names, not CamelCase. Also, the  name should
be prefixed with "gl_basic".

>> +
>> +    # install index.html that loads gl_basic_nacl.nmf
>> +    file(INSTALL index.html DESTINATION ${CMAKE_RUNTIME_OUTPUT_DIRECTORY})
>> +
> If the following works, can we use it to be consistent with the rest of waffle ?
> 
> install(
>     FILES index.html
>     DESTINATION ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}
>     )
> 

The `file(INSTALL ...)` command is new to me, but if I understand the CMake
docs correctly, it behaves differently than the `install(FILES ...)` command.
The `install` command doesn't install the file until you run `make install`.
The `file(INSTALL ...)` command installs the file immediately (but, um,
I'm unsure exactly when "immediately" is).

If I'm reading the patch right, index.html needs to be in the correct location
during build time, not at install time.

NaCl is still really new to me. So I don't know what the best thing to do is
with the auxillary json and manifest files. But I am sure that putting a file
named 'index.html' into $WAFFLE_BUILD_DIR/bin is the wrong thing to do, because
that precludes the possibility of adding additional NaCl apps to the build
system due to filename conflicts.

>> +
>> +# ----------------------------------------------------------------------------
>>  # Target: gl_basic (executable)
>>  # ----------------------------------------------------------------------------
>>
>> diff --git a/examples/gl_basic.c b/examples/gl_basic.c
>> index fb62d52..371e423 100644
>> --- a/examples/gl_basic.c
>> +++ b/examples/gl_basic.c
>> @@ -508,8 +508,16 @@ removeXcodeArgs(int *argc, char **argv)
>>
>>  #endif // __APPLE__
>>
>> +#ifdef __native_client__
>> +#include "ppapi_simple/ps_main.h"
>> +int basic_test_main(int argc, char **argv);
>> +PPAPI_SIMPLE_REGISTER_MAIN(basic_test_main)
>> +int
>> +basic_test_main(int argc, char **argv)
>> +#else
>>  int
>>  main(int argc, char **argv)
>> +#endif

Bear with my NaCl-ignorant question.

Why do you rename main to basic_test_main? They have the same signature.
Is it because the NaCl runtime already defines a symbol named main?

>>  {
>>      bool ok;
>>      int i;
>> @@ -530,9 +538,18 @@ main(int argc, char **argv)
>>          cocoa_init();
>>      #endif
>>
>> +    #ifndef __native_client__
>>      ok = parse_args(argc, argv, &opts);
>>      if (!ok)
>>          exit(EXIT_FAILURE);
>> +    #else
>> +        // Fixed arguments for native client.
>> +        opts.context_api = WAFFLE_CONTEXT_OPENGL_ES2;
>> +        opts.platform = WAFFLE_PLATFORM_NACL;
>> +        opts.dl = WAFFLE_DL_OPENGL_ES2;
>> +        opts.context_profile = WAFFLE_NONE;
>> +        opts.context_version = -1;
>> +    #endif
>>
> Bikeshed:
> Can we have consistent use of __native_client__. I.e.
> 
> +    #ifdef __native_client__
> +        // Fixed arguments for native client.
> +        opts.context_api = WAFFLE_CONTEXT_OPENGL_ES2;
> +        opts.platform = WAFFLE_PLATFORM_NACL;
> +        opts.dl = WAFFLE_DL_OPENGL_ES2;
> +        opts.context_profile = WAFFLE_NONE;
> +        opts.context_version = -1;
> +    #else
>     ok = parse_args(argc, argv, &opts);
>      if (!ok)
>          exit(EXIT_FAILURE);
> +    #endif

In reply to your cover letter's question: I think #ifdefs are appropriate here.