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 AC3CC6F3C4; Tue, 11 May 2021 12:50:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AC3CC6F3C4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1620726631; bh=e2f/bUIngYErnfIKeldWCt4H34wQd6XV7icWJX2UAWg=; 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=KVacDw9V2H0g5AybMpMR+KFLjVLJ648iVhlJnXjwHOAgtQGfvjLTnzVa2wqjkn0jK pjZ9QpLK6ZU9j049l91/H6ATDevOekdT1xB5QLxaulR6UAr0xGY5YNGq7RrYkHeDvF RntP39cNw892cX2Q6semsNNaG93EXF5Ni6tV4GAc= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 82EB66F3C4 for ; Tue, 11 May 2021 12:50:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 82EB66F3C4 Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1lgP25-0000zp-K1; Tue, 11 May 2021 12:50:30 +0300 Date: Tue, 11 May 2021 12:49:25 +0300 To: Igor Munkin Message-ID: References: <4dd97d6fbfb470d41e9ef5ad12492a0def664b2b.1620678384.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4dd97d6fbfb470d41e9ef5ad12492a0def664b2b.1620678384.git.imun@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD95978C26455E69BE0B2C7F7C3B0039F139F0EF8E3717E163C182A05F538085040529DF32A1C97533ED7637057AFAB56B1306F9051885FFD00418948D07D4957DC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C4FDA87F74E4A9F6EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F7C2A16C2A438FE58638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B28666DA94FBAF760B660D15EC03B78CA3C26CFBAC0749D213D2E47CDBA5A96583C09775C1D3CA48CF27ED053E960B195E117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE74A95F4E53E8DCE969FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA71A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16BA91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A55EBCD851251DAC22B9F546548927940572C31ECF786FE681D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34951738D4D62E58A4A0EF3154E87CC21E1CFC888F6C0872D1A52B478E0DAD8ADB4A5831F62E55CB4E1D7E09C32AA3244C3EAF7782F58E52DB53EE6259039E6C9033C9DC155518937FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPN2Bz4bQWZvKM0ww9TQpPA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB458F1EF12CEF049EE29BDDCBD156AD43DF9B348218248E6A5F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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/ > +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). Also you may take a look to Mike's solution here[3]. 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)? > + 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? Feel free to ignore. > +) > + 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. 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