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.

Sign up for GitHub

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

Jump to bottom

[libcurl] update usage of tools.apple_deployment_target_flag #4457

Merged
merged 7 commits into from
Feb 12, 2021
Merged

[libcurl] update usage of tools.apple_deployment_target_flag #4457

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd43802
- [libcurl] update usage of tools.apple_deployment_target_flag
SSE4 Feb 2, 2021
6bf43e5
libcurl: Update Conan conventions
bincrafters-user Feb 2, 2021
cbad6fe
- remove iOS-specific code, AutoToolsBuildEnvironment already handles…
SSE4 Feb 6, 2021
d735c72
- use autoreconf instead of buildconf
SSE4 Feb 7, 2021
76eeab9
- use rootpath everywhere instead of lib_paths[0]
SSE4 Feb 7, 2021
00b1918
Update recipes/libcurl/all/conanfile.py
SSE4 Feb 9, 2021
9442753
Update recipes/libcurl/all/conanfile.py
SSE4 Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 7 additions & 58 deletions recipes/libcurl/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def build_requirements(self):
tools.os_info.detect_windows_subsystem() != "msys2":
self.build_requires("msys2/20200517")
Copy link
Contributor

Choose a reason for hiding this comment

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

i would say that is too lax mingw detection. This leads to msys2 being installed as build requirements even though the library is compiled for android for example. #4277

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably, needs also settings.os == "Windows"? and probably, is_mingw property needs to check settings.compiler == "gcc" as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

also don't forget this comment: #4457 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@SSE4 I guess so.

conan-center-index/recipes/libcurl/all/conanfile.py

Line 77 in 4cfe106

return self.settings.os == "Windows" and self.settings.compiler != "Visual Studio"

return self.settings.os == "Windows" and self.settings.compiler == "gcc"

Copy link
Contributor

Choose a reason for hiding this comment

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

@SpaceIm _is_using_cmake_build is always true when cross compile for Android on Windows

    if not self._is_using_cmake_build:
    ....
    elif self._is_win_x_android:

conan-center-index/recipes/libcurl/all/conanfile.py

Lines 84 to 85 in 4cfe106

