Change the default of objc_enable_binary_stripping to "true".

[rahul-malik]

Enabling this flag greatly reduces the size of an iOS application and it is expected that release builds (compilation_mode opt) would have had this enabled by default.

In our experience we observed a 50% reduction in binary size and since this only takes effect when --compilation_mode=opt is enabled it should be harmless for debug / fastbuild configurations.

[bazel-io]

Can one of the admins verify this patch?

[rahul-malik]

This PR addresses the corresponding issue: #3731

[c-parsons]

There are unfortunate implications with this flip, namely that dynamic frameworks are unlikely to build "correctly". Dynamic frameworks expose symbols that would otherwise get stripped away..

We may need to be extra careful to handle dynamic frameworks carefully.

(It might make the most sense to determine binary stripping on a target-by-target basis, as a result, instead of globally with a flag.)

[rahul-malik]

@c-parsons - I thought this flag only applied to statically linked binaries? Dynamic frameworks should be omitted IMO.

[c-parsons]

I'm concerned about statically linked binaries that are used as a dylib for other binaries, if that makes sense.

If I have an IOS application MyApplication that points to a dynamic framework MyFramework (and I'm building both from source and linking them myself), then specifying --objc_enable_binary_stripping would make MyFramework's binary stripped of symbols that MyApplication may depend on.

[rahul-malik]

Whether to perform symbol and dead-code strippings on linked binaries. Binary strippings will be performed if both this flag and --compilationMode=opt are specified

From the docs it appears that this strips dead code paths / symbols. If a framework depended on certain symbols I would assume they are not affected by this flag. Do we have a test example of the concern?

[rahul-malik]

Side note: If you're linking them yourself isn't that out of the scope of Bazel? I would assume we're trying to solve for users building and linking entirely within Bazel.

[c-parsons]

In my example above, MyFramework, at link time, may have "dead code paths" because we do not force the user to declare framework entry points. So most of MyFramework may be stripped away.

The problem is that at link time for a dylib, you don't know what your dependers expect your available entry points to be. Yes, we should maybe make this explicitly declared for building frameworks, or again, restrict binary stripping to apply only to "binaries which have a main()", but we don't do either of these things today.

Doing cross-binary dead code analysis is not something we support today.

As for test cases, unfortunately we have poor open-sourced ios_framework test coverage at this time, but I've verified that setting this flag does break a number of our internal tests around this rule.
(Indeed, what happens is that symbols expected in the ios_framework by its depending ios_application are dropped from the framework)

[rahul-malik]

@c-parsons - Got it, what changes do you think would be necessary to add to this PR to make this change?

[c-parsons]

IMO, objc_enable_binary_stripping should not be a global flag, and should instead be toggled as an attribute on binary rules, such as apple_binary.
Or, alternatively (but less nicely), we special-case apple_binary targets of type dylib so that they are not subject to the settings of objc_enable_binary_stripping.

[rahul-malik]

I would agree but the direct usage of apple_binary seems to be discouraged in rules_apple.

https://github.com/bazelbuild/rules_apple/wiki/Migrating-from-the-native-rules#do-not-use-objc_binary

[c-parsons]

objc_enable_binary_stripping can be forwarded through the bzl (skylark) macro that creates it, for instance, ios_application. I am not suggesting users define their own apple_binary targets.

[rahul-malik]

So it sounds like the change would need to expose a way to strip binaries on apple_binary / objc_binary rather than a global flag and then have a corresponding PR in rules_apple to specify that setting?

[c-parsons]

Correct.

[rahul-malik]

Follow up, should we remove the global flag entirely if it's this dangerous?

[c-parsons]

We should provide the alternative functionality before we deprecate and remove the global flag. (But then, I'm all for removing the global flag.)

[damienmg]

(Please close the PR if it is no longer relevant)

[rahul-malik]

I'm going to look into these changes over the weekend and see if I can make these changes. Happy to open a new PR if that makes things easier

[c-parsons]

As discussed, objc_enable_binary_stripping should be an attribute instead of a global flag, and that way frameworks can remain unstripped in larger builds.

[hlopko]

Friendly ping...

[bazel-io]

Can one of the admins verify this patch?

[rahul-malik]

@mhlopko - is the ping for me? Sorry haven't had a chance to work on this at the moment

[hlopko]

@rahul-malik If I understand this thread correctly you're the one holding the wheel now. It's ok if you didn't have time, just wanted to make sure that this pull request is not abandoned and/or is not blocked by bazel team. Thanks!

[buchgr]

@rahul-malik now I am the one pining 😄 . Is there still interested in this change?

[rahul-malik]

@buchgr - I would like to make this change but it's unlikely I'll get to it soon. Let's close for now and I'll resubmit when I get a chance to work on this