Re: [Tarantool-patches] [PATCH luajit 2/8] test: introduce MakeLuaPath.cmake helper

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for you review!

On 15.08.22, Sergey Bronnikov wrote:
> Igor, thanks for the patch! See my comments inline.
> 
> On 11.08.2022 14:17, Igor Munkin wrote:
> > While extending test suites it is often required to append additional
> > path where Lua or Lua-C auxiliary modules are located to LUA_PATH or
> > LUA_CPATH environment variables. Due to insane semicolon interpolation
> > in CMake strings (that converts such string to a list as a result), we
> > need to escape semicolon in LUA_PATH/LUA_CPATH strings while building
> > the resulting value.
> >
> > After the years of struggling MakeLuaPath.cmake module is introduced to
> > make LUA_PATH and LUA_CPATH definition convenient with <make_lua_path>
> > helper. This function takes all paths given as a variable list argument,
> > joins them in a reverse order by a semicolon and yields the resulting
> > string to a specified CMake variable.
> >
> > Signed-off-by: Igor Munkin<imun@tarantool.org>
> > ---
> >   cmake/MakeLuaPath.cmake                   | 46 +++++++++++++++++++++++
> >   test/CMakeLists.txt                       |  2 +
> >   test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt |  8 +++-
> >   test/lua-Harness-tests/CMakeLists.txt     | 16 +++++---
> >   test/tarantool-tests/CMakeLists.txt       | 28 ++++++++------
> >   5 files changed, 81 insertions(+), 19 deletions(-)
> >   create mode 100644 cmake/MakeLuaPath.cmake
> >
> > diff --git a/cmake/MakeLuaPath.cmake b/cmake/MakeLuaPath.cmake
> > new file mode 100644
> > index 00000000..9a5a3bb8
> > --- /dev/null
> > +++ b/cmake/MakeLuaPath.cmake

<snipped>

> > +
> > +  # FIXME: if we update to CMake >= 3.5, can remove this line.
> 
> I would replace comment with condition that will check CMake version
> 
> and will fail when CMake >= 3.5:
> 
> |if (CMAKE_VERSION VERSION_GREATER_EQUAL 35) |
> 
>     message(FATAL_ERROR "Please remove snippet for CMake < 3.5")
> 
> endif()

I doubt this is a working solution. The comment means, that there is no
need to include a separate module if CMake to be used is greater than
3.5. With your code, fatal error is raised in that case. I have CMake
3.20 on my local machine and LuaJIT fails to be built with your check
added (I've fixed the typo in the version number).

Ignoring for now.

> 
> 
> > +  include(CMakeParseArguments)

<snipped>

-- 
Best regards,
IM