Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake

[Timur Safin via Tarantool-patches <tarantool-patches@dev.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