Skip to content

build: fix cross-platform android builds on OS X #237

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.

Sign up for GitHub

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

Merged
devbww merged 6 commits into from
Nov 3, 2022
Merged

build: fix cross-platform android builds on OS X #237

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3eaff4f
build: attempting to fix https://github.com/abseil/abseil-cpp/issues/326
alyssawilk Nov 2, 2022
555c92f
review comments
alyssawilk Nov 2, 2022
a793292
now with comments and full foundation removal
alyssawilk Nov 3, 2022
c214363
trying to improve comment
alyssawilk Nov 3, 2022
bf359bb
guessing on corefoundation
alyssawilk Nov 3, 2022
ddb4149
suggested comment
alyssawilk Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 4 additions & 23 deletions BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,6 @@

licenses(["notice"])

config_setting(
name = "osx",
constraint_values = [
"@platforms//os:osx",
],
)

config_setting(
name = "ios",
constraint_values = [
"@platforms//os:ios",
],
)

### libraries

cc_library(
Expand Down Expand Up @@ -66,15 +52,10 @@ cc_library(
"include/cctz/zone_info_source.h",
],
includes = ["include"],
linkopts = select({
"//:osx": [
Comment thread
alyssawilk marked this conversation as resolved.
"-framework Foundation",
],
Comment on lines -70 to -72
Copy link
Copy Markdown
Contributor

@devbww devbww Nov 2, 2022

Choose a reason for hiding this comment

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

This was added in #102, which references abseil/abseil-cpp#291.

Do we understand why that was wrong, or what has changed?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 2, 2022

Choose a reason for hiding this comment

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

What I know is what is on the associated issue, which is this change broke absl for Envoy Mobile and tensorflow builds.
Edit: If the removal causes problems for anyone I can add a bazel select to make it either opt-in or opt-out.

Copy link
Copy Markdown
Contributor

@devjgm devjgm Nov 2, 2022

Choose a reason for hiding this comment

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

From reading the history of that other PR and issue that @devbww mentioned, I think the issue was that old versions of bazel didn't automatically link with -framework Foundation, and newer bazel's may do that automatically. I'm guessing that the bazel versions that we support today all implicitly link w/ Foundation, so I think removing these explicit deps may work OK.

That said, I'm not sure we actually should do that. Today, our code explicitly references macOS TimeZone types, for example:

cctz/src/time_zone_lookup.cc

Line 25 in c674330

#include <CoreFoundation/CFTimeZone.h>

and in the spirit of include link with what you use, cctz probably should be explicit about its dep on Foundation[1], IMO.

From abseil/abseil-cpp#326, it sounds like the real issue is w/ the native android build rules. @alyssawilk did you say that you have an alternative workaround that may allow you to fix your issue while allowing CCTZ to retain its explicit dep?

[1]: We're currently linking with Foundation, but we actually only need to link with CoreFoundation (it's a lower-level library). So this dep should probably actually be -framework CoreFoundation... @alyssawilk does making that change fix your issue (probably not; but we might as well check)

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 2, 2022

Choose a reason for hiding this comment

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

Our workaround is carrying an abseil.patch file* which removes the foundation files (which looks like what everyone else is doing). I'm now trying to get rid of that patch file, either by removing the explicit dependency or making it optional because the code is switching repos and I'm trying to clean up all the backported patches that I can. Worst case I can probably move the backported patch but I'd rather find a fix we're all Ok with, whether it be removing the dependency as outdated and known problematic, or having an opt-in bazel select. I'm happy to do whatever works best for you folks but I'd really prefer we fix abseil rather than expect everyone downstream to patch!

https://github.com/envoyproxy/envoy-mobile/blob/main/bazel/abseil.patch

Copy link
Copy Markdown
Contributor

@devjgm devjgm Nov 2, 2022

Choose a reason for hiding this comment

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

I agree that it would be good to avoid the need for everyone (anyone) to have to apply a patch in order to use abseil/cctz w/ an android build target (when compiling on macos).

I think I'd be OK with replacing the -framework Foundation explicit dep with a comment stating that we actually do need the Foundation dep, but it's added implicitly by Bazel and not listing it explicitly works around a bug in the android native deps (and link to that issue). That's basically this PR but with an additional comment.

@devbww WDYT?

@derekmauro can you confirm whether this PR, when patched in to Abseil, will pass all of Abseil's tests?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

SG thanks! I'f we're doing that as general clean up I'm inclined to do it for both, not just for the dependency my project needs. I'm very happy to revert that half if y'all prefer but I'll start with that so @derekmauro 's CI run tests both removals and optimistically hope the other ccd folks buy in on this plan.

Copy link
Copy Markdown
Member

@derekmauro derekmauro Nov 3, 2022

Choose a reason for hiding this comment

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

This passes CI: abseil/abseil-cpp#1306

Copy link
Copy Markdown
Contributor

@devbww devbww Nov 3, 2022

Choose a reason for hiding this comment

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

Forgive my naivety, but if I'm understanding correctly bazel now adds -framework Foundation, so we don't have to, even though we would prefer to (or at least CoreFoundation).

And not adding it avoids this: "the problem is when you're trying to build android libraries on a mac machine it gets confused about platforms."

That seems like a problem far removed from this change, and not even cctz specific. Is there an alternative that targets the real confusion?

"//:ios": [
"-framework Foundation",
],
"//conditions:default": [],
}),
# OS X and iOS no longer use `linkopts = ["-framework CoreFoundation"]`
# as (1) bazel adds it automatically, and (2) it caused problems when
# cross-compiling for Android.
# See https://github.com/abseil/abseil-cpp/issues/326 for details.
visibility = ["//visibility:public"],
deps = [":civil_time"],
)
Expand Down