def _is_using_cmake_build(self):
return self.settings.compiler == "Visual Studio" or self._is_win_x_android

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a couple of another weirdness in the recipe:

  • CMake build is used only for some platforms (e.g. Android)
  • Android build enforces ninja (shouldn't it work with any generator?)

anyway, I am not going to address this right now. let's just merge what we have already, otherwise, it will never end :) future PR may address these problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, building for Android on Windows is just my hobby project. :-) However what I have never understood why the autoconfig build is still used when the CMake build is available.

elif self._is_win_x_android:
self.build_requires("ninja/1.10.1")
self.build_requires("ninja/1.10.2")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

auto-added by conventions

elif not tools.os_info.is_windows:
self.build_requires("libtool/2.4.6")
self.build_requires("pkgconf/1.7.3")
Expand Down Expand Up @@ -249,7 +249,7 @@ def _get_configure_command_args(self):
params.append("--without-wolfssl")

if self.options.with_libssh2:
params.append("--with-libssh2={}".format(tools.unix_path(self.deps_cpp_info["libssh2"].lib_paths[0])))
params.append("--with-libssh2={}".format(tools.unix_path(self.deps_cpp_info["libssh2"].rootpath)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same than zlib: https://github.com/curl/curl/blob/f3294d9d86e6a7915a967efff2842089b8b0d071/configure.ac#L2800

Copy link
Contributor

Choose a reason for hiding this comment

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

not everything has to be done in this PR, SSE4 did an awesome job with this improvement, adding more load on him is maybe not fair and nice. And there can also be space for future improvements :-)

else:
params.append("--without-libssh2")

Expand All @@ -259,7 +259,7 @@ def _get_configure_command_args(self):
params.append("--without-nghttp2")

if self.options.with_zlib:
params.append("--with-zlib={}".format(tools.unix_path(self.deps_cpp_info["zlib"].lib_paths[0])))
params.append("--with-zlib={}".format(tools.unix_path(self.deps_cpp_info["zlib"].rootpath)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to use pkgconf: https://github.com/curl/curl/blob/f3294d9d86e6a7915a967efff2842089b8b0d071/configure.ac#L904

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it's too much, it has to be tested first, and it's not the goal of the PR, so maybe for another one

else:
params.append("--without-zlib")

Expand Down Expand Up @@ -347,7 +347,7 @@ def _patch_mingw_files(self):
def _build_with_autotools(self):
with tools.chdir(self._source_subfolder):
# autoreconf
self.run("./buildconf", win_bash=tools.os_info.is_windows, run_environment=True)
self.run("{} -fiv".format(tools.get_env("AUTORECONF") or "autoreconf"), win_bash=tools.os_info.is_windows, run_environment=True)

# fix generated autotools files on alle to have relocateable binaries
if tools.is_apple_os(self.settings.os):
Expand Down Expand Up @@ -375,60 +375,6 @@ def _configure_autotools_vars(self):
del autotools_vars["LIBS"]
self.output.info("Autotools env vars: " + repr(autotools_vars))

if tools.cross_building(self.settings):
if self.settings.os == "iOS":
iphoneos = tools.apple_sdk_name(self.settings)
ios_dev_target = str(self.settings.os.version).split(".")[0]

env_cppflags = tools.get_env("CPPFLAGS", "")
socket_flags = " -DHAVE_SOCKET -DHAVE_FCNTL_O_NONBLOCK"
if self.settings.arch in ["x86", "x86_64"]:
autotools_vars["CPPFLAGS"] = "-D__IPHONE_OS_VERSION_MIN_REQUIRED={}0000 {} {}".format(
ios_dev_target, socket_flags , env_cppflags)
elif self.settings.arch in ["armv7", "armv7s", "armv8"]:
autotools_vars["CPPFLAGS"] = "{} {}".format(socket_flags, env_cppflags)
else:
raise ConanInvalidConfiguration("Unsuported iOS arch {}".format(self.settings.arch))

cc = tools.XCRun(self.settings, iphoneos).cc
sysroot = "-isysroot {}".format(tools.XCRun(self.settings, iphoneos).sdk_path)

if self.settings.arch == "armv8":
configure_arch = "arm64"
configure_host = "arm" #unused, autodetected
else:
configure_arch = self.settings.arch
configure_host = self.settings.arch #unused, autodetected


arch_flag = "-arch {}".format(configure_arch)
ios_min_version = tools.apple_deployment_target_flag(self.settings.os, self.settings.os.version)
extra_flag = "-Werror=partial-availability"

# if we debug, maybe add a -gdwarf-2 , but why would we want that?

autotools_vars["CC"] = cc
autotools_vars["IPHONEOS_DEPLOYMENT_TARGET"] = ios_dev_target
env_cflags = tools.get_env("CFLAGS", "")
autotools_vars["CFLAGS"] = "{} {} {} {}".format(
sysroot, arch_flag, ios_min_version, env_cflags
)

if self.options.with_ssl == "openssl":
openssl_path = self.deps_cpp_info["openssl"].rootpath
openssl_libdir = self.deps_cpp_info["openssl"].libdirs[0]
autotools_vars["LDFLAGS"] = "{} {} -L{}/{}".format(arch_flag, sysroot, openssl_path, openssl_libdir)
elif self.options.with_ssl == "wolfssl":
wolfssl_path = self.deps_cpp_info["wolfssl"].rootpath
wolfssl_libdir = self.deps_cpp_info["wolfssl"].libdirs[0]
autotools_vars["LDFLAGS"] = "{} {} -L{}/{}".format(arch_flag, sysroot, wolfssl_path, wolfssl_libdir)
else:
autotools_vars["LDFLAGS"] = "{} {}".format(arch_flag, sysroot)

elif self.settings.os == "Android":
# nothing do to at the moment, this seems to just work
pass

return autotools_vars

def _configure_autotools(self):
Expand All @@ -449,6 +395,9 @@ def _configure_autotools(self):

self._autotools.defines.append("_AMD64_")

if tools.cross_building(self) and tools.is_apple_os(self.settings.os):
self._autotools.defines.extend(['HAVE_SOCKET', 'HAVE_FCNTL_O_NONBLOCK'])

configure_args = self._get_configure_command_args()

if self.settings.os == "iOS" and self.settings.arch == "x86_64":
Expand Down