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

Fix iOS cc_library build for protobuf. #3757

Merged
merged 1 commit into from Oct 20, 2017

Conversation

Projects
None yet
6 participants
@spinorx
Contributor

spinorx commented Oct 16, 2017

The SDK and os versions were hard coded. Archs were mixed up.
Because of this, Was getting errors with latest SDK:
clang: warning: no such sysroot directory: '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.2.sdk/' [-Wmissing-sysroot]
clang: warning: no such sysroot directory: '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.2.sdk/' [-Wmissing-sysroot]
clang: warning: no such sysroot directory: '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.2.sdk/' [-Wmissing-sysroot]
In file included from external/com_google_protobuf/src/google/protobuf/io/printer.cc:35:
In file included from external/com_google_protobuf/src/google/protobuf/io/printer.h:40:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:470:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string_view:171:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__string:56:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/algorithm:638:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cstring:61:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string.h:61:15: fatal error: 'string.h' file not found
^~~~~~~~~~
1 error generated.

Currently none of these are needed when using bazel with https://github.com/bazelbuild/rules_apple.

  1. -target arm64-apple-ios etc is passed properly to clang. So -arch armv7 etc are not needed.
    OS_IOS is not used anywhere.
  2. Sources have: GOOGLE_PROTOBUF_NO_THREADLOCAL defined in src/google/protobuf/stubs/platform_macros.h for iOS. So __thread= is not needed. In fact now that bazel is using C++11 by default, __thread should ideally be moved to thread_local.
  3. -miphoneos-version-min is passed by rules_apple.
Fix iOS cc_library build for protobuf.
The SDK and os versions were hard coded.  Archs were mixed up.
Because of this,  Was getting errors with latest SDK:
clang: warning: no such sysroot directory: '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.2.sdk/' [-Wmissing-sysroot]
clang: warning: no such sysroot directory: '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.2.sdk/' [-Wmissing-sysroot]
clang: warning: no such sysroot directory: '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.2.sdk/' [-Wmissing-sysroot]
In file included from external/com_google_protobuf/src/google/protobuf/io/printer.cc:35:
In file included from external/com_google_protobuf/src/google/protobuf/io/printer.h:40:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:470:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string_view:171:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/__string:56:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/algorithm:638:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cstring:61:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string.h:61:15: fatal error: 'string.h' file not found
              ^~~~~~~~~~
              1 error generated.

Currently none of these are needed when using bazel with https://github.com/bazelbuild/rules_apple.
-target arm64-apple-ios is passed properly to clang.  So -arch armv7 etc are not needed.
OS_IOS is not used anywhere.
Sources have:  GOOGLE_PROTOBUF_NO_THREADLOCAL defined in src/google/protobuf/stubs/platform_macros.h for iOS.  So __thread= is not needed.  In fact now that bazel is using C++11 by default,  __thread should ideally be moved to thread_local.
-miphoneos-version-min is passed by rules_apple.
@grpc-jenkins

This comment has been minimized.

Show comment
Hide comment
@grpc-jenkins

grpc-jenkins Oct 16, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-jenkins commented Oct 16, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-jenkins

This comment has been minimized.

Show comment
Hide comment
@grpc-jenkins

grpc-jenkins Oct 16, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-jenkins commented Oct 16, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@bazel-io

This comment has been minimized.

Show comment
Hide comment
@bazel-io

bazel-io Oct 16, 2017

Can one of the admins verify this patch?

bazel-io commented Oct 16, 2017

Can one of the admins verify this patch?

@spinorx

This comment has been minimized.

Show comment
Hide comment
@spinorx

spinorx Oct 19, 2017

Contributor

Ping. Can someone please take a look.

Contributor

spinorx commented Oct 19, 2017

Ping. Can someone please take a look.

@liujisi

This comment has been minimized.

Show comment
Hide comment
@liujisi

liujisi Oct 19, 2017

Contributor

What about other users without using the rules_apple? Can you make it work for both cases?

Contributor

liujisi commented Oct 19, 2017

What about other users without using the rules_apple? Can you make it work for both cases?

@liujisi liujisi added bazel ios labels Oct 19, 2017

@spinorx

This comment has been minimized.

Show comment
Hide comment
@spinorx

spinorx Oct 19, 2017

Contributor

@pherl - Not sure what other users is being referred to here. I verified it on linux and android. I am not aware of any other way to build on apple using bazel other than using rules_apple.

Can you help me understand which other cases you have in mind?

Contributor

spinorx commented Oct 19, 2017

@pherl - Not sure what other users is being referred to here. I verified it on linux and android. I am not aware of any other way to build on apple using bazel other than using rules_apple.

Can you help me understand which other cases you have in mind?

@liujisi liujisi requested a review from thomasvl Oct 19, 2017

@liujisi

This comment has been minimized.

Show comment
Hide comment
@liujisi

liujisi Oct 19, 2017

Contributor

@petewarden who added those configuration to also take a look.

Contributor

liujisi commented Oct 19, 2017

@petewarden who added those configuration to also take a look.

@liujisi

This comment has been minimized.

Show comment
Hide comment
@liujisi

liujisi Oct 19, 2017

Contributor

ok to test

Contributor

liujisi commented Oct 19, 2017

ok to test

@thomasvl

@pherl sorry, don't know enough about the state of the cc_* rules (and crosstools) in opensourced bazel to say what is right. The current hardcoded values in there now aren't right in that it ties things to a specific Xcode release.

Since there doesn't seem to be anything currently in there for simulator builds, just removing the info will put device and simulator on the same basic setup.

@spinorx

This comment has been minimized.

Show comment
Hide comment
@spinorx

spinorx Oct 19, 2017

Contributor

From my understanding we don't need to distinguish between simulator and devices at the build file level. They are covered with defines like GOOGLE_PROTOBUF_ARCH_ARM and GOOGLE_PROTOBUF_ARCH_AARCH64 in google/protobuf/stubs/platform_macros.h in the sources. For example
google/protobuf/stubs/atomicops.h includes appropriate platform specifics using these:
#elif defined(GOOGLE_PROTOBUF_ARCH_AARCH64)
#include <google/protobuf/stubs/atomicops_internals_arm64_gcc.h>

Though I am a bit confused about _gcc when these are working fine with clang on linux/android and ios.

Contributor

spinorx commented Oct 19, 2017

From my understanding we don't need to distinguish between simulator and devices at the build file level. They are covered with defines like GOOGLE_PROTOBUF_ARCH_ARM and GOOGLE_PROTOBUF_ARCH_AARCH64 in google/protobuf/stubs/platform_macros.h in the sources. For example
google/protobuf/stubs/atomicops.h includes appropriate platform specifics using these:
#elif defined(GOOGLE_PROTOBUF_ARCH_AARCH64)
#include <google/protobuf/stubs/atomicops_internals_arm64_gcc.h>

Though I am a bit confused about _gcc when these are working fine with clang on linux/android and ios.

@liujisi liujisi merged commit b189389 into protocolbuffers:master Oct 20, 2017

5 checks passed

Jenkins Build finished.
Details
Jenkins 32bit Build finished.
Details
cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@spinorx

This comment has been minimized.

Show comment
Hide comment
@spinorx

spinorx Oct 20, 2017

Contributor

Thanks Jisi Liu!

Contributor

spinorx commented Oct 20, 2017

Thanks Jisi Liu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment