bazelbuild / rules_apple

Bazel rules to build apps for Apple platforms.
Apache License 2.0
512 stars 269 forks source link

Don't use `pngcrush` on macOS #2361

Closed maxbelanger closed 9 months ago

maxbelanger commented 10 months ago

Xcode uses the copypng script to post-process PNGs on most platforms, but not on macOS. This is enabled incorrectly on macOS, which can make PNGs appear "corrupt" to some libraries on this platform as the use of Foundation is not guaranteed. This fixes this by making the behavior match that of copypng, replacing pngcrush in favor of a simple copy (via a symlink action) on macOS.

thii commented 10 months ago

I looked at how Xcode does it, and looks like it doesn't compress the PNG files if the target platform is macOS, and just copies the files. There are two build settings that control this: COMPRESS_PNG_FILES and STRIP_PNG_TEXT, and they default to NO for macOS.

The logic in copypng:

if ( $compress ) {
    @args = ( $PNGCRUSH, "-q", "-iphone", "-f", "0" );
    if ( $stripPNGText ) {
        push ( @args, "-rem", "text" );
    }
    push ( @args, $SRCFILE, $DSTFILE );

} else {
    @args = ( "cp", "$SRCFILE", "$DSTFILE" );
}
thii commented 10 months ago

So this should be a symlink action if the platform type is macOS.

keith commented 10 months ago

fwiw it might have been google was doing this intentionally differently from xcode just for the potential size win

maxbelanger commented 10 months ago

@keith happy to make this controllable in some way, maybe a feature to force this to happen on macOS? I'm not sure how to fully replicate the Xcode behavior here though (different defaults on each platform).

maxbelanger commented 10 months ago

Alternatively, using a define could also work and would allow us to express the default as False on macOS and True on other platforms. The only drawback is that users would have to use the command-line or a .bazelrc to override the default. Any preference between the two approaches?

thii commented 10 months ago

It's preferable to use features (or Starlark flags) than define. features are more configurable. define is already considered legacy.

luispadron commented 10 months ago

@maxbelanger If you want to run buildifier to format and fix CI we can get this merged

maxbelanger commented 9 months ago

Could you add the new feature flag to: https://github.com/bazelbuild/rules_apple/blob/master/doc/common_info.md

Added a reference to this in doc/resources.md, where the functionality to compress PNGs is already discussed.