-
Notifications
You must be signed in to change notification settings - Fork 979
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
Autotools adds apple deployement target flag #7862
Conversation
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, |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
There was a problem hiding this 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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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. |
Ok I leave this in my inbox and will look at it when 1.31 will be out! |
Changelog: Fix: Automatically add OSX deployment flags in
AutootoolsBuildEnvironment
with the value ofos_version
, unless the values are already defined in environment variablesCFLAGS
orCXXFLAGS
.Docs: Omit
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