[c-ares] Fix cross building #4492

anton-danielsson · 2021-02-05T15:06:21Z

Add option to build tools or not. Defaults to false when cross building and true otherwise.
Add patch to that adds CARES_BUILD_TOOLS on older versions (1.14.0)

This enables the package to be used in iOS and Android builds.

Specify library name and version: c-ares/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.

CLAassistant · 2021-02-05T15:06:25Z

All committers have signed the CLA.

SpaceIm · 2021-02-05T23:47:56Z

Usually we add an option (others cross compilation can produce these executables).
This option could be tools (default value True)
Then you could replace your modification by self._cmake.definitions["CARES_BUILD_TOOLS"] = self.options.tools
And finally you should modify package_info():

        if self.options.tools:
            bin_path = os.path.join(self.package_folder, "bin")
            self.output.info("Appending PATH environment variable: {}".format(bin_path))
            self.env_info.PATH.append(bin_path)

Take also into account, that this option has been added in 1.15.0 in upstream's CMakeLists. Maybe a small patch could be added for 1.14.0 to avoid branching based on versions in conanfile: https://github.com/c-ares/c-ares/blob/17dc1b3102e0dfc3e7e31369989013154ee17893/CMakeLists.txt#L673

SpaceIm · 2021-02-06T00:20:02Z

Please consider DiracResearch#1

anton-danielsson · 2021-02-06T11:15:15Z

Good points @SpaceIm. I have applied the changes you suggested.
I made one slight adjustment to default the tools option to False if cross building, otherwise True.

FYI: I have applied for the EAP thing but I'm not in yet.
As soon as the CI system builds the PR I will make sure to check for any failures :)

conan-center-bot · 2021-02-06T11:28:43Z

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

ghost · 2021-02-07T11:47:29Z

I detected other pull requests that are modifying c-ares/all recipe:

WIP [c-ares] try simple things first #4553

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

conan-center-bot · 2021-02-08T14:58:27Z

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

SpaceIm · 2021-02-08T15:25:32Z

recipes/c-ares/all/conanfile.py

+        # Default build tools to false if cross building, otherwise to true.
+        if self.options.tools == None:
+            self.options.tools = not tools.cross_building(self.settings)


Not sure if we should not delegate this responsability to consumers (coherent with others recipes), so c-ares:tools=True by default
And if you want to cross build to Android/iOS, don't forget to set to False any option like utilities, tools etc in all recipes which have such option.

I just think that options should have good defaults and in almost all situations I think you are not interested to have these tools for the host environment when cross building. The recipe still allows you to set the option if the default is not what you want.

I'm planing to contribute similar things for other recipes (like protobuf with it's protoc tool). So I guess it good to take make a decision here and go with it :)

Why not. Let's wait opinion of others contributors.

Anyway, if self.options.tools == None: is useless, options values are unknown at config_options time. Just keep self.options.tools = not tools.cross_building(self.settings)

I think the == None is needed. See conan-io/conan#3519.
The "pattern" is taken from the docs: https://docs.conan.io/en/latest/reference/conanfile/attributes.html#default-options

def config_options(self): if self.options.some_option == None: if self.settings.os == 'Android': self.options.some_option = True else: self.options.some_option = False

I noted that conan create and conan install behaves differently.
With conan install the None check is needed but not with conan create. Weird..

conan-center-bot · 2021-02-10T11:44:12Z

Some configurations of 'c-ares/1.17.1' failed in build 9 (4aeb8be7f9599bb9daa15876db13c595f4bd6502):

Macos x86_64, Release, apple-clang 11.0, libc++ . Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Macos x86_64, Release, apple-clang 9.1, libc++ . Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Macos x86_64, Debug, apple-clang 11.0, libc++ . Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Macos x86_64, Debug, apple-clang 9.1, libc++ . Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Macos x86_64, Release, apple-clang 10.0, libc++ . Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Macos x86_64, Release, apple-clang 11.0, libc++ . Options: c-ares:shared-True
- Log download
- Log error Download
- Profile
Access to all the logs
Macos x86_64, Release, apple-clang 9.1, libc++ . Options: c-ares:shared-True
- Log download
- Log error Download
- Profile
Access to all the logs
Macos x86_64, Debug, apple-clang 9.1, libc++ . Options: c-ares:shared-True
- Log download
- Log error Download
- Profile
Access to all the logs
Windows x86_64, Release, Visual Studio 16, MT. Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Windows x86_64, Release, Visual Studio 15, MT. Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Windows x86_64, Release, Visual Studio 14, MD. Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Windows x86_64, Release, Visual Studio 15, MD. Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Windows x86_64, Release, Visual Studio 16, MD. Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs
Windows x86_64, Debug, Visual Studio 14, MTd. Options: c-ares:shared-False
- Log download
- Log error Download
- Profile
Access to all the logs

anton-danielsson · 2021-02-11T09:02:57Z

Sorry, I don't understand what the errors are.
Is it that create_stderr.txt is not empty?
Like:

CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_EXPORT_NO_PACKAGE_REGISTRY


CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_EXPORT_NO_PACKAGE_REGISTRY

I think that's unrelated the this PR and is more likely caused by different CMake versions on the build agents.
(Warning cased by cmake policy stuff)
It can be fixed though, I just need to understand if that's the problem or not.
Could be that I'm completely blind and can't see the real problem...

dmn-star · 2021-02-11T09:56:21Z

This has always been my guess that the (wrong) stdout/stderr redirections/interceptions cause CCI to mark the successful build as failed. @jgsogo

@anton-dirac Try to close the PR and open it again after 15 seconds, maybe the "errors" are gone.

- Add option to build tools or not. Can for example be used to disable tools when cross building. - Add patch to that adds CARES_BUILD_TOOLS on older versions (1.14.0). - iOS, not just Macos, needs `resolv`. This enables the package to be used in iOS and Android builds.

conan-center-bot · 2021-02-13T12:13:28Z

An unexpected error happened and has been reported. Help is on its way! 🏇

conan-center-bot · 2021-02-13T13:46:49Z

All green in build 17 (59565ddc8d8c76cbc0ae4c87b4a076be8c963035)! 😊

c-ares/1.17.1: Generated 88 packages. All logs here
c-ares/1.16.1: Generated 88 packages. All logs here
c-ares/1.15.0: Generated 88 packages. All logs here
c-ares/1.14.0: Generated 88 packages. All logs here

dmn-star · 2021-02-13T14:09:00Z

All of this is really interesting. 🤔

anton-danielsson · 2021-02-16T09:02:30Z

Reviewers:
I'm actually not 100% happy with this PR yet.
If option tools is enabled the iOS build will still fail with:

CMake Error at source_subfolder/src/tools/CMakeLists.txt:19 (INSTALL):
  INSTALL TARGETS given no BUNDLE DESTINATION for MACOSX_BUNDLE executable
  target "ahost".
CMake Error at source_subfolder/src/tools/CMakeLists.txt:36 (INSTALL):
  INSTALL TARGETS given no BUNDLE DESTINATION for MACOSX_BUNDLE executable
  target "adig".
CMake Error at source_subfolder/src/tools/CMakeLists.txt:53 (INSTALL):
  INSTALL TARGETS given no BUNDLE DESTINATION for MACOSX_BUNDLE executable
  target "acountry".

I think the only way to fix this is to patch the CMakeLists file in c-ares.
However, I see no reason to build these executables for iOS (or Android).
Would it be OK to just skip tools if iOS or Android?
If not I think I want to add a patch for c-ares.

jgsogo · 2021-02-16T09:07:12Z

It is totally fine. Someone can take care of them in the future if those tools are needed in a ios/android scenario. We cannot fix everything in each PRs, let's go step by step!

Every contribution helps to improve C++ ecosystem.

Thanks a lot!

SpaceIm · 2021-02-16T09:07:53Z

No because it will require to patch c-ares forever. I prefer to wait that conan supports setting default option value with tools.cross_building(self.settings). This PR already improves the situation for iOS/Android, now consumers can set tools option at least, even if it doesn't work out of the box with default values.

anton-danielsson · 2021-02-16T09:19:53Z

Ok, makes sense. So merge this and I might create a new PR in the future depending on how other things pan out?
I might also make a PR to upstream c-ares to fix the install problem. I agree that maintaining patches is a nightmare..

anton-danielsson · 2021-02-16T10:45:31Z

FYI: It was a one-liner fix in c-ares c-ares/c-ares#395
With some luck it will be included in the next release :)

SpaceIm · 2021-02-16T10:53:37Z

Nice, it looks good to me to add a patch in all current versions of c-ares in CCI.

Fixes: INSTALL TARGETS given no BUNDLE DESTINATION for MACOSX_BUNDLE executable on iOS.

anton-danielsson · 2021-02-16T11:34:28Z

@SpaceIm Do you mean like: 65313e9?
BTW, is there a magic trick to avoid having to create different patches for different versions?

SpaceIm · 2021-02-16T12:31:56Z

@SpaceIm Do you mean like: 65313e9?
BTW, is there a magic trick to avoid having to create different patches for different versions?

Yes and it depends.
If we must live with a patch forever, tools.replace_in_file() is your friend, otherwise patch files are preferred (to keep conanfile.py as generic as possible). Here you can't have the same patch for all versions since lines numbers are slightly different, and tools.replace_in_file() is not a good solution because it will be fixed upstream.

conan-center-bot · 2021-02-17T11:02:05Z

All green in build 18 (65313e9f7c108fd834602f17d0688a72f2aaf257)! 😊

c-ares/1.17.1: Generated 88 packages. All logs here
c-ares/1.16.1: Generated 88 packages. All logs here
c-ares/1.15.0: Generated 88 packages. All logs here
c-ares/1.14.0: Generated 88 packages. All logs here