[libcurl] update usage of tools.apple_deployment_target_flag #4457

SSE4 · 2021-02-02T06:35:03Z

conan-io/conan#8263 adds os.sdk sub-setting, conan-io/conan#8264 adds os.subsystem sub-setting, they need to be passed to the tools.apple_deployment_target_flag

P.S. the code to handle iOS in this recipe is HUGE mess... e.g. it sets a lot of things manually, which should have been done automatically by the AutoToolsBuildEnvironment helper. this has to be cleaned up, but that's for another PR (needs to be checked carefully).

Specify library name and version: libcurl/all

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

Signed-off-by: SSE4 <[email protected]>

Automatically created by bincrafters-conventions 0.30.2

ghost · 2021-02-02T06:43:44Z

I detected other pull requests that are modifying libcurl/all recipe:

libcurl: add xp compatibility option #4385

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

a4z · 2021-02-03T19:08:18Z

doesn't this change also mean you need a minimum conan version for being able to build that package?

SSE4 · 2021-02-05T08:07:27Z

recipes/libcurl/all/conanfile.py

@@ -170,7 +170,7 @@ def build_requirements(self):
           tools.os_info.detect_windows_subsystem() != "msys2":
            self.build_requires("msys2/20200517")
        elif self._is_win_x_android:
-            self.build_requires("ninja/1.10.1")
+            self.build_requires("ninja/1.10.2")


auto-added by conventions

… sysroot, arch and min version Signed-off-by: SSE4 <[email protected]>

SpaceIm · 2021-02-09T11:31:51Z

recipes/libcurl/all/conanfile.py

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


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

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

SpaceIm · 2021-02-09T11:32:32Z

recipes/libcurl/all/conanfile.py

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


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

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

SpaceIm · 2021-02-09T11:34:10Z

recipes/libcurl/all/conanfile.py

@@ -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("autoreconf -fiv", win_bash=tools.os_info.is_windows, run_environment=True)


Suggested change

self.run("autoreconf -fiv", win_bash=tools.os_info.is_windows, run_environment=True)

self.run("{} -fiv".format(tools.get_env("AUTORECONF")), win_bash=tools.os_info.is_windows, run_environment=True)

this will raise if AUTORECONF not in the env

Might need to require libtool for mingw also in build_requirement.

AUTORECONF is in env since we have autoconf in build requirement (through libtool)

build_requirement also needs to be updated, to not break MinGW:

def build_requirements(self): if not self._is_using_cmake_build: self.build_requires("libtool/2.4.6") self.build_requires("pkgconf/1.7.3") if tools.os_info.is_windows and not tools.get_env("CONAN_BASH_PATH"): self.build_requires("msys2/20200517") elif self._is_win_x_android: self.build_requires("ninja/1.10.2")

it's fine for me (keep buildconf, and replace this logic in another PR).

Just one comment, and @madebr may confirm: msys2 recipe does not guarantee to provide autoreconf of pkg-config. This is why we slowly try to build require pkgconf or autoconf/automake/libtool etc for mingw if required during build, even if msys2 is required (msys2 is just used for the shell).

@a4z I probably will revert back to buildconf for now, as it seems to be problematic to avoid breakage for MinGW. we may try one more time in another PR :)

See https://github.com/curl/curl/blob/ec5d9b44a2e837fc7b82d1c60d5fae3f851620dc/buildconf#L3
I don't see the point in using buildconf

as I understand, buildconf script is more or less equivalent to:

self.run("{} -fiv".format(tools.get_env("AUTORECONF") or "autoreconf"), win_bash=tools.os_info.is_windows, run_environment=True)

so, this change should be okay

What do you need autoreconf for here anyway? Wouldn't configure be enough?

Co-authored-by: SpaceIm <[email protected]>

dmn-star · 2021-02-09T15:52:19Z

recipes/libcurl/all/conanfile.py

@@ -170,7 +170,7 @@ def build_requirements(self):
           tools.os_info.detect_windows_subsystem() != "msys2":
            self.build_requires("msys2/20200517")


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

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

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

@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"

@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

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.

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.

conan-center-bot · 2021-02-09T16:51:22Z

Failure in build 8 (00b1918caa8bb5de1634f588d7a0019df6853e25):

libcurl/7.74.0@:
All packages built successfully! (All logs)
libcurl/7.72.0@:
All packages built successfully! (All logs)
libcurl/7.73.0@:
All packages built successfully! (All logs)
libcurl/7.66.0@:
All packages built successfully! (All logs)
libcurl/7.70.0@:
All packages built successfully! (All logs)
libcurl/7.71.0@:
All packages built successfully! (All logs)
libcurl/7.67.0@:
All packages built successfully! (All logs)
libcurl/7.64.1@:
All packages built successfully! (All logs)
libcurl/7.71.1@:
All packages built successfully! (All logs)
libcurl/7.68.0@:
An unexpected error happened and has been reported
libcurl/7.69.1@:
All packages built successfully! (All logs)

^{Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.}

conan-center-bot · 2021-02-09T18:21:32Z

All green in build 9 (00b1918caa8bb5de1634f588d7a0019df6853e25):

libcurl/7.74.0@:
All packages built successfully! (All logs)
libcurl/7.71.1@:
All packages built successfully! (All logs)
libcurl/7.73.0@:
All packages built successfully! (All logs)
libcurl/7.72.0@:
All packages built successfully! (All logs)
libcurl/7.71.0@:
All packages built successfully! (All logs)
libcurl/7.70.0@:
All packages built successfully! (All logs)
libcurl/7.69.1@:
All packages built successfully! (All logs)
libcurl/7.64.1@:
All packages built successfully! (All logs)
libcurl/7.68.0@:
All packages built successfully! (All logs)
libcurl/7.67.0@:
All packages built successfully! (All logs)
libcurl/7.66.0@:
All packages built successfully! (All logs)

recipes/libcurl/all/conanfile.py

conan-center-bot · 2021-02-10T08:25:02Z

All green in build 10 (9442753c9fe1cf24c64319eeb1274f5b608ca255):

libcurl/7.73.0@:
All packages built successfully! (All logs)
libcurl/7.74.0@:
All packages built successfully! (All logs)
libcurl/7.72.0@:
All packages built successfully! (All logs)
libcurl/7.66.0@:
All packages built successfully! (All logs)
libcurl/7.70.0@:
All packages built successfully! (All logs)
libcurl/7.68.0@:
All packages built successfully! (All logs)
libcurl/7.67.0@:
All packages built successfully! (All logs)
libcurl/7.69.1@:
All packages built successfully! (All logs)
libcurl/7.64.1@:
All packages built successfully! (All logs)
libcurl/7.71.1@:
All packages built successfully! (All logs)
libcurl/7.71.0@:
All packages built successfully! (All logs)

SSE4 · 2021-02-10T08:31:21Z

okay, let's finalize, no more improvements, please :) as every new change has to be tested with MinGW and iOS (which are not covered by CCI), which is not very convenient for me (it requires a reboot to bootcamp and back to mac).

