Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Alexander Tikhonov <avtikhon@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 1/1] travis-ci: fixed Mojave mac build with CMAKE_OSX_DEPLOYMENT_TARGET
Date: Wed, 20 Feb 2019 17:58:27 +0300	[thread overview]
Message-ID: <20190220145826.4leayus7oo6b4iom@tkn_work_nb> (raw)
In-Reply-To: <1550585135.878144114@f412.i.mail.ru>

Hi!

Common notes:

* Please, squash your patches before send (into 1, 2, several meaningful
  patches, w/o temporary commits).
* Your branch is named
  remotes/origin/avtikhon/gh-3797-fix-mojave-mac-build (with extra
  'remotes/origin/' prefix).
* Please, add me to 'To' or 'CC' header when ask a review from me, so
  I'll not miss it.
* Work hard to fit a commit message header within 50 symbols and a
  commit message within 72 symbols (see our guidelines).

Other comments are below.

Feel free to engage me into discussions / investigations if needed.

WBR, Alexander Turenko.

On Tue, Feb 19, 2019 at 05:05:35PM +0300, Alexander Tikhonov wrote:
> 
> 1. Changed the Travis-CI testing default OSX image from 'xcode9' to 'xcode10.1',
> also added Travis-CI's default 'xcode9.4' image to the '2.1' branch testing.
> 2. Corrected virtualenv setup for OSX images where it is not installed by default.
> 3. Corrected the minimum version of OSX image 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. The CMAKE_OSX_DEPLOYMENT_TARGET depends on DARWIN_VERSION:
> DARWIN_VERSION <= 10.12 -> CMAKE_OSX_DEPLOYMENT_TARGET = 10.6
> DARWIN_VERSION >= 10.13 -> CMAKE_OSX_DEPLOYMENT_TARGET = 10.14
> 
> Closes #3797
> ---
> Travis-CI:  https://travis-ci.org/tarantool/tarantool/builds/495448673  
> Travis-CI(checked all OSX):  https://travis-ci.org/tarantool/tarantool/builds/495415195  
> Branch:  https://github.com/tarantool/tarantool/tree/remotes/origin/avtikhon/gh-3797-fix-mojave-mac-build
> 
> diff --git a/.travis.mk b/.travis.mk
> index edd94cd7d..181bab438 100644
> --- a/.travis.mk
> +++ b/.travis.mk
> @@ -47,6 +47,10 @@ test_ubuntu: deps_ubuntu
>  deps_osx:
>      brew update
>      brew install openssl readline curl icu4c --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 ) && \
> +        brew install pyenv-virtualenv --force && \
> +        pip install virtualenv )

It seems that we install pip twice: here and in test_osx.

I wonder whether all problems are resolved if we just add
pyenv-virtualenv into 'brew install' command in deps_osx?

>  
>  test_osx: deps_osx
>      cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
> diff --git a/.travis.yml b/.travis.yml
> index ffe2e8247..e03bd3185 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -8,7 +8,7 @@ language: cpp
>  os: linux
>  compiler: gcc
>  
> -osx_image: xcode9
> +osx_image: xcode10.1
>  
>  cache:
>      directories:
> @@ -27,6 +27,11 @@ jobs:
>        - name: RelWithDebInfoWError build + test (OS X)
>          env: TARGET=test
>          os: osx
> +      - name: RelWithDebInfoWError build + test (OS X 9.4)
> +        env: TARGET=test
> +        os: osx
> +        osx_image: xcode9.4
> +        if: branch = "2.1"

9.4 is not OS X version, it is xcode version. Proposed name:

RelWithDebInfoWError build + test (OS X 10.13 High Sierra)

See the following links to match os_image values into OS X versions and
names:

https://docs.travis-ci.com/user/reference/osx/#macos-version
https://en.wikipedia.org/wiki/MacOS_version_history#toc

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).

Also consider: Travis-CI added Mojave builds:
https://blog.travis-ci.com/2019-02-12-xcode-10-2-beta-2-is-now-available

Also I still think fix cmakes for Mojave and tweak travis targets should
be in the separate commits.

>        - name: Debug build + test + coverage (Linux, gcc)
>          env: TARGET=coverage
>        - name: LTO build + test (Linux, gcc)
> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index ea4878a93..d2253a0f9 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -194,9 +194,28 @@ macro(luajit_build)
>          endif()
>          # Pass deployment target
>          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)

Since we set 10.6 for some targets, the comment seems to be still
actual. I don't think we should remove it.

> +            # DARWIN_VERSION <= 10.12 -> CMAKE_OSX_DEPLOYMENT_TARGET = 10.6
> +            # DARWIN_VERSION >= 10.13 -> CMAKE_OSX_DEPLOYMENT_TARGET = 10.14
> +            execute_process(COMMAND sw_vers -productVersion
> +                OUTPUT_VARIABLE PRODUCT_VERSION)
> +            message(STATUS "PRODUCT_VERSION=${PRODUCT_VERSION}")
> +            string(REGEX MATCH "[0-9]+.[0-9]+" DARWIN_VERSION ${PRODUCT_VERSION})
> +            message(STATUS "DARWIN_VERSION=${DARWIN_VERSION}")
> +            string(REGEX MATCH "^[0-9]+" MAJOR_VERSION ${DARWIN_VERSION})
> +            message(STATUS "MAJOR_VERSION=${MAJOR_VERSION}")
> +            if (MAJOR_VERSION GREATER 10)
> +                set (luajit_osx_deployment_target 10.14)
> +            elseif (MAJOR_VERSION LESS 10)
> +                set (luajit_osx_deployment_target 10.6)
> +            elseif (MAJOR_VERSION EQUAL 10)
> +                string(REGEX MATCH "[0-9]+$" MINOR_VERSION ${DARWIN_VERSION})
> +                message(STATUS "MINOR_VERSION=${MINOR_VERSION}")
> +                if (MINOR_VERSION GREATER 12)
> +                    set (luajit_osx_deployment_target 10.14)
> +                else ()
> +                    set (luajit_osx_deployment_target 10.6)
> +                endif ()
> +            endif ()

Can we use VERSION_LESS operation to simplify the code?

I think only the first 'message' directive is useful, others are
redundant.

Why the edge is between 10.12-10.13, while a problem we trying to solve
appears only on 10.14 (Mojave)?

>          else()
>              set (luajit_osx_deployment_target ${CMAKE_OSX_DEPLOYMENT_TARGET})
>          endif()
> 
> 
> -- 
> Alexander Tikhonov

  reply	other threads:[~2019-02-20 14:58 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-19 14:05 [tarantool-patches] [tarantool-patches] " Alexander Tikhonov
2019-02-20 14:58 ` Alexander Turenko [this message]
2019-02-21 10:39   ` [tarantool-patches] Re[2]: " Alexander Tikhonov
     [not found]   ` <1550951995.54875386@f494.i.mail.ru>
2019-03-05 20:27     ` [tarantool-patches] " Alexander Turenko
2019-03-12  7:27       ` [tarantool-patches] " Alexander Tikhonov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190220145826.4leayus7oo6b4iom@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=avtikhon@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 1/1] travis-ci: fixed Mojave mac build with CMAKE_OSX_DEPLOYMENT_TARGET' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox