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 luajit 2/5] build: replace GNU Make with CMake
Date: Fri, 5 Feb 2021 01:53:53 +0300	[thread overview]
Message-ID: <11be01d6fb48$9d0e1050$d72a30f0$@tarantool.org> (raw)
In-Reply-To: <7cd42be38d86c832ecd4ba0f3edd7ae83aead7ad.1612291495.git.imun@tarantool.org>

Few preliminary observations below...

: From: Igor Munkin <imun@tarantool.org>
: Subject: [PATCH luajit 2/5] build: replace GNU Make with CMake
: 
...

: 
: diff --git a/.gitignore b/.gitignore
: index 1a07bf7..a21ee1c 100644
: --- a/.gitignore
: +++ b/.gitignore
: @@ -1,5 +1,5 @@
:  *.[oa]
: -*.so
: +*.so*
:  *.obj
:  *.lib
:  *.exp
: @@ -9,3 +9,13 @@
:  *.dmp
:  *.swp
:  .tags
: +
: +# 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!

: diff --git a/CMakeLists.txt b/CMakeLists.txt
: new file mode 100644
: index 0000000..0dba5d8
: --- /dev/null
: +++ b/CMakeLists.txt
: @@ -0,0 +1,261 @@
: +# LuaJIT -- interpreter and JIT compiler for Lua language.
: +# This is the main entry point for building, testing and
: +# packaging the project.
: +# Major portions taken verbatim or adapted from the uJIT.
: +# Copyright (C) 2015-2019 IPONWEB Ltd.

2019, IPONWEB?

...
: +# --- Fine-tuning cmake environment ---------------------------------------
: -----
: +
: +set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
: +set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

Could you please remove this enforcement of CMAKE_EXPORT_COMPILE_COMMANDS here?
Agreed that this is always a good idea to generate compile_commands.json. But 
disagreed that we should enforce it automatically. This is developer choice to 
either generate or not this compile database. (And it's slowing down a bit cmake
generation phase on big projects. Which is not yet case here, but in any case)

...
: +
: +# Features from Lua 5.2 that are unlikely to break existing code
: +# are enabled by default. Some other features that *might* break
: +# some existing code (e.g. __pairs or os.execute() return values)
: +# can be enabled here.
: +# XXX: this does not provide full compatibility with Lua 5.2 at
: +# this time.
: +option(LUAJIT_LUA52COMPAT "Compatibility with Lua 5.2" OFF)
: +if(LUAJIT_LUA52COMPAT)
: +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_LUA52COMPAT)
: +endif()

I do not know whether we do care that much about consistency of an option names
or not care? But worth to mention that all other options do have ENABLE/DISABLE
or HAS prefix in similar contexts, but here we do not use it that way, and it's
simple LUAJIT_LUA52COMPAT. What about LUAJIT_ENABLE_LUA52COMPAT as it's 
passed to compile options? (Not insist, but worth to note)


: +
: +# Disable the JIT compiler, i.e. turn LuaJIT into a pure
: +# interpreter.
: +option(LUAJIT_DISABLE_JIT "JIT support" OFF)
: +if(LUAJIT_DISABLE_JIT)
: +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_DISABLE_JIT)
: +endif()
: +set(LUAJIT_HAS_JIT NOT LUAJIT_DISABLE_JIT)
: +

...

: +# Enable GC64 mode for x64.
: +option(LUAJIT_ENABLE_GC64 "GC64 mode for x64" OFF)
: +if(LUAJIT_ENABLE_GC64)
: +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_GC64)
: +endif()
: +
: +# Disable memory profiler.
: +option(LUAJIT_DISABLE_MEMPROF "LuaJIT memory profiler support" OFF)
: +if(LUAJIT_DISABLE_MEMPROF)
: +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_DISABLE_MEMPROF)
: +endif()
: +
: +# 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.

: +
: +# Turn on assertions for the whole LuaJIT VM. This significantly
: +# slows down everything. Use only if you suspect a problem with
: +# LuaJIT itself.
: +option(LUAJIT_USE_ASSERT "Assertions for the whole LuaJIT VM" OFF)
: +if(LUAJIT_USE_ASSERT)
: +  AppendFlags(TARGET_C_FLAGS -DLUA_USE_ASSERT)
: +endif()
: +


: --- /dev/null
: +++ b/cmake/LuaJITUtils.cmake
: @@ -0,0 +1,31 @@
...
: diff --git a/cmake/MakeSourceList.cmake b/cmake/MakeSourceList.cmake
: new file mode 100644
: index 0000000..fa455bb
: --- /dev/null
: +++ b/cmake/MakeSourceList.cmake
: @@ -0,0 +1,47 @@
: +# Major portions taken verbatim or adapted from the uJIT.
: +# Copyright (C) 2015-2019 IPONWEB Ltd.
: +#
: +# make_source_list provides a convenient way to define a list of sources
: +# and get a list of absolute paths.
: +#
: +# Example usage:
: +#
: +#   make_source_list(SOURCES_CORE
: +#     SOURCES
: +#       main.c
: +#       test.c
: +#       subdir/test2.c
: +#   )
: +#
: +# This will give you the list:
: +#    "<...>/main.c;<...>/test.c;<...>/subdir/test2.c"
: +# (where `<...>` is ${CMAKE_CURRENT_SOURCE_DIR}).
: +#
: +# Absolute paths in `SOURCES` list don't get ${CMAKE_CURRENT_SOURCE_DIR}
: +# prepended to them.
: +

Very convenient macro below! Much respect to author!

: +function(make_source_list list)
: +  set(prefix ARG)
: +  set(noValues)
: +  set(singleValues)
: +  set(multiValues SOURCES)
: +
: +  include(CMakeParseArguments) # if we update to CMake >= 3.5, can remove
: this line
: +  cmake_parse_arguments(${prefix}
: +                        "${noValues}"
: +                        "${singleValues}"
: +                        "${multiValues}"
: +                        ${ARGN})
: +
: +  set(result_list "")
: +
: +  foreach(fn ${ARG_SOURCES})
: +    if (IS_ABSOLUTE ${fn})
: +      list(APPEND result_list "${fn}")
: +    else()
: +      list(APPEND result_list "${CMAKE_CURRENT_SOURCE_DIR}/${fn}")
: +    endif()
: +  endforeach()
: +
: +  set(${list} "${result_list}" PARENT_SCOPE)
: +endfunction()
...

: diff --git a/cmake/SetTargetFlags.cmake b/cmake/SetTargetFlags.cmake
: new file mode 100644
: index 0000000..260fc6b
: --- /dev/null
: +++ b/cmake/SetTargetFlags.cmake
: @@ -0,0 +1,42 @@
...
: +
: +LuaJITTestArch(TESTARCH "${TARGET_C_FLAGS}")
: +LuaJITArch(LUAJIT_ARCH "${TESTARCH}")
: +
: +# Target-specific compiler options.
: +#
: +# x86/x64 only: For GCC 4.2 or higher and if you don't intend to
: +# distribute the binaries to a different machine you could also
: +# use: -march=native.
: +if(LUAJIT_ARCH STREQUAL "x86")
: +  AppendFlags(TARGET_C_FLAGS -march=i686 -msse -msse2 -mfpmath=sse)
: +endif()

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.
...


: diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
: new file mode 100644
: index 0000000..8ada1a4
: --- /dev/null
: +++ b/src/CMakeLists.txt
: @@ -0,0 +1,391 @@
: +# Building LuaJIT core: bootstrapping, VM, runtime, JIT compiler.
: +# Major portions taken verbatim or adapted from the uJIT.
: +# Copyright (C) 2015-2019 IPONWEB Ltd.

2019 IPONWEB?

: +
: +cmake_minimum_required(VERSION 2.8.12 FATAL_ERROR)
: +
...

: +
: +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 )


...

: +# Compiling and linking CLIs.
: +
: +add_executable(luajit_static EXCLUDE_FROM_ALL ${CLI_SOURCES})
: +set_target_properties(luajit_static PROPERTIES
: +  OUTPUT_NAME "${LUAJIT_CLI_NAME}"
: +  COMPILE_FLAGS "${TARGET_C_FLAGS}"
: +  LINK_FLAGS "${TARGET_BIN_FLAGS}"
: +  RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
: +)
: +target_include_directories(luajit_static PRIVATE
: +  ${CMAKE_CURRENT_BINARY_DIR}
: +)
: +target_link_libraries(luajit_static libluajit_static ${TARGET_LIBS})
: +
: +add_executable(luajit_shared EXCLUDE_FROM_ALL ${CLI_SOURCES})
: +set_target_properties(luajit_shared PROPERTIES
: +  OUTPUT_NAME "${LUAJIT_CLI_NAME}"
: +  COMPILE_FLAGS "${TARGET_C_FLAGS}"
: +  LINK_FLAGS "${TARGET_BIN_FLAGS}"
: +  RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
: +)
: +target_include_directories(luajit_shared PRIVATE
: +  ${CMAKE_CURRENT_BINARY_DIR}
: +)
: +target_link_libraries(luajit_shared libluajit_shared ${TARGET_LIBS})
: +
: +if(NOT BUILDMODE STREQUAL "dynamic")
: +  set(LIBLUAJIT_STATIC_DEPS libluajit_static)
: +endif()
: +if(NOT BUILDMODE STREQUAL "static")
: +  set(LIBLUAJIT_SHARED_DEPS libluajit_shared)
: +endif()
: +set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})
: +
: +if(BUILDMODE STREQUAL "dynamic")
: +  set(LUAJIT_DEPS luajit_shared)
: +else()
: +  set(LUAJIT_DEPS luajit_static)
: +endif()
: +
: +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
: +)

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.

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.

: +install(TARGETS ${LIBLUAJIT_STATIC_DEPS}
: +  ARCHIVE
: +  DESTINATION lib
: +  COMPONENT luajit
: +)
: +install(TARGETS ${LIBLUAJIT_SHARED_DEPS}
: +  LIBRARY
: +  DESTINATION lib
: +  COMPONENT luajit
: +)
: +


: diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
: new file mode 100644
: index 0000000..f9ffc5e
: --- /dev/null
: +++ b/tools/CMakeLists.txt
: @@ -0,0 +1,77 @@
: +# Building tools for developing with uJIT.
: +# Major portions taken verbatim or adapted from the uJIT.
: +# Copyright (C) 2015-2019 IPONWEB Ltd.

2019 IPONWEB?

: +
: +# See the rationale in the root CMakeLists.txt
: +cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
: +
: +set(LUAJIT_TOOLS_DEPS)
: +
: +if(LUAJIT_DISABLE_MEMPROF)
: +  message(STATUS "LuaJIT memory profiler support is disabled")
: +else()
: +  # XXX: Can use genex here since the value need to be evaluated
: +  # on the configuration phase. Fortunately, we know the exact
: +  # path where LuaJIT binary is located.
: +  set(LUAJIT_TOOLS_BIN ${LUAJIT_BINARY_DIR}/${LUAJIT_CLI_NAME})
: +  set(LUAJIT_TOOLS_DIR ${CMAKE_CURRENT_SOURCE_DIR})
: +  configure_file(luajit-parse-memprof.in luajit-parse-memprof @ONLY
: ESCAPE_QUOTES)
: +
: +  add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS
: +    luajit-parse-memprof
: +    memprof/humanize.lua
: +    memprof/parse.lua
: +    memprof.lua
: +    utils/bufread.lua
: +    utils/symtab.lua
: +  )
: +  list(APPEND LUAJIT_TOOLS_DEPS tools-parse-memprof)
: +
: +  install(FILES
: +      ${CMAKE_CURRENT_SOURCE_DIR}/memprof/humanize.lua
: +      ${CMAKE_CURRENT_SOURCE_DIR}/memprof/parse.lua
: +    DESTINATION ${LUAJIT_DATAROOTDIR}/memprof
: +    PERMISSIONS
: +      OWNER_READ OWNER_WRITE
: +      GROUP_READ
: +      WORLD_READ
: +    COMPONENT tools-parse-memprof
: +  )
: +  install(FILES
: +      ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua
: +      ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua
: +    DESTINATION ${LUAJIT_DATAROOTDIR}/utils
: +    PERMISSIONS
: +      OWNER_READ OWNER_WRITE
: +      GROUP_READ
: +      WORLD_READ
: +    COMPONENT tools-parse-memprof
: +  )
: +  install(FILES
: +      ${CMAKE_CURRENT_SOURCE_DIR}/memprof.lua
: +    DESTINATION ${LUAJIT_DATAROOTDIR}
: +    PERMISSIONS
: +      OWNER_READ OWNER_WRITE
: +      GROUP_READ
: +      WORLD_READ
: +    COMPONENT tools-parse-memprof
: +  )
: +  install(CODE
: +    # XXX: Since the auxiliary script need to be configured in
: +    # other way it need to be reconfigured it prior to its
: +    # installation. Unfortunately, we need to manually specify
: +    # the installation path in <configure_file> command.
: +    # Hope this script will be gone as a result of the issue below
: +    # https://github.com/tarantool/tarantool/issues/5688.
: +    "
: +      set(LUAJIT_TOOLS_BIN ${CMAKE_INSTALL_PREFIX}/bin/${LUAJIT_CLI_NAME})
: +      set(LUAJIT_TOOLS_DIR ${CMAKE_INSTALL_PREFIX}/${LUAJIT_DATAROOTDIR})
: +      configure_file(${CMAKE_CURRENT_SOURCE_DIR}/luajit-parse-memprof.in
: +        ${CMAKE_INSTALL_PREFIX}/bin/luajit-parse-memprof @ONLY
: ESCAPE_QUOTES)
: +      message(STATUS \"Installing: ${CMAKE_INSTALL_PREFIX}/bin/luajit-
: parse-memprof\")
: +    "
: +    COMPONENT tools-parse-memprof
: +  )

Шайтан!

: +endif()
: +
: +add_custom_target(LuaJIT-tools DEPENDS ${LUAJIT_TOOLS_DEPS})
: --
: 2.25.0

Regards,
Timur


  reply	other threads:[~2021-02-04 22:54 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 [this message]
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
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>
2021-02-08 15:57     ` Igor Munkin via Tarantool-patches
     [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-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='11be01d6fb48$9d0e1050$d72a30f0$@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 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