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

Autotools adds apple deployement target flag #7862

Merged

Conversation

MartinDelille
Copy link
Contributor

@MartinDelille MartinDelille commented Oct 11, 2020

Changelog: Fix: Automatically add OSX deployment flags in AutootoolsBuildEnvironment with the value of os_version, unless the values are already defined in environment variables CFLAGS or CXXFLAGS.
Docs: Omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

After submitting this pull request on CCI I realized that maybe that was something that AutoToolsBuildEnvironment could do automatically since 4 recipes are using the same kind of code: zlib, icu, libcurl, openssl

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2020

CLA assistant check
All committers have signed the CLA.

if tools.is_apple_os(self._os):
concat = " ".join(tmp_compilation_flags)
if self._os_version and "-version-min" not in concat and "-target" not in concat:
tmp_compilation_flags.append(tools.apple_deployment_target_flag(self._os,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to understand a bit better (not a OSX user) what is the effect of not adding this flag. Is it a compilation error? What have been the users doing so far for this?

Also, is there a chance that this could be breaking for other users that do not expect this flag to be added? Should this flag be added in other generators as well (cmake, xcode)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this flag is not set and someone is trying to link a library with no minimal os version against an application explicitely specifying it then warning will span like mentioned here: bincrafters/community#1264 (comment)

Regarding the users that do not expect this flag to be added I wondered why they would have specify the os.version setting that (if I'm understanding well) states that the dependencies should support such OS.

Cmake generators allready handle the os.version flags by setting the CMAKE_OSX_DEPLOYMENT_TARGET definition: https://github.com/conan-io/conan/blob/develop/conans/client/build/cmake_flags.py#L216-L219

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is looking good, please have a look to the CXXFLAGS issue and tell me.

@@ -339,6 +341,13 @@ def append(*args):
tmp_compilation_flags = copy.copy(self.flags)
if self.fpic:
tmp_compilation_flags.append(pic_flag(self._conanfile.settings))
if tools.is_apple_os(self._os):
concat = " ".join(tmp_compilation_flags)
if os.environ.get("CFLAGS", None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't the version-min come defined also in CXXFLAGS? I think it would make sense check that var too, just in case, as C++ users are probably defining it in that var, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@memsharded memsharded added this to the 1.31 milestone Oct 17, 2020
@jgsogo jgsogo merged commit fc2078a into conan-io:develop Oct 20, 2020
@memsharded
Copy link
Member

Merged! Will be in Conan 1.31, thanks for your contribution @MartinDelille

When it is released, maybe it is worth to revisit those recipes in conan-center-index to simplify them if the flags are no longer necessary in the recipes.

@MartinDelille
Copy link
Contributor Author

Ok I leave this in my inbox and will look at it when 1.31 will be out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants