Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Igor,

LGTM, except a few nitpicks regarding to the commit message.

On 20.02.21, Igor Munkin wrote:
> Sergey,
> 

<snipped>

> > > > > +if(CMAKE_LIBRARY_ARCHITECTURE)
> > > > > +  AppendFlags(TARGET_C_FLAGS -DLUA_MULTILIB='"lib/${CMAKE_LIBRARY_ARCHITECTURE}"')
> > > > > +endif()
> > > > 
> > > > What about `LUA_LMULTILIB`?
> > > > 
> > > > Side note: should we provide `DESTDIR` or/and `MULTILIB` control variable
> > > > like it does in Makefile.original?
> > > 
> > > DESTDIR is supported out of the box in CMake. MULTILIB is "autodetected"
> > > via CMake[1] (I hope, but never tried). Regardging LMULTILIB, I have no
> > > idea what its purpose is. Do you?
> > 
> > To configure default CPATH, IINM, see luaconf.h for details.
> 
> My fault, ask you in a different way: how do you suppose to use it?

For example, if I want to separate directories with Lua libraries and C
libraries. May be it is the superfluous customization. If you think so,
please add the notice that this option is not implemented in CMake, and
users should use original Makefile.

> 

<snipped>

> 
> > > > > diff --git a/cmake/SetTargetFlags.cmake b/cmake/SetTargetFlags.cmake
> > > > > new file mode 100644
> > > > > index 0000000..260fc6b
> > > > > --- /dev/null
> > > > > +++ b/cmake/SetTargetFlags.cmake
> > > 
> > 
> > I add some comments to the following chunk.
> > 
> > | +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> > | +  if(LUAJIT_ARCH STREQUAL "x64")
> > 
> > Side note: I have a linking error with the original Makefile
> > without `MACOSX_DEPLOYMENT_TARGET` specification:
> > || LINK      luajit
> > || Undefined symbols for architecture x86_64:
> > ||   "__Unwind_DeleteException", referenced from:
> > ||       _lj_err_unwind_dwarf in libluajit.a(lj_err.o)
> > ||   "__Unwind_GetCFA", referenced from:
> > ||       _lj_err_unwind_dwarf in libluajit.a(lj_err.o)
> > ||   "__Unwind_RaiseException", referenced from:
> > ||       _lj_err_throw in libluajit.a(lj_err.o)
> > ||   "__Unwind_SetGR", referenced from:
> > ||       _lj_err_unwind_dwarf in libluajit.a(lj_err.o)
> > ||   "__Unwind_SetIP", referenced from:
> > ||       _lj_err_unwind_dwarf in libluajit.a(lj_err.o)
> > || ld: symbol(s) not found for architecture x86_64
> > 
> > But nothing with CMake...
> > Nonetheless, I suggest to safe the old Makefile behaviour to specify
> > the minimum OS version (see [1]):
> 
> There is no error in CMake. This behaviour is dropped in LuaJIT in scope
> of this patch[1]. Why do we need to add this to CMake?

I think, because it may influence configuration of the toolchain and
flags. Let's return to this later if it really will become a problem.

> 
> > 
> 
> <snipped>
> 
> > Feel free to ignore.
> 
> Ignoring.
> 
> > 

<snipped>

> 
> > 
> > | +    AppendFlags(TARGET_SHARED_FLAGS -image_base 7fff04c4a000)
> > 
> > Also I've found that this line produces warnings from linker like:
> > 
> > || ld: warning: -seg1addr not 16384 byte aligned, rounding up
> > 
> > So, I suggest the following solution for cmake and for the original
> > Makefile (just add 8Kb):
> 
> There is the same value in the build system of the vanilla LuaJIT[2], so
> I leave everything intact. BTW, I see no warning on my Mac.

This is why I mentioned Darwin version and add corresponding patch for
the old Makefile too. Anyway, the linker can handle this by itself, so
lets drop this as is. We can return to this with a new issue (inside our
or LuaJIT repo).

> 

<snipped>

> > 
> > > 
> > > > 
> > > > > -- 
> > > > > 2.25.0
> > > > > 
> > > > 
> > > > [1]: https://stackoverflow.com/questions/41773161/negate-boolean-variable-in-cmake
> > > > [2]: https://cmake.org/pipermail/cmake/2015-July/061116.html
> > > > [3]: https://cmake.org/cmake/help/latest/command/continue.html
> > > > [4]: https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#can-i-do-make-uninstall-with-cmake
> > > > 
> > > > -- 
> > > > Best regards,
> > > > Sergey Kaplun
> > > 
> > > [1]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_LIBRARY_ARCHITECTURE.html
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > [1]: https://cmake.org/cmake/help/v3.14/envvar/MACOSX_DEPLOYMENT_TARGET.html
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> [1]: https://github.com/LuaJIT/LuaJIT/commit/8961a92
> [2]: https://github.com/LuaJIT/LuaJIT/blob/v2.1/src/Makefile#L327
> [3]: https://cmake.org/cmake/help/latest/prop_tgt/VERSION.html#mach-o-versions
> [4]: https://cmake.org/cmake/help/latest/prop_tgt/SOVERSION.html#mach-o-versions
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun