[tarantool-patches] Re: [PATCH v2] travis-ci: fixed Mojave mac build

Alexander Turenko alexander.turenko at tarantool.org
Thu Mar 14 02:44:33 MSK 2019


> travis-ci: fixed Mojave mac build

Nit: Use imperative mood in a commit header. See
https://tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message

The Travis-CI changes looks okay for me.

I have comments re CMake changes, see below. I added proposed changes on
top of your commit. Please, if you are agree, test them and squash.

Then, please, send the next version of the patch to one of maintainers.

WBR, Alexander Turenko.

> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-3797-mojave-mac-build
> Issue: https://github.com/tarantool/tarantool/issues/3797
 
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -196,10 +196,19 @@ macro(luajit_build)
>          if ("${CMAKE_OSX_DEPLOYMENT_TARGET}" STREQUAL "")
>              # Default to 10.6 since @rpath support is NOT available in
>              # earlier versions, needed by AddressSanitizer.
> -            set (luajit_osx_deployment_target 10.6)
> +            execute_process(COMMAND sw_vers -productVersion
> +                OUTPUT_VARIABLE PRODUCT_VERSION)
> +            message(STATUS "PRODUCT_VERSION=${PRODUCT_VERSION}")
> +            if (${PRODUCT_VERSION} VERSION_LESS 10.14)
> +                set (luajit_osx_deployment_target 10.6)
> +            else ()
> +                set (luajit_osx_deployment_target 10.14)
> +            endif ()

According to [1] `make MACOSX_DEPLOYMENT_TARGET=10.6` is enough, so we
can get rid of this `if` and just set it always to 10.6.

[1]: https://github.com/LuaJIT/LuaJIT/issues/484

>          else()
>              set (luajit_osx_deployment_target ${CMAKE_OSX_DEPLOYMENT_TARGET})
>          endif()
> +        set(macosx_deployment_target_env MACOSX_DEPLOYMENT_TARGET=${luajit_osx_deployment_target})

We can add MACOSX_DEPLOYMENT_TARGET="${luajit_osx_deployment_target}" into
set(luajit_buildoptions ...) below instead.

> +        message(STATUS "${macosx_deployment_target_env}")
>          set(luajit_ldflags
>              ${luajit_ldflags} -Wl,-macosx_version_min,${luajit_osx_deployment_target})
>      endif ()
> @@ -238,7 +247,7 @@ macro(luajit_build)
>          add_custom_command(OUTPUT ${PROJECT_BINARY_DIR}/third_party/luajit/src/libluajit.a
>              WORKING_DIRECTORY ${PROJECT_BINARY_DIR}/third_party/luajit
>              COMMAND $(MAKE) ${luajit_buildoptions} clean
> -            COMMAND $(MAKE) -C src ${luajit_buildoptions} jit/vmdef.lua libluajit.a
> +            COMMAND ${macosx_deployment_target_env} $(MAKE) -C src ${luajit_buildoptions} jit/vmdef.lua libluajit.a

No need to set provide env if we'll add it MACOSX_DEPLOYMENT_TARGET to
${luajit_buildoptions}. It looks simpler.

>              DEPENDS ${CMAKE_SOURCE_DIR}/CMakeCache.txt
>          )
>      else()
> @@ -249,7 +258,7 @@ macro(luajit_build)
>              WORKING_DIRECTORY ${PROJECT_BINARY_DIR}/third_party/luajit
>              COMMAND ${CMAKE_COMMAND} -E copy_directory ${PROJECT_SOURCE_DIR}/third_party/luajit ${PROJECT_BINARY_DIR}/third_party/luajit
>              COMMAND $(MAKE) ${luajit_buildoptions} clean
> -            COMMAND $(MAKE) -C src ${luajit_buildoptions} jit/vmdef.lua libluajit.a
> +            COMMAND ${macosx_deployment_target_env} $(MAKE) -C src ${luajit_buildoptions} jit/vmdef.lua libluajit.a

The same.

>              DEPENDS ${PROJECT_BINARY_DIR}/CMakeCache.txt ${PROJECT_BINARY_DIR}/third_party/luajit
>          )
>      endif()
> -- 
> 2.17.1
> 




More information about the Tarantool-patches mailing list