[tarantool-patches] Re: [PATCH 1/1] travis-ci: fixed Mojave mac build with CMAKE_OSX_DEPLOYMENT_TARGET

[Alexander Turenko]

On Sat, Feb 23, 2019 at 10:59:55PM +0300, Alexander Tikhonov wrote:
> 
> Hi!
> 
> Once more time sorry, that I didn't see your comments from the first time ))
> 
> 1) 
> > It seems that we install pip twice: here and in test_osx.
> 
> Right, pip is needed twice:
>  - for virtualenv to be installed while easy_install is depricated
>  - inside the virtualenv
> > I wonder whether all problems are resolved if we just add
> > pyenv-virtualenv into 'brew install' command in deps_osx?
> 
> pyenv-virtualenv into 'brew install' command in deps_osx  can be installed - I'll do it
> 
> but the virtualenv on the new OSX versions is not installed by default and it
> needs additional pip routine installation for 'pip install virtualenv' final install,
> otherwise check the following bugus build without additional pip install command:
> https://travis-ci.org/tarantool/tarantool/jobs/497450467  
> 

ld: library not found for -lgcc_s.10.4
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I don't understood how it is related to pip.

I don't understood your description too to be honest. Let's discuss it
tommorow voicely.

> 2)
> > 9.4 is not OS X version, it is xcode version. Proposed name:
> 
> > RelWithDebInfoWError build + test (OS X 10.13 High Sierra)
> 
> > Now I see both versions are High Sierra. I don't see a reason to have
> > two target of the same Mac OS versions (even despite different xcode
> > versions).
> 
> Right, I'll do your suggestion
> 
> > Also consider: Travis-CI added Mojave builds:
> >  https://blog.travis-ci.com/2019-02-12-xcode-10-2-beta-2-is-now-available
> 
> I missed it, thanks!
> 
> > Also I still think fix cmakes for Mojave and tweak travis targets should
> > be in the separate commits.
> 
> Right, I'll do it in this way
> 
> 3)
> > Since we set 10.6 for some targets, the comment seems to be still
> > actual. I don't think we should remove it.
> Restored back.
> 
> 4)
> > Can we use VERSION_LESS operation to simplify the code?
> 
> Right, changed as suggested
> 
> > I think only the first 'message' directive is useful, others are
> > redundant.
> 
> Ok, sure
> 
> > Why the edge is between 10.12-10.13, while a problem we trying to solve
> > appears only on 10.14 (Mojave)?
> 
> Right, corrected
> 
> Patches below to check the changes made on suggestions:
> https://github.com/tarantool/tarantool/compare/2.1...e4beb12d7d3e   
> https://travis-ci.org/tarantool/tarantool/builds/497561631  
> https://travis-ci.org/tarantool/tarantool/builds/497554600  
> 

I'll paste your commits below as plaintext to comment them per line.

> commit e265d6686761471301a1de2bf0ca7eab157527cc
> Author: Alexander V. Tikhonov <avtikhon at tarantool.org>
> Date:   Sun Feb 24 05:18:58 2019 -0500
> 
>     travis-ci: OSX version image updated
>     
>     Changed the OSX image from 9.4 to 10.2,
>     added 9.4 version image to 2.1 release criteria

The commit message assumes this a reading person has xcode version ->
mac os version mapping in a head. 9.4 is not 'OSX image', it is xcode
version. I think nobody is interested in xcode versions, so it is better
to use Mac OS version in messages.

I would also say 'CI for 2.1 branch' instead of vague '2.1 release
criteria'.

Note: 'OS X' should be written with a whitespace as I see.

> 
> diff --git a/.travis.mk b/.travis.mk
> index edd94cd7d..35ed2612f 100644
> --- a/.travis.mk
> +++ b/.travis.mk
> @@ -46,7 +46,12 @@ test_ubuntu: deps_ubuntu
>  
>  deps_osx:
>  	brew update
> -	brew install openssl readline curl icu4c --force
> +	brew install openssl readline curl icu4c pyenv-virtualenv --force
> +	virtualenv -h >/dev/null 2>&1 || \
> +		( pip -h >/dev/null 2>&1 || \
> +			( curl --silent --show-error --retry 5 https://bootstrap.pypa.io/get-pip.py | python && \
> +			pip --version ) && \
> +		pip install virtualenv )
>  

