Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake
Date: Sat, 27 Feb 2021 13:48:07 +0300	[thread overview]
Message-ID: <20210227104807.GB6842@root> (raw)
In-Reply-To: <20210220191835.GB9042@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

  reply	other threads:[~2021-02-27 10:48 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-02 20:57 [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 1/5] build: preserve the original build system Igor Munkin via Tarantool-patches
2021-02-04 22:53   ` Timur Safin via Tarantool-patches
2021-02-08 15:56     ` Igor Munkin via Tarantool-patches
2021-02-09 11:38   ` Sergey Kaplun via Tarantool-patches
2021-02-09 12:47     ` Igor Munkin via Tarantool-patches
2021-02-09 14:45       ` Sergey Kaplun via Tarantool-patches
2021-02-09 15:28         ` Igor Munkin via Tarantool-patches
2021-02-10  9:35           ` Sergey Kaplun via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake Igor Munkin via Tarantool-patches
2021-02-04 22:53   ` Timur Safin via Tarantool-patches
2021-02-08 15:56     ` Igor Munkin via Tarantool-patches
2021-02-09 13:55       ` Timur Safin via Tarantool-patches
2021-02-09 15:09         ` Igor Munkin via Tarantool-patches
2021-02-11 19:23   ` Sergey Kaplun via Tarantool-patches
2021-02-16 15:28     ` Igor Munkin via Tarantool-patches
2021-02-18  9:56       ` Sergey Kaplun via Tarantool-patches
2021-02-20 19:18         ` Igor Munkin via Tarantool-patches
2021-02-27 10:48           ` Sergey Kaplun via Tarantool-patches [this message]
2021-02-28 18:18             ` Igor Munkin via Tarantool-patches
2021-02-13  3:47   ` Sergey Kaplun via Tarantool-patches
2021-02-16 15:32     ` Igor Munkin via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 3/5] test: run LuaJIT tests via CMake Igor Munkin via Tarantool-patches
2021-02-08 15:05   ` Timur Safin via Tarantool-patches
2021-02-08 16:29     ` Igor Munkin via Tarantool-patches
2021-02-09  8:16       ` Timur Safin via Tarantool-patches
2021-02-09  8:43         ` Igor Munkin via Tarantool-patches
2021-02-09 13:59           ` Timur Safin via Tarantool-patches
2021-02-09 15:10             ` Igor Munkin via Tarantool-patches
2021-02-14 18:48   ` Sergey Kaplun via Tarantool-patches
2021-02-19 19:04     ` Igor Munkin via Tarantool-patches
2021-02-27 13:50       ` Sergey Kaplun via Tarantool-patches
2021-02-28 18:18         ` Igor Munkin via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 4/5] test: fix warnings found with luacheck in misclib* Igor Munkin via Tarantool-patches
     [not found]   ` <012f01d6fe1a$a2aa6890$e7ff39b0$@tarantool.org>
     [not found]     ` <2c495492-50f4-acfd-ad66-2cb44abb5fa1@tarantool.org>
2021-02-08 15:40       ` Sergey Bronnikov via Tarantool-patches
2021-02-08 15:58       ` Igor Munkin via Tarantool-patches
2021-02-08 15:57     ` Igor Munkin via Tarantool-patches
2021-02-14 19:16   ` Sergey Kaplun via Tarantool-patches
2021-02-16 15:29     ` Igor Munkin via Tarantool-patches
2021-02-16 16:36       ` Sergey Kaplun via Tarantool-patches
2021-02-02 20:57 ` [Tarantool-patches] [PATCH luajit 5/5] test: run luacheck static analysis via CMake Igor Munkin via Tarantool-patches
2021-02-04 22:52   ` Timur Safin via Tarantool-patches
2021-02-08 15:57     ` Igor Munkin via Tarantool-patches
2021-02-14 19:32   ` Sergey Kaplun via Tarantool-patches
2021-02-19 19:14     ` Igor Munkin via Tarantool-patches
2021-02-28 22:04 ` [Tarantool-patches] [PATCH luajit 0/5] Self-sufficient LuaJIT testing environment Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210227104807.GB6842@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox