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

[protobuf] Enable Android/iOS cross building #4556

Merged

Conversation

anton-danielsson
Copy link
Contributor

@anton-danielsson anton-danielsson commented Feb 13, 2021

Specify library name and version: protobuf/3.13.0

Allows protobuf to be cross built using dual profiles (profile host/build).

Test changed to also build when cross building.
As the test shows a client recipe should typically have both a requires and build requires to protobuf.
One will provide the libraries for the host environment while the other one will proved protoc for the build environment.

I first considered adding an option like "protobuf:tools" that controls if protoc is included or not.
But it seems that Conan has a bug that means you can't have different options for a recipe is in the build and host requirement.
That meant I couldn't have protobuf:tools set to False in host and True in build...
I think it's the same problem as here: conan-io/conan#8443

Has been tested on both android and ios:

Android profile:

Configuration (profile_host):
[settings]
arch=armv8
build_type=Release
compiler=clang
compiler.libcxx=c++_static
compiler.version=11
os=Android
os.api_level=21
[options]
[build_requires]
*: android-ndk/r22
[env]

Configuration (profile_build):
[settings]
arch=x86_64
build_type=Release
compiler=Visual Studio
compiler.runtime=MD
compiler.version=16
os=Windows
[options]
[build_requires]
[env]

iOS profile:

Configuration (profile_host):
[settings]
arch=armv8
build_type=Release
compiler=apple-clang
compiler.libcxx=libc++
compiler.version=11.0
os=iOS
os.version=10.2
[options]
ios-cmake:enable_bitcode=False
[build_requires]
*: ios-cmake/3.1.2
[env]

Configuration (profile_build):
[settings]
arch=x86_64
build_type=Release
compiler=apple-clang
compiler.libcxx=libc++
compiler.version=11.0
os=Macos
[options]
[build_requires]
[env]
  • 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.

@prince-chrismc
Copy link
Contributor

This will require a CCI team member review, I have no clue about this stuff 👀

Great work though!

@anton-danielsson
Copy link
Contributor Author

Is the build still running two days later? 😮

@prince-chrismc
Copy link
Contributor

no it's been a bug recently in CCI the update status got lost...

Close the PR wait 10s and then reopen... that will retrigger CI to run

@conan-center-bot
Copy link
Collaborator

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

@anton-danielsson
Copy link
Contributor Author

Oh... I changed my username a few days ago and now it seems I need to apply for the EAP program again...
I'll do that.

@prince-chrismc
Copy link
Contributor

Oh... I changed my username a few days ago and now it seems I need to apply for the EAP program again...

@danimtb is this something the conan team can help with?

@danimtb
Copy link
Member

danimtb commented Feb 16, 2021

It is done! Thanks for the ping 😄

prince-chrismc
prince-chrismc previously approved these changes Feb 16, 2021
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

LGTM!

👏 for all the upstream PRs

@conan-center-bot
Copy link
Collaborator

All green in build 9 (08297d78d5b4a18a2bd9ac7639f0462c70e8dc5a)! 😊

dmn-star
dmn-star previously approved these changes Feb 17, 2021
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

I first considered adding an option like "protobuf:tools" that controls if protoc is included or not.

Please, remove. Since Bincrafters we tried to do it, at first sight was easy, but after each version update became a hell because that issue you pointed and we need to update each patch. So we unified protoc and protobuf again. If will accept such option, it should be official in the future, so let's wait become official. Keep you PR focused on your feature only, enabling Android building.

anton-danielsson added a commit to DiracResearch/conan-grpc that referenced this pull request Feb 20, 2021
If protoc has been built by both host and build profile this
makes sure CMake finds protoc for the build profile

See discussion in:
conan-io/conan-center-index#4556
@ericLemanissierBot
Copy link

ericLemanissierBot commented Feb 25, 2021

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

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

@anton-danielsson
Copy link
Contributor Author

Reviewers: Any more comments? Should I change something or what do you think?

@SSE4 SSE4 requested a review from uilianries February 26, 2021 13:03
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

My comment is still the same. No Tools! Keep focused in your PR, solve only the problem related to Android.

@conan-center-bot
Copy link
Collaborator

All green in build 18 (6a14a091f3b63f0f7039520d03627c607e58f770):

  • protobuf/3.9.1@:
    All packages built successfully! (All logs)

  • protobuf/3.11.4@:
    All packages built successfully! (All logs)

  • protobuf/3.12.4@:
    All packages built successfully! (All logs)

  • protobuf/3.13.0@:
    All packages built successfully! (All logs)

prince-chrismc
prince-chrismc previously approved these changes Mar 9, 2021
@mathbunnyru
Copy link
Contributor

@anton-danielsson please, do not use push-force after PR creation.
It makes it difficult to see the changes.
And, it might be nice to see the history of changes during PR.

@mjvankampen
Copy link
Contributor

mjvankampen commented Mar 11, 2021

I didn't look at everything, but flatbuffers had a similar issue that was done in a bit different way. https://github.com/conan-io/conan-center-index/blob/master/recipes/flatbuffers/all/conanfile.py . Is the protobuf way the preferred way to tackle this?

@anton-danielsson
Copy link
Contributor Author

I didn't look at everything, but flatbuffers had a similar issue that was done in a bit different way. https://github.com/conan-io/conan-center-index/blob/master/recipes/flatbuffers/all/conanfile.py . Is the protobuf way the preferred way to tackle this?

From what I understand one difference is that the flatbuffers recipe automatically disables the build of flatc for the in the host context.
I also did that (or a very similar thing) in the beginning but it was changed after review comments.
This is also what we have done in our private fork of the protobuf recipe.
For me it doesn't really matter which way we go, I just want cross building to Android and iOS to work :)

mathbunnyru
mathbunnyru previously approved these changes Mar 11, 2021
@prince-chrismc
Copy link
Contributor

I just want cross building to Android and iOS to work :)

So do we! Thanks for all your hard work!

patches/upstream-pr-7761-cmake-regex-fix.patch
and
patches/upstream-pr-7288-android-log.patch
no longer needed, they have been merged before 3.15.5

patches/upstream-pr-8301-bundle-destination.patch
is in master but not in 3.15.5
@anton-danielsson
Copy link
Contributor Author

Had to make a small update (facc289) after #4682 was merged.
Means the approvals where reset again.

@prince-chrismc
Copy link
Contributor

You'll need to retrigger CI, close the pr wait 10s and then re-open it 🔁

@conan-center-bot
Copy link
Collaborator

All green in build 20 (facc2895230f752557d90c3e7e5c005147193ba9):

  • protobuf/3.9.1@:
    All packages built successfully! (All logs)

  • protobuf/3.11.4@:
    All packages built successfully! (All logs)

  • protobuf/3.12.4@:
    All packages built successfully! (All logs)

  • protobuf/3.13.0@:
    All packages built successfully! (All logs)

  • protobuf/3.15.5@:
    All packages built successfully! (All logs)

prince-chrismc referenced this pull request in prince-chrismc/conan-center-index-pending-review Mar 16, 2021
this will allow testing it in a preview wihtout opening an issue
prince-chrismc referenced this pull request in prince-chrismc/conan-center-index-pending-review Mar 16, 2021
prince-chrismc referenced this pull request in prince-chrismc/conan-center-index-pending-review Mar 16, 2021
prince-chrismc referenced this pull request in prince-chrismc/conan-center-index-pending-review Mar 16, 2021
@conan-center-bot conan-center-bot merged commit 369121c into conan-io:master Mar 16, 2021
anton-danielsson added a commit to DiracResearch/conan-grpc that referenced this pull request Mar 21, 2021
If protoc has been built by both host and build profile this
makes sure CMake finds protoc for the build profile

See discussion in:
conan-io/conan-center-index#4556
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.

None yet