Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cmake flag 'CMAKE_OSX_DEPLOYMENT_TARGET' according to settings.os.version #3486

Merged
merged 4 commits into from Sep 28, 2018

Conversation

Projects
None yet
4 participants
@jgsogo
Copy link
Member

commented Sep 5, 2018

  • close #3346
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • No need for docs
  • Discussion ongoing in #3346

Changelog: Feature: Added definition CMAKE_OSX_DEPLOYMENT_TARGET to the CMake build helper following the os.version setting for Macos.

@ghost ghost assigned jgsogo Sep 5, 2018

@ghost ghost added the stage: review label Sep 5, 2018

@@ -168,6 +168,8 @@ def _cmake_cross_build_defines(self):
ret["CMAKE_SYSTEM_NAME"] = "Generic"
if os_ver:
ret["CMAKE_SYSTEM_VERSION"] = os_ver
if str(os_).lower() == "macos":

This comment has been minimized.

Copy link
@lasote

lasote Sep 5, 2018

Contributor

why the lower? the setting is standardized as "Macos"

This comment has been minimized.

Copy link
@jgsogo

jgsogo Sep 5, 2018

Author Member

It was a copy/paste from

if str(os_).lower() == "macos":
, let's use it like Macos and see what happens

@@ -962,7 +963,8 @@ def test_cmake_system_version_android(self):
cmake = CMake(conan_file)
self.assertEquals(cmake.definitions["CMAKE_SYSTEM_VERSION"], "32")

def test_cmake_system_version_osx(self):
@mock.patch('platform.system', return_value="Macos")

This comment has been minimized.

Copy link
@lasote

lasote Sep 6, 2018

Contributor

In general, I have some aversion to the mocking and especially with system-wide things. It SHOULD work but I'm always afraid parallelism of tests or stuff like that could affect. I would be ok if the test only runs in Macos

@jgsogo jgsogo added this to the 1.8 milestone Sep 19, 2018

@lasote lasote merged commit 7e9727e into conan-io:develop Sep 28, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Sep 28, 2018

@jgsogo jgsogo deleted the jgsogo:issue/3346 branch Sep 28, 2018

@Croydon

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

Why is this named CMAKE_OSX_DEPLOYMENT_TARGET instead of CMAKE_MACOS_DEPLOYMENT_TARGET?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

@Croydon

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

Sorry, my brain read CONAN_OSX_.. instead of CMAKE_OSX_... 💁🏼🙈

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Add cmake flag 'CMAKE_OSX_DEPLOYMENT_TARGET' according to settings.os…
….version (conan-io#3486)

* Add cmake flag 'CMAKE_OSX_DEPLOYMENT_TARGET' following CMAKE_SYSTEM_VERSION

* remove line

* no cross building in CI when running in other platforms

* no lowercase, no need for it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.