build: fix cross-platform android builds on OS X by alyssawilk · Pull Request #237 · google/cctz

alyssawilk · 2022-11-02T13:12:05Z

Fixing abseil/abseil-cpp#326 by removing a non-working Foundation linkopt for OS X and iOS.
Foundation should now be implicitly added by modern versions of bazel so this will hopefully not be problematic for builds previously affected by abseil/abseil-cpp#291

This change is

co-authored-by: devbww

devbww

Perhaps the PR title could be more descriptive of the actual change.

devbww · 2022-11-02T16:20:46Z

BUILD

-        "//:osx": [
-            "-framework Foundation",
-        ],


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

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

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.

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)

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

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?

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.

This passes CI: abseil/abseil-cpp#1306

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?

BUILD

devjgm

Thanks for the PR, Alyssa. Can you tell me how to repro the error you're seeing? I just checked out cctz on my Mac and built it w/ bazel test ... and it worked OK. What do I need to do to see the error that's being fixed here?

alyssawilk · 2022-11-02T20:12:14Z

I think the attached issue explains better than I can - the problem is when you're trying to build android libraries on a mac machine it gets confused about platforms. You can repro if you check out and build envoy mobile (or tensorflow but I can't help with that one) on a mac machine without the absl patch. I'm happy to walk through repro instructions if you'd be game for trying it, or I can add a custom select so that we (and other folks who ran into the same issue) can work around the issue locally.

devjgm · 2022-11-03T16:02:52Z

This PR LGTM. @devbww, thoughts?

devbww

Perhaps the PR title could be more descriptive of the actual change.

Sorry for the confusion, but my concern was mostly with the title (first line), not the body. It should try to summarize the actual effect.

devbww · 2022-11-03T15:42:29Z

BUILD

-        "//:osx": [
-            "-framework Foundation",
-        ],


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?

BUILD

devbww · 2022-11-03T15:51:44Z

BUILD

+    # it is no longer explicitly added as a linkopt as bazel adds it
+    # automatically. See https://github.com/abseil/abseil-cpp/issues/326 for details.


"as bazel adds it automatically"

If that is the case, why doesn't the problem remain no matter what cctz does?

If bazel only does it in more limited circumstances, couldn't we use the same, limited circumstances (e.g., better discrimination between execution and target platforms)?

the way bazel adds it doesn't cause problems for android builds on macos as shown by commenting out this block upstream fixing everyone's builds
Given this fixes known bugs for multiple downstream customers and the only down-side is an implicit rather than explicit dependency, would it be possible to file a tracking issue for you folks to investigate what bazel does and add back a (working) explicit dependency? I don't know enough about bazel internals to know what they're doing and how it works but I know from again the multiple reports of breakage with explicit includes that there is a real problem I would like to fix, without the perfect solution stopping a known improvement.

devbww

Thanks.

I also removed "attempting to" from the PR title.

devbww · 2022-11-03T16:41:48Z

BUILD

+    # Note that OS X and iOS both have a dependency on CoreFoundation but it is
+    # no longer explicitly added as a linkopt as that causes problems
+    # cross-compiling android builds on OSX, and bazel will add it
+    # automatically.


OK, thanks. If you have the stamina for one more edit, though, I'd suggest ...

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

SGTM! I have no preference on phrasing so happy to land whatever works for you folks

This also allows us to remove the mobile override and its patch because the new version of abseil-cpp pulls in google/cctz#237. Fixes envoyproxy/envoy-mobile#136. Signed-off-by: JP Simard <jp@jpsim.com>

* build(deps): bump abseil-cpp to latest version This also allows us to remove the mobile override and its patch because the new version of abseil-cpp pulls in google/cctz#237. Fixes envoyproxy/envoy-mobile#136. Signed-off-by: JP Simard <jp@jpsim.com>

sluongng · 2023-04-10T05:11:43Z

Im a bit confused with this PR. I see many comments cited Bazel implicitly adding -framework flag to the link opts but I don't see any reference to that.
When searching for it via https://cs.opensource.google/bazel/bazel, all I see is that Bazel "may" do it for objective-c compilation and not for cc.

https://cs.opensource.google/search?q=%22-framework%22&sq=&ss=bazel%2Fbazel

My personal guess is that on MacOS, the default cc toolchain Bazel uses is the host's local cc toolchain (xcode clang). This toolchain is poorly configured and has leaks that may automatically include the framework where needed. However, when you switch to using a more hermetic, air-tight cc toolchain (such as https://github.com/uber/bazel-zig-cc), the missing of implicit -foundation flag is clear.

As suggested in envoyproxy/envoy#24782, this PR might be a bit rushed and android issue might be unrelated/tangent to the linkopts flag. I would urge folks to reconsider and perhaps, revert this PR.

devbww · 2023-04-10T05:22:34Z

@alyssawilk ... please comment. Thanks.

alyssawilk · 2023-04-10T12:58:26Z

Sorry would reverting the PR require everyone to maintain the workaround patches they used to? I'm not understand what changed to make the prior behavior less problematic than it was.

bdhess · 2023-04-12T05:42:48Z

I'm not sure what changed either, but there's definitely a problem here. I have a real program running on macOS 13 / M1 that crashes due to this issue. We don't see this issue in Kokoro which uses Intel Macs.

Termination Reason:    Namespace DYLD, Code 4 Symbol missing
missing symbol called
(terminated at launch; ignore backtrace)

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   dyld                          	       0x184b69118 __abort_with_payload + 8
1   dyld                          	       0x184b74d7c abort_with_payload_wrapper_internal + 104
2   dyld                          	       0x184b74db0 abort_with_payload + 16
3   dyld                          	       0x184b008a8 dyld4::halt(char const*) + 328
4   dyld                          	       0x184b36770 dyld4::APIs::_dyld_missing_symbol_abort() + 44
5   aes_gcm_test                  	       0x100e4d618 absl::time_internal::cctz::local_time_zone() + 36
6   aes_gcm_test                  	       0x100e2e188 absl::LocalTimeZone() + 16
7   aes_gcm_test                  	       0x100e2e14c absl::FormatTime(absl::Time) + 56
8   aes_gcm_test                  	       0x100d8bc2c grpc_core::StatusToString(absl::Status const&)::$_0::operator()(std::__1::basic_string_view<char, std::__1::char_traits<char>>, absl::Cord const&) const + 1228

Is it possible that the "default Bazel behavior" wasn't a Bazel behavior at all, but actually some default in Xcode?

This breaks toolchains that don't add the implicit Foundation linkopt. I can no longer reproduce this building envoy for Android so I'm not sure if we mis-diagnosed the issue, or if it was fixed another way. This reverts commit 5417654.

keith · 2023-07-17T18:32:05Z

I submitted a revert for this here #264

This breaks toolchains that don't add the implicit Foundation linkopt. I can no longer reproduce this building envoy for Android so I'm not sure if we mis-diagnosed the issue, or if it was fixed another way. This reverts commit 5417654.

build: attempting to fix abseil/abseil-cpp#326

3eaff4f

devbww added the kokoro:run label Nov 2, 2022

kokoro-team removed the kokoro:run label Nov 2, 2022

devbww reviewed Nov 2, 2022

View reviewed changes

review comments

555c92f

devbww added the kokoro:run label Nov 2, 2022

kokoro-team removed the kokoro:run label Nov 2, 2022

devjgm reviewed Nov 2, 2022

View reviewed changes

alyssawilk mentioned this pull request Nov 3, 2022

Foundation framework link option leaking into Android builds abseil/abseil-cpp#326

Closed

now with comments and full foundation removal

a793292

devbww reviewed Nov 3, 2022

View reviewed changes

devbww added the kokoro:run label Nov 3, 2022

kokoro-team removed the kokoro:run label Nov 3, 2022

trying to improve comment

c214363

alyssawilk changed the title ~~build: attempting to fix https://github.com/abseil/abseil-cpp/issues/326~~ build: attempting to fix cross-platform android builds on OS X Nov 3, 2022

guessing on corefoundation

bf359bb

devbww changed the title ~~build: attempting to fix cross-platform android builds on OS X~~ build: fix cross-platform android builds on OS X Nov 3, 2022

devbww approved these changes Nov 3, 2022

View reviewed changes

suggested comment

ddb4149

devbww added the kokoro:run label Nov 3, 2022

kokoro-team removed the kokoro:run label Nov 3, 2022

devbww merged commit 5417654 into google:master Nov 3, 2022

jpsim mentioned this pull request Dec 6, 2022

build(deps): bump abseil-cpp to latest version envoyproxy/envoy#24386

Merged

keith mentioned this pull request Jan 6, 2023

macOS build no longer works: symbol not found in flat namespace '_CFRelease' envoyproxy/envoy#24782

Closed

keith mentioned this pull request Jul 17, 2023

Revert "build: fix cross-platform android builds on OS X (#237)" #264

Merged

		# it is no longer explicitly added as a linkopt as bazel adds it
		# automatically. See https://github.com/abseil/abseil-cpp/issues/326 for details.

Conversation

alyssawilk commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devbww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devjgm left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devjgm commented Nov 3, 2022

Uh oh!

devbww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devbww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sluongng commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devbww commented Apr 10, 2023

Uh oh!

alyssawilk commented Apr 10, 2023

Uh oh!

bdhess commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keith commented Jul 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

alyssawilk commented Nov 2, 2022 •

edited

Loading

alyssawilk Nov 3, 2022 •

edited

Loading

alyssawilk commented Nov 2, 2022 •

edited

Loading

alyssawilk Nov 3, 2022 •

edited

Loading

sluongng commented Apr 10, 2023 •

edited

Loading

bdhess commented Apr 12, 2023 •

edited

Loading