From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B07B56EC5B; Thu, 13 May 2021 01:16:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B07B56EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1620857809; bh=0n2MPVVcR3w+fyMVeucfpccjVTwq5df3seUMmMvSm0s=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=S+6+blTBWdi5f9IhcXUp0w0obu4XX/zlJfW4WkJNP+06fmCPiTDPLI4ZqRA2YSR9z 983N2NiNGAHlb2fvrq1Es6/cRVrLS76OnlDKDIqp8k50kRaZMEk0gglZTR48v7mvtv L3JWoQLAb8/B0kCZr6zx1tZw8ovLhgdZFh8/Ba3w= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3109C6EC5B for ; Thu, 13 May 2021 01:16:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3109C6EC5B Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lgx9q-0000np-Vb; Thu, 13 May 2021 01:16:47 +0300 Date: Thu, 13 May 2021 00:55:32 +0300 To: Sergey Kaplun Message-ID: <20210512215532.GE3944@tarantool.org> References: <4dd97d6fbfb470d41e9ef5ad12492a0def664b2b.1620678384.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD95978C26455E69BE0AAE50420B6EFF6A4E51E23FC86C8E287182A05F53808504006DD93D67C552B1D93CAF7CC5FC0C59CCEE48334AA0461D59B60090B13174CA2 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AA1605287C7F04D6EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063727BBC20C3D5F36038638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2C15117B44DFCFB288D287A77AF67E57AA2BB4B7391E66DBBD2E47CDBA5A96583C09775C1D3CA48CFED8438A78DFE0A9E117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA71A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F616AD31D0D18CD5CC6EABA9B74D0DA47B5C8C57E37DE458BEDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C9783140D4935546D86B7CA31E5B19C74FC7CE X-C1DE0DAB: 0D63561A33F958A5463D754B1FD41F6D5BCFEDBD40A72F3BF4ECFE66A1FFECDDD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D1AE09A115117C9692F086BBD277649D5DD25B18B08AA01F434FABDB015B9EF825354314CE37DB2A1D7E09C32AA3244C93D0E3A49E8DFAD55950B30F74018DE35A1673A01BA68E40927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojo6y/qPNd2uxi9OnQLiqVEA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822F21F4F32E23D3210847F698F83DFC53EA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/3] build: pass sysroot to MacOS SDK X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > > 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 > > --- > > > > 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 -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: 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 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 --- 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: 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