Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for such precise review! I've answered the questions below and
re-implemented the fix (the new patch is at the end), so hold your LGTM
for a while.

On 11.05.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> LGTM, except ignorable questions below.
> 
> On 11.05.21, Igor Munkin wrote:
> > There were issues with configuring LuaJIT on Apple machines, since
> > <LuaJITTestArch> CMake auxiliary routine fails to locate system headers
> > (e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
> > platform detection fails and LuaJIT configuration ends with the fatal
> > error. This patch adds the necessary flags to help the routine to find
> > the required system headers.
> > 
> > Relates to tarantool/tarantool#5629
> > Needed for tarantool/tarantool#5983
> > Follows up tarantool/tarantool#4862
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> >  CMakeLists.txt          | 9 +++++++++
> >  cmake/LuaJITUtils.cmake | 7 ++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 5348e043..110a989f 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -114,6 +114,15 @@ if(LUAJIT_ENABLE_WARNINGS)
> >    )
> >  endif()
> >  
> > +# Set sysroot settings on OSX to find SDK with the system headers.
> > +# XXX: Obviously, there is no need in this setup if everything is
> > +# already set via CMAKE_C_FLAGS or in parent project build system.
> 
> Typo: s/parent project/a parent project/

Dropped the comment in the new patch. Ignoring.

> 
> > +if(CMAKE_OSX_SYSROOT AND CMAKE_C_SYSROOT_FLAG AND
> 
> I found nothing about CMAKE_C_SYSROOT_FLAG by looking in CMake variable list
> 
> | $ cmake --help-variable-list | grep SYSROOT
> | CMAKE_OSX_SYSROOT
> | CMAKE_SYSROOT
> | CMAKE_SYSROOT_COMPILE
> | CMAKE_SYSROOT_LINK
> 
> or documentation[1][2]. Looks like it is an internal CMake variable and
> it can be silently renamed in future versions. In my opinion, it is
> better to use -isysroot directly here, or at last drop a comment about it.
> Also, this variable is set by project() (and I suppose enable_language()
> too) call, and this fact should be mentioned (to avoid using it
> and LuaJITTestArch() before project() is called).

Nice catch, thanks! I didn't look into the docs, just took this part
from Tarantool CMake. Changed to -isysroot.

> 
> Also you may take a look to Mike's solution here[3].

Well, we're using CMake to avoid this complexity. Anyway, you can run
CMake with --trace-expand to see that CMake uses xcrun underneath.

> 
> Thoughts?
> 
> Side note: Also I've seen that Mike uses OSX_DEPLOYMENT_TARGET variable.
> Moreover, CMAKE_OSX_SYSROOT should "computed based on the
> CMAKE_OSX_DEPLOYMENT_TARGET or the host platform" according to [4].
> And by itself, it means the minimum possible IOS version, where an
> application may run [5]. May be we should fix this too with a separate
> issue and patch (to protect users from building LuaJIT on ancient
> devices)?

Sorry, I don't get this one. What exactly is bothering you here?

> 
> > +  NOT "${CMAKE_C_FLAGS}" MATCHES "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}"
> 
> Why must it match exactly the same default SYSROOT? Why user can't
> define something custom if he wants?

CMake tries to locate MacOS SDK by itself. If user specify something
it's either the same or something additional. There is nothing critical
to add the one found by CMake, IMHO. Anyway, this nit is irrelevant,
considering the changes below.

> 
> Feel free to ignore.

Ignoring.

> 
> > +)
> > +  AppendFlags(CMAKE_C_FLAGS "${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}")
> > +endif()
> 
> Also, as far as this misbehaviour is occuring only for LuaJITTestArch()
> macro we may avoid tweaking of global CMAKE_C_FLAGS, but just append it
> to TESTARCH_C_FLAGS if it is necessary.

Unfortunately, CMake is a crap. At first, these flags are required at
the configuration phase, but everything works fine at the build phase.
As a result of investigation, I've found that CMake emits[1] these flags
(both -arch and -isysroot) to a separate flags.make file located deep
into CMakeFiles/ directory. Moreover, they are not available via CMake
variables (used at configuration phase). They are just <fprintf>-ed to
the freaking file being included in build.make. Nice work, dudes! If
they wanted the most foolproof solution, they made it!