Answered above.

>  test_osx: deps_osx
>  	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
> diff --git a/.travis.yml b/.travis.yml
> index ffe2e8247..90af044ff 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -8,7 +8,7 @@ language: cpp
>  os: linux
>  compiler: gcc
>  
> -osx_image: xcode9
> +osx_image: xcode10.2

Please, add comment like 'Max OS 10.14'

>  
>  cache:
>      directories:
> @@ -24,11 +24,16 @@ jobs:
>        - name: RelWithDebInfoWError build + test (Linux, clang)
>          env: TARGET=test
>          compiler: clang
> -      - name: RelWithDebInfoWError build + test (OS X)
> -        env: TARGET=test
> +      - name: RelWithDebInfoWError build + test (OS X 10.14 Mojave)
> +        env: TARGET=test MACOSX_DEPLOYMENT_TARGET=10.14

Other targets are named like 'Ubuntu Trusty (14.04) build + deploy DEB':
name of a release, then release number. So I think it should be 'OS X
Mojave 10.14'.

Why do you add MACOSX_DEPLOYMENT_TARGET=10.14 here? As I understand your
work is about allowing to don't set it.

>          os: osx
>        - name: Debug build + test + coverage (Linux, gcc)
>          env: TARGET=coverage
> +      - name: RelWithDebInfoWError build + test (OS X 10.13 High Sierra)
> +        env: TARGET=test
> +        os: osx
> +        osx_image: xcode9.4
> +        if: branch = "2.1"

OS X High Sierra 10.13.

>        - name: LTO build + test (Linux, gcc)
>          env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
>          if: branch = "2.1"
> @@ -36,9 +41,9 @@ jobs:
>          env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
>          if: branch = "2.1"
>          compiler: clang
> -      - name: LTO build + test (OS X)
> +      - name: LTO build + test (OS X 10.14 Mojave)
>          os: osx
> -        env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
> +        env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON MACOSX_DEPLOYMENT_TARGET=10.14

The same: 'OS X Mojave 10.14' and why MACOSX_DEPLOYMENT_TARGET=10.14 is
added?

>          if: branch = "2.1"
>        - name: Create and deploy tarball
>          env: TARGET=source
> 
> commit 0e703e04704de59a29f2f38f2bcb2ec0f028f046
> Author: Alexander V. Tikhonov <avtikhon at tarantool.org>
> Date:   Sun Feb 24 02:38:07 2019 -0500
> 
>     travis-ci: fixed Mojave mac build
>     
>     Fixed Mojave Mac build with CMAKE_OSX_DEPLOYMENT_TARGET.
>     Specified the minimum version of OS X on which the target
>     binaries are to be deployed. CMake uses this value for the
>     -mmacosx-version-min flag and to help choose the default SDK
> 
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index ea4878a93..40f2472ef 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -196,10 +196,20 @@ 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.

Hm. Now the comment is only partially valid. We need to expand it.

> -            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 ()
> +            set(CMAKE_OSX_DEPLOYMENT_TARGET "${luajit_osx_deployment_target}"
> +		CACHE STRING "Minimum OS X deployment version")

Please, don't mix whitespaces and tabs.

BTW, why we're set CMAKE_OSX_DEPLOYMENT_TARGET if it is not used
anywhere else?

>          else()
>              set (luajit_osx_deployment_target ${CMAKE_OSX_DEPLOYMENT_TARGET})
>          endif()
> +        message(STATUS "LUAJIT_OSX_DEPLOYMENT_TARGET=${luajit_osx_deployment_target}")
>          set(luajit_ldflags
>              ${luajit_ldflags} -Wl,-macosx_version_min,${luajit_osx_deployment_target})
>      endif ()