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

libcurl iOS build added #1008

Merged
merged 10 commits into from
Mar 26, 2020
Merged

libcurl iOS build added #1008

merged 10 commits into from
Mar 26, 2020

Conversation

a4z
Copy link
Contributor

@a4z a4z commented Mar 1, 2020

libcurl/7.78.0 iOS build added ( and Android build checked)

  • 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.

see #955 ,
maybe wait for possible feedback from the issue

@conan-center-bot
Copy link
Collaborator

All green in build 1 (0fccc036da1d5409acfa71a1081174b427b00426)! 😊

recipes/libcurl/all/conanfile.py Outdated Show resolved Hide resolved
recipes/libcurl/all/conanfile.py Outdated Show resolved Hide resolved
@a4z
Copy link
Contributor Author

a4z commented Mar 1, 2020

thanks a lot for the feedback (so far)!
will update the PR according to the notes tomorrow morning

params.append("--enable-threaded-resolver")
params.append("--disable-verbose")
# if there is anything special for arm or simulator, add it here
if "arm" in self.settings.arch:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's nice to leave code blocks doing nothing right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe for documentation purpose it is OK ?

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a helper method to get sysroot flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? I thought I use it ?
tools.XCRun(self.settings, iphoneos).sdk_path

arch_flag = "-arch {}".format(configure_arch)
ios_min_version = "-miphoneos-version-min={}".format(ios_dev_target)
bitcode = "-fembed-bitcode" if self.options.enable_bitcode else ""
extra_flag = "-Werror=partial-availability"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need -Werror?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we don't, but if we have a build config that is incorrect by using something not available on all deployment targets, maybe the build shall fail?

autotools_vars['LDFLAGS'] = "{} {}".format(arch_flag, sysroot)
autotools_vars['CPPFLAGS'] += extra_def

elif self.settings.os == "Android":
Copy link
Contributor

Choose a reason for hiding this comment

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

same, pls avoid adding empty code blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shall I also remove the system_requirements. function?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, if it does nothing

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 there is documentation, like, it should be like this, or hey, for Android, its tested but there is nothing to do, I think I prefer having this in the file, since it is documentation

But I removed the parts that did nothing and had no docu purpose


if self.settings.os == "iOS" and self.settings.arch == "x86_64":
# please do not autodetect --build for the iOS simulator, thanks!
self._autotools.configure(vars=autotools_vars, args=configure_args, build=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a bug in conan that it doesn't properly deduce build for iOS simulator?
I am asking cause we already had similar PR for another package in the past

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe a bug, I do not know,
it will generate a wrong --build argument for configure run, the default (auto detection) from automake gets it right , so it is better to not let conan detect the --build argument for the simulator, since it will get it wrong

autotools_vars['CFLAGS'] = "{} {} {} {} {}".format(
sysroot, arch_flag, ios_min_version, bitcode, extra_flag
)
autotools_vars['LDFLAGS'] = "{} {}".format(arch_flag, sysroot)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it would be better to manipulate the properties of AutoToolsBuildEnvironment (cflags, link_flags) instead of overriding environment variables. the primary reason that user may have set CFLAGS/LDFLAGS within his conan profile, and such flags would be lost here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will check what is possible, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does not work, with not using the environment variables

reduced to the smallest example, using the -isysroot flag

having it in the environment

configure:4717: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang  -c -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk -arch armv7 -miphoneos-version-min=10  -Werror=partial-availability  -DHAVE_SOCKET -DHAVE_FCNTL_O_NONBLOCK conftest.c >&5
configure:4717: $? = 0
configure:4730: result: none needed

using self._autotools.flags.append(sysroot) , where sysroot is the exact same path

configure:4717: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang  -c -arch armv7 -miphoneos-version-min=10  -Werror=partial-availability  -DHAVE_SOCKET -DHAVE_FCNTL_O_NONBLOCK conftest.c >&5
conftest.c:10:10: fatal error: 'stdio.h' file not found
#include <stdio.h>
         ^~~~~~~~~
1 error generated.
configure:4717: $? = 1
configure: failed program was:
......

so obviously, this is not the same for configure, the values are not passed on

I have now invested some hours in this and will not continue not using the env_var, if you think you can improve, please provide a working example


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

Choose a reason for hiding this comment

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

dangerous, as settings.os.version is not always there. use self.settings.get_safe("os.version").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will it not 'just crash' ?
but ok, what to do if get_safe returns None
raise a configuration error ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, raise a ConanInvalidConfiguration.

@conan-center-bot
Copy link
Collaborator

All green in build 2 (63dc9f552783fa0d14a57b6f824b795a3506ad15)! 😊

a4z added 4 commits March 2, 2020 15:56
* remove code doing nothing
* use inbuild tools instead of self made functionality
@conan-center-bot
Copy link
Collaborator

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

@danimtb
Copy link
Member

danimtb commented Mar 2, 2020

There was an error contacting the server. Relaunching the CI...

@conan-center-bot
Copy link
Collaborator

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

@a4z
Copy link
Contributor Author

a4z commented Mar 3, 2020

I assume the not successful check, for which I can not see any details due to permissions, are a side effect of #1020 , yes?

@conan-center-bot
Copy link
Collaborator

All green in build 6 (4496ea8df31c906017799d16113a365833fb4c31)! 😊

@conan-center-bot
Copy link
Collaborator

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

@conan-center-bot
Copy link
Collaborator

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

@danimtb
Copy link
Member

danimtb commented Mar 6, 2020

Sorry, we are experiencing some networking issues today. I have relaunched once again the job.

@conan-center-bot
Copy link
Collaborator

All green in build 9 (3aaa3b7fd5a1da3ff2ef1aeb24085a5f1818cb01)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 10 (0fbc9110e428170977da7506b266e12e8f9f46bf)! 😊

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.

7 participants