Tarantool development patches archive
 help / color / mirror / Atom feed
From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "'Igor Munkin'" <imun@tarantool.org>,
	"'Sergey Kaplun'" <skaplun@tarantool.org>
Cc: <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system
Date: Sat, 20 Feb 2021 10:42:10 +0300	[thread overview]
Message-ID: <0b6401d7075b$e61456f0$b23d04d0$@tarantool.org> (raw)
In-Reply-To: <20210219231009.GS5448@tarantool.org>

: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [PATCH 2/3] build: adjust LuaJIT build system
: 
: Sergey,
: 
: Thanks for your review!
: 
...
: 
: > > +macro(TestAndAppendFLag flags flag)
: >
: > Nit: Why macro not function? It is visible to all project after
: > including.
: > Feel free to ignore.
: 
: <flags> variable need to be expanded twice: the first time to
: interpolate the variable name and the second time to interpolate that
: variable value. Unfortunately, there is no way to implement it in a
: different way via CMake.

Yup, with function instead of macro it would be very verbose and
Unreadable. Macro is the simplest and cleanest way here for their
purpose.

: 
: Ignoring.
: 
: >
: > > +    string(REGEX REPLACE "-" "_" TESTFLAG ${flag})
: > > +    string(TOUPPER ${TESTFLAG} TESTFLAG)
: > > +    # XXX: can't use string(PREPEND ...) on ancient versions.
: > > +    set(TESTFLAG "CC_HAS${TESTFLAG}")
: > > +    if(${${TESTFLAG}})
: > > +        set(${flags} "${${flags}} ${flag}")
: 
: Here is the problem spot.
: 
: > >      endif()
: > >  endmacro()
: > >
: 
: <snipped>
: 
: 
: >
: > > +    "Generate <test> target" FORCE)
: > > +
: > > +# Enable internal LuaJIT assertions for Tarantool Debug build.
: > > +if(CMAKE_BUILD_TYPE STREQUAL "Debug")
: >
: > Side note: I get tons of warnings like:
: >
: > | [ 47%] Building C object
: third_party/luajit/src/CMakeFiles/core_static.dir/lib_string.c.o
: > | [ 47%] Building C object
: third_party/luajit/src/CMakeFiles/core_static.dir/lib_table.c.o
: > | /home/burii/reviews/tarantool/luajit-
: cmake/third_party/luajit/src/lj_strfmt.c: In function 'lj_strfmt_putfxint':
: > | /home/burii/reviews/tarantool/luajit-
: cmake/third_party/luajit/src/lj_strfmt.c:260:9: warning: variable 'ps' set
: but not used [-Wunused-but-set-variable]
: > |   260 |   char *ps;
: > |       |         ^~
: >
: > If build with `-DCMAKE_BUILD_TYPE=Debug` first, **do not** run
: > `rm CMakeCache.txt` and then build with
: > `-DCMAKE_BUILD_TYPE=RelWithDebInfo`. I am not familiar with CMake, is it
: > an expected behaviour?
: 
: I don't know. I guess there is something cached in CMakeCache.txt that
: leads to such warnings. Maybe Timur will clarify this behaviour.

Yes, toolchain settings (including compiler settings and build mode)
Are cached in CMakeCache.txt and not reassigned from command-line if
there is cached value. You have to delete toolchain cached values
to make all variables be redefined.

If you not delete CMAkeCache.txt you get shorter cmake cycle,
which might be not exactly what you hoped to get.

: 
: >
: 
: <snipped>
: 
: > > +add_dependencies(build_bundled_libs libluajit)
: >
: > Nit: Looks like it should be in the root <CMakeLists.txt>, not here.
: > Feel free to ignore.
: 
: Originally, it was here, so I would like to leave it here too.

The more local - the better. Agreed. 

: 
: Ignoring.
: 
: >
: 

Timur


  reply	other threads:[~2021-02-20  7:42 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust " Igor Munkin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 1/3] build: fix lua.c file generation Igor Munkin via Tarantool-patches
2021-02-15 13:12   ` Sergey Kaplun via Tarantool-patches
2021-02-19 21:17     ` Igor Munkin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system Igor Munkin via Tarantool-patches
2021-02-15 16:13   ` Sergey Kaplun via Tarantool-patches
2021-02-19 23:10     ` Igor Munkin via Tarantool-patches
2021-02-20  7:42       ` Timur Safin via Tarantool-patches [this message]
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI Igor Munkin via Tarantool-patches
2021-02-15 16:29   ` Sergey Kaplun via Tarantool-patches
2021-02-19 21:29     ` Igor Munkin via Tarantool-patches
     [not found]       ` <0b6601d7075c$64329060$2c97b120$@tarantool.org>
2021-02-20 10:18         ` Igor Munkin via Tarantool-patches
2021-02-24  7:16   ` Timur Safin via Tarantool-patches
2021-02-25 22:08     ` Igor Munkin via Tarantool-patches
2021-02-26 19:04       ` Timur Safin via Tarantool-patches
2021-02-26 19:09 ` [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Timur Safin via Tarantool-patches
2021-02-26 21:06   ` Igor Munkin via Tarantool-patches
2021-02-27 13:56 ` Sergey Kaplun via Tarantool-patches
2021-02-28 22:05 ` 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='0b6401d7075b$e61456f0$b23d04d0$@tarantool.org' \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system' \
    /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