From: Timur Safin 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: Tue, 9 Feb 2021 16:55:35 +0300 [thread overview] Message-ID: <05a601d6feeb$3ddc0f50$b9942df0$@tarantool.org> (raw) In-Reply-To: <20210208155638.GB5448@tarantool.org> LGTM, if you need a shorter answer, but there are some further comments below... : From: Igor Munkin <imun@tarantool.org> : Subject: Re: [PATCH luajit 2/5] build: replace GNU Make with CMake : : > : + : > : +# CMake generated artefacts : > : +CMakeCache.txt : > : +CMakeFiles : > : +Makefile : > : +cmake_install.cmake : > : +compile_commands.json : > : +install_manifest.txt : > : +luajit-parse-memprof : > : +luajit.pc : > : > Uh-oh, this ugly hack would be handled by single exclusion : > build*/ : > if we would all use the same (idiomatic) approach for out-of-source : > build. But we do not yet accustomed to that, so... never mind! : : I personally prefer mechanisms to policies: this approach allows one to : freely name so called <build> directory on his own e.g. <bindir>, : <tuta-lejit-sborka>, or even <qkrq>, that is my favourite one. : Furthermore this approach allows to ignore CMake output for both : out-of-source and in-source build. Such configuration is much more : flexible, IMHO, and leads to a neglible and well localized changes in : .gitignore. But, nevermind ;) Ok, most of those names are generic enough as generated by cmake, so it's a good idea to exclude them. ... : : > : + : > : +# Switch to harder (and slower) hash function when a collision : > : +# chain in the string hash table exceeds certain length. : > : +option(LUAJIT_SMART_STRINGS "Harder string hashing function" ON) : > : +if(LUAJIT_SMART_STRINGS) : > : + AppendFlags(TARGET_C_FLAGS -DLUAJIT_SMART_STRINGS=1) : > : +endif() : > : > The same note about lack of ENABLE prefix in the option name. : : Well, again: we do care about consistency. Unfortunately, this option is : provided only in our fork, so the inconsistency you're talking about was : messed up years ago. I propose to port the build system as is now with : no changes in the option names, but this is a nice point to refactor : them and make them consistent in scope of a separate issue. Thoughts? Ok, then leave them as is. This patchset supposed to be refactoring of what is already there. ... : > : > FWIW -msse2 implicitly assumes -msse but if that way it used to be : > in makefiles than so be it. Don't need to "improve" it. : : Yes, I tried to move everything making as few changes as possible. I believe : there are lots of places to be "modernized" later. Yup, we should leave it as is today. ... : : > : > : + : > : +include(SetTargetFlags) : > : +list(APPEND TARGET_LIBS m) : > : > ${TARGET_LIBS} below used as space separated list of words, it's probably : > bad idea to operate with it as list, because we end up in trace with : something : > like : > : > ./third_party/luajit/src/CMakeLists.txt(302): : target_include_directories(luajit_static PRIVATE : ./build/third_party/luajit/src ) : > ./third_party/luajit/src/CMakeLists.txt(305): : target_link_libraries(luajit_static libluajit_static dl;m ) : > : : Hm, I'm not quite sure what is broken by such usage, but everything is : OK when I run <make> with VERBOSE=1. Here is the related part: : | $ make VERBOSE=1 : | <...> : | [100%] Linking C executable luajit : | cd /tarantool-luajit/src && /usr/bin/cmake -E cmake_link_script : CMakeFiles/luajit_static.dir/link.txt --verbose=1 : | /usr/bin/cc -fomit-frame-pointer -fno-stack-protector -Wall -rdynamic - : Wl,-E CMakeFiles/luajit_static.dir/luajit.c.o -o luajit libluajit.a -ldl - : lm : | make[2]: Leaving directory '/tarantool-luajit' : | [100%] Built target luajit_static : | <...> Ok then, that's probably means that target_link_libraries() is expecting list at that place, and this works as expected. So, never mind. ... : > : > Here we have reasonable cmake complain: : > : > -- Configuring done : > WARNING: Target "luajit_static" has EXCLUDE_FROM_ALL set and will not be : built by default but : > an install rule has been provided for it. CMake does not define : behavior for this case. : > WARNING: Target "libluajit_static" has EXCLUDE_FROM_ALL set and will not : be built by default : > but an install rule has been provided for it. CMake does not define : behavior for this case. : > : : Well, I have no warning at all on my machine... It's cmake 3.13.4 here, may be warning is version dependent. : | $ cmake . : | -- The C compiler identification is GNU 8.3.0 : | -- Check for working C compiler: /usr/bin/cc : | -- Check for working C compiler: /usr/bin/cc -- works : | -- Detecting C compiler ABI info : | -- Detecting C compiler ABI info - done : | -- Detecting C compile features : | -- Detecting C compile features - done : | -- [SetVersion] Reading version from VCS: v2.1.0-beta3-79-ge616d62 : | -- The ASM compiler identification is GNU : | -- Found assembler: /usr/bin/cc : | -- Configuring done : | -- Generating done : | -- Build files have been written to: /tarantool-luajit : : > There is no much point to install ${LIBLUAJIT_STATIC_DEPS} or : ${LIBLUAJIT_SHARED_DEPS} if we have : > excluded luajit_static and/or libluajit_shared from target all : dependencies via EXCLUDE_FROM_ALL. : : ... however, I looked to the docs[2] and there is mentioned the fact : such behaviour is not defined and user is responsible to ensure whether : the corresponding artefacts are built prior to installing them. I : believed this machinery can be done on CMake side, but I forgot that : CMake is a crap. Wrapped all install rules with if/endif, squashed, : force-pushed to the branch. Diff is below: : : ============================================================================ : ==== : : diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt : index 5724c8b..51e97a6 100644 : --- a/src/CMakeLists.txt : +++ b/src/CMakeLists.txt : @@ -338,21 +338,35 @@ set(LUAJIT_BINARY $<TARGET_FILE:${LUAJIT_DEPS}> : PARENT_SCOPE) : add_custom_target(libluajit DEPENDS ${LIBLUAJIT_DEPS}) : add_custom_target(luajit ALL DEPENDS libluajit ${LUAJIT_DEPS}) : : -install(TARGETS ${LUAJIT_DEPS} : - RUNTIME : - DESTINATION bin : - COMPONENT luajit : -) : -install(TARGETS ${LIBLUAJIT_STATIC_DEPS} : - ARCHIVE : - DESTINATION lib : - COMPONENT luajit : -) : -install(TARGETS ${LIBLUAJIT_SHARED_DEPS} : - LIBRARY : - DESTINATION lib : - COMPONENT luajit : -) : +# Unfortunately, CMake provides no guarantees for install commands : +# used for the targets excluded from <all> and obligues user to : +# handle this manually on his side. Hence check whether the : +# targets used below are presented for the chosen build mode. : +# See more info in CMake docs below: : +# https://cmake.org/cmake/help/v3.1/prop_tgt/EXCLUDE_FROM_ALL.html : +if(TARGET ${LUAJIT_DEPS}) : + install(TARGETS ${LUAJIT_DEPS} : + RUNTIME : + DESTINATION bin : + COMPONENT luajit : + ) : +endif() : + : +if(TARGET ${LIBLUAJIT_STATIC_DEPS}) : + install(TARGETS ${LIBLUAJIT_STATIC_DEPS} : + ARCHIVE : + DESTINATION lib : + COMPONENT luajit : + ) : +endif() : + : +if(TARGET ${LIBLUAJIT_SHARED_DEPS}) : + install(TARGETS ${LIBLUAJIT_SHARED_DEPS} : + LIBRARY : + DESTINATION lib : + COMPONENT luajit : + ) : +endif() Noice! Best Regards, Timur
next prev parent reply other threads:[~2021-02-09 13:55 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 [this message] 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 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='05a601d6feeb$3ddc0f50$b9942df0$@tarantool.org' \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=tsafin@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