what is not included (and won't be) in this PR:

switching zlib and other deps to pkg-config
using conan's autoconf for MinGW (appears to be broken)
not using MSYS for Android

a4z · 2021-02-10T09:26:11Z

Awesome work!
and I totally agree that the rest has to be part of future iterations.

prince-chrismc

Merge!

SSE4 and others added 2 commits February 2, 2021 13:32

- [libcurl] update usage of tools.apple_deployment_target_flag

bd43802

Signed-off-by: SSE4 <[email protected]>

libcurl: Update Conan conventions

6bf43e5

Automatically created by bincrafters-conventions 0.30.2

SSE4 requested a review from uilianries February 2, 2021 06:35

ghost mentioned this pull request Feb 2, 2021

[service] Potentially conflicting recipes #3571

Closed

ghost mentioned this pull request Feb 2, 2021

libcurl: add xp compatibility option #4385

Closed

4 tasks

github-actions bot mentioned this pull request Feb 2, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

SSE4 closed this Feb 2, 2021

SSE4 reopened this Feb 2, 2021

This comment has been minimized.

Sign in to view

conan-center-bot added the Unexpected Error label Feb 2, 2021

SSE4 closed this Feb 2, 2021

SSE4 reopened this Feb 2, 2021

This comment has been minimized.

Sign in to view

conan-center-bot added Failed and removed Unexpected Error labels Feb 3, 2021

SSE4 closed this Feb 3, 2021

SSE4 reopened this Feb 3, 2021

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Feb 4, 2021

SSE4 commented Feb 5, 2021

View reviewed changes

- remove iOS-specific code, AutoToolsBuildEnvironment already handles…

cbad6fe

… sysroot, arch and min version Signed-off-by: SSE4 <[email protected]>

This comment has been minimized.

Sign in to view

conan-center-bot added Failed Unexpected Error labels Feb 6, 2021

SSE4 closed this Feb 6, 2021

SSE4 reopened this Feb 6, 2021

This comment has been minimized.

Sign in to view

conan-center-bot removed the Unexpected Error label Feb 6, 2021

prince-chrismc previously approved these changes Feb 8, 2021

View reviewed changes

SpaceIm reviewed Feb 9, 2021

View reviewed changes

Update recipes/libcurl/all/conanfile.py

00b1918

Co-authored-by: SpaceIm <[email protected]>

SSE4 dismissed stale reviews from prince-chrismc and a4z via 00b1918 February 9, 2021 11:43

a4z previously approved these changes Feb 9, 2021

View reviewed changes

dmn-star reviewed Feb 9, 2021

View reviewed changes

conan-center-bot added Failed Unexpected Error labels Feb 9, 2021

conan-center-bot removed Unexpected Error Failed labels Feb 9, 2021

SSE4 commented Feb 9, 2021

View reviewed changes

recipes/libcurl/all/conanfile.py Outdated Show resolved Hide resolved

SSE4 commented Feb 10, 2021

View reviewed changes

recipes/libcurl/all/conanfile.py Outdated Show resolved Hide resolved

Update recipes/libcurl/all/conanfile.py

9442753

SSE4 dismissed a4z’s stale review via 9442753 February 10, 2021 04:50

a4z approved these changes Feb 10, 2021

View reviewed changes

dmn-star approved these changes Feb 10, 2021

View reviewed changes

prince-chrismc approved these changes Feb 10, 2021

View reviewed changes

SpaceIm approved these changes Feb 10, 2021

View reviewed changes

uilianries approved these changes Feb 12, 2021

View reviewed changes

conan-center-bot merged commit 0239862 into conan-io:master Feb 12, 2021

CroydonBot deleted the libcurl_apple_subsettings branch February 12, 2021 17:27

	self.run("autoreconf -fiv", win_bash=tools.os_info.is_windows, run_environment=True)
	self.run("{} -fiv".format(tools.get_env("AUTORECONF")), win_bash=tools.os_info.is_windows, run_environment=True)

		@@ -170,7 +170,7 @@ def build_requirements(self):
		tools.os_info.detect_windows_subsystem() != "msys2":
		self.build_requires("msys2/20200517")

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

[libcurl] update usage of tools.apple_deployment_target_flag #4457

[libcurl] update usage of tools.apple_deployment_target_flag #4457

Conversation

SSE4 commented Feb 2, 2021

ghost commented Feb 2, 2021

This comment has been minimized.

This comment has been minimized.

a4z commented Feb 3, 2021

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conan-center-bot commented Feb 9, 2021

conan-center-bot commented Feb 9, 2021

conan-center-bot commented Feb 10, 2021

SSE4 commented Feb 10, 2021

a4z commented Feb 10, 2021

prince-chrismc left a comment

Choose a reason for hiding this comment

SpaceIm Feb 9, 2021 •

edited

Loading

SpaceIm Feb 9, 2021 •

edited

Loading