However, thanks to CMake authors for their tests: I've taken the recipe
there[2], so you can consider this as "Официальная позиция CMake".
Again, the changes are below.

> 
> Feel free to ignore.
> 
> > +
> >  # Auxiliary flags for main targets (libraries, binaries).
> >  AppendFlags(TARGET_C_FLAGS
> >    -D_FILE_OFFSET_BITS=64
> > diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
> > index d9f8b12a..8ff26a6a 100644
> > --- a/cmake/LuaJITUtils.cmake
> > +++ b/cmake/LuaJITUtils.cmake
> > @@ -1,8 +1,13 @@
> >  function(LuaJITTestArch outvar strflags)
> > +  # XXX: Compiler flags are also required in this routine. It can
> > +  # use e.g. external headers, which location is specified either
> > +  # implicitly (within CMake machinery) or explicitly (manually by
> > +  # configuration options).
> > +  set(TESTARCH_C_FLAGS "${CMAKE_C_FLAGS} ${strflags}")
> >    # XXX: <execute_process> simply splits the COMMAND argument by
> >    # spaces with no further parsing. At the same time GCC is bad in
> >    # argument handling, so let's help it a bit.
> > -  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${strflags})
> > +  separate_arguments(TESTARCH_C_FLAGS UNIX_COMMAND ${TESTARCH_C_FLAGS})
> >    # TODO: It would be nice to drop a few words, why do we use this
> >    # approach instead of CMAKE_HOST_SYSTEM_PROCESSOR variable.
> >    execute_process(
> > -- 
> > 2.25.0
> > 
> 
> [1]: https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html
> [2]: https://cmake.org/cmake/help/v3.1/manual/cmake-variables.7.html
> [3]: https://github.com/LuaJIT/LuaJIT/blob/8dc3cd6c84dfc81539604340b7884408e1c71d55/doc/install.html#L437
> [4]: https://cmake.org/cmake/help/v3.1/variable/CMAKE_OSX_SYSROOT.html
> [5]: https://developer.apple.com/library/archive/documentation/ToolsLanguages/Conceptual/Xcode_Overview/WorkingwithTargets.html
> 
> -- 
> Best regards,
> Sergey Kaplun

================================================================================

build: pass sysroot to MacOS SDK

There were issues with configuring LuaJIT on Apple machines, since
<LuaJITTestArch> CMake auxiliary routine fails to locate system headers
(e.g. assert.h in case when LUA_USE_ASSERT is enabled). As a result
platform detection fails and LuaJIT configuration ends with the fatal
error. This patch adds the necessary flags to help the routine to find
the required system headers.

Needed for tarantool/tarantool#6065
Relates to tarantool/tarantool#5629
Follows up tarantool/tarantool#4862

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 cmake/LuaJITUtils.cmake | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/cmake/LuaJITUtils.cmake b/cmake/LuaJITUtils.cmake
index d9f8b12a..3497edc4 100644
--- a/cmake/LuaJITUtils.cmake
+++ b/cmake/LuaJITUtils.cmake
@@ -1,4 +1,12 @@
 function(LuaJITTestArch outvar strflags)
+  # XXX: This routine uses external headers (e.g. system ones),
+  # which location is specified either implicitly (within CMake
+  # machinery) or explicitly (manually by configuration options).
+  # Need -isysroot flag on recentish MacOS after command line
+  # tools no longer provide headers in /usr/include.
+  if(CMAKE_OSX_SYSROOT)
+    set(strflags "${strflags} -isysroot ${CMAKE_OSX_SYSROOT}")
+  endif()
   # XXX: <execute_process> simply splits the COMMAND argument by
   # spaces with no further parsing. At the same time GCC is bad in
   # argument handling, so let's help it a bit.

================================================================================

[1]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmLocalGenerator.cxx#L1911-1916
[2]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Tests/FindPackageModeMakefileTest/CMakeLists.txt#L24-28


-- 
Best regards,
IM