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
next prev parent 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