[ENG-6619] validate icon PNGs before running build #1477

dsokal · 2022-10-25T14:05:25Z

Checklist

I've added an entry to CHANGELOG.md if necessary. You can comment this pull request with /changelog-entry [breaking-change|new-feature|bug-fix|chore] [message] and CHANGELOG.md will be updated automatically.

Why

From https://linear.app/expo/issue/ENG-6619/validate-pngs-before-running-build:

"INVALID_PNG_SIGNATURE" error seems like it may be caused by the user providing an invalid png file for their app icon or splash screen. For managed apps, we should be able to validate this before running their build, making this much less frustrating and we can provide a nice error at that point in the process.

How

For Android, validate the following app config fields:

exp.icon
exp.android.icon
exp.android.adaptiveIcon.foregroundImage
exp.android.adaptiveIcon.backgroundImage
exp.splash.image
exp.notification.icon

For iOS:

exp.icon
exp.ios.icon
exp.splash.image
exp.notification.icon

plus check whether exp.ios.icon ?? ctx.exp.icon is not tranparent.

Test Plan

Tests.

linear · 2022-10-25T14:05:27Z

ENG-6619 Validate PNGs before running build

"INVALID_PNG_SIGNATURE" error seems like it may be caused by the user providing an invalid png file for their app icon or splash screen. For managed apps, we should be able to validate this before running their build, making this much less frustrating and we can provide a nice error at that point in the process. https://sentry.io/organizations/expoio/issues/3596287951/?project=1837720&query=is%3Aunresolved

github-actions · 2022-10-25T14:09:36Z

Size Change: +170 kB (0%)

Total Size: 40.3 MB

Filename	Size	Change
`./packages/eas-cli/dist/eas-linux-x64.tar.gz`	40.3 MB	+170 kB (0%)

_{compressed-size-action}

szdziedzic · 2022-10-25T14:22:32Z

packages/eas-cli/src/build/validate.ts

+  if (!foregroundImage.endsWith('.png')) {
+    Log.error(`The Android Adaptive Icon foreground image must be a PNG file.`);
+    Log.error(`expo.android.adaptiveIcon.foregroundImage = ${chalk.bold(foregroundImage)}`);
+    Errors.exit(1);
+  }


We are quickly checking the Android icon using endsWith method. Do we want to do the same for the iOS icon?

codecov · 2022-10-26T08:42:00Z

Codecov Report

Merging #1477 (6c23ff5) into main (fa704c4) will increase coverage by 0.26%.
The diff coverage is 90.30%.

@@            Coverage Diff             @@
##             main    #1477      +/-   ##
==========================================
+ Coverage   51.16%   51.42%   +0.26%     
==========================================
  Files         450      451       +1     
  Lines       15483    15584     +101     
  Branches     3041     3064      +23     
==========================================
+ Hits         7921     8013      +92     
- Misses       7549     7558       +9     
  Partials       13       13

Impacted Files	Coverage Δ
packages/eas-cli/src/build/android/build.ts	`29.86% <50.00%> (-0.45%)`	⬇️
packages/eas-cli/src/build/ios/build.ts	`35.56% <50.00%> (-0.80%)`	⬇️
packages/eas-cli/src/utils/image.ts	`88.53% <88.53%> (ø)`
packages/eas-cli/src/build/validate.ts	`66.67% <97.37%> (+41.67%)`	⬆️
packages/eas-cli/src/fetch.ts	`94.45% <0.00%> (+5.56%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

wkozyra95 · 2022-10-26T10:59:17Z

packages/eas-cli/src/build/validate.ts

+}
+
+async function validateAndroidIconAsync(ctx: CommonContext<Platform.ANDROID>): Promise<void> {
+  if (!ctx.exp.android?.adaptiveIcon?.foregroundImage) {


the linear issue you linked is talking about the icon and splash screen, can you clarify why are you checking the adaptive icon foreground image(and why only that image)?

packages/eas-cli/src/build/validate.ts

EvanBacon · 2022-10-26T19:49:53Z

This seems like a bug in Prebuild. The icon generation step should be converting the inputs into compliant outputs. Also icon fields can be URLs and a variety of other image formats which will now suddenly not work with EAS Build after this change lands.

Potentially could be fixed here. Would be curious to learn more about why this is happening since we should be able to just automatically fix all images.

brentvatne · 2022-10-26T21:58:33Z

✅ the fields being validated for Android look correct to me and this should help a lot to prevent the ~30-40 failed builds per day that we see
⚠️ I believe it's not necessary to validate on iOS - I tried a couple builds and prebuild appears to be resilient against different file types. That said, if we don't have these validations in EAS CLI and an image is invalid, it would be annoying to spin up a job and get all the way to where the build fails in prebuild (or later) to give feedback.

Some follow up here for @byCedric / @EvanBacon on other CLI tooling:

expo doctor warns about non-PNG image files for icon fields, even those these are actually supported on iOS (even if we don't recommend them). We need to decide if we want to explicitly support non-PNG files and unify behavior.
Android icons should also be made similarly resilient in prebuild if we decide that we want to uniformly support non-PNG files, I made a task for this

brentvatne

let's comment out iOS validation for now - I think it's possible that we'll revisit it in the future. otherwise looks good to ship

dsokal · 2022-10-27T08:23:05Z

@EvanBacon

Also icon fields can be URLs and a variety of other image formats which will now suddenly not work with EAS Build after this change lands.

Can you elaborate? My code supports URLs. See https://github.com/expo/eas-cli/pull/1477/files#diff-e1c7866672b8c773c57f0c858398754872eebbf02a3ee3e8b238f45396efb312R72

This seems like a bug in Prebuild.

I don't know anything about Prebuild. This PR is based on the Linear task description. Happy to see this fixed in Prebuild.
Also, fixing this in prebuild doesn't prevent users from running redundant builds that will fail because of incorrect images.

@brentvatne

let's comment out iOS validation for now - I think it's possible that we'll revisit it in the future. otherwise looks good to ship

Sounds good!

⚠️ I believe it's not necessary to validate on iOS

What about transparency?

brentvatne · 2022-10-27T16:04:42Z

@dsokal

What about transparency?

We handle removing transparency automatically in prebuild: https://github.com/expo/expo/blob/15d2337a623a7921f68d4e800cf10cf38c95f051/packages/%40expo/prebuild-config/src/plugins/icons/withIosIcons.ts#L130-L137

I tested this and it worked as expected. Where we may still encounter problems here is if people have bare apps where they set the icon manually and it has transparency

brentvatne · 2022-10-27T18:33:13Z

@dsokal - one other suggestion - can we scope these validations to only projects that are using sdk 46 and lower? @EvanBacon has improved prebuild so it should be able to handle non-png icons and convert them to png.

EvanBacon · 2022-10-27T19:03:17Z

The feature looks good, but adding a gate to only apply on managed projects less than SDK 47 would be good.

dsokal · 2022-10-28T11:20:59Z

@brentvatne @EvanBacon can you please review the PR again?

dsokal requested review from brentvatne and szdziedzic October 25, 2022 14:05

dsokal requested a review from wkozyra95 as a code owner October 25, 2022 14:05

szdziedzic reviewed Oct 25, 2022

View reviewed changes

dsokal force-pushed the @dsokal/validate-app-icon branch from 0869fde to e94b3f7 Compare October 25, 2022 14:48

dsokal marked this pull request as draft October 26, 2022 07:41

dsokal force-pushed the @dsokal/validate-app-icon branch 2 times, most recently from 4c18c69 to 3c2a1a7 Compare October 26, 2022 08:36

dsokal force-pushed the @dsokal/validate-app-icon branch 2 times, most recently from 78ba92d to 2141884 Compare October 26, 2022 08:56

dsokal marked this pull request as ready for review October 26, 2022 08:59

wkozyra95 reviewed Oct 26, 2022

View reviewed changes

dsokal force-pushed the @dsokal/validate-app-icon branch from 7287991 to 2141884 Compare October 26, 2022 11:41

dsokal marked this pull request as draft October 26, 2022 11:44

dsokal force-pushed the @dsokal/validate-app-icon branch from eb493d7 to 4bfc26a Compare October 26, 2022 13:16

dsokal marked this pull request as ready for review October 26, 2022 13:18

dsokal requested review from szdziedzic and wkozyra95 October 26, 2022 13:18

dsokal force-pushed the @dsokal/validate-app-icon branch from 4bfc26a to 0f21407 Compare October 26, 2022 13:24

wkozyra95 approved these changes Oct 26, 2022

View reviewed changes

szdziedzic approved these changes Oct 26, 2022

View reviewed changes

brentvatne approved these changes Oct 26, 2022

View reviewed changes

EvanBacon mentioned this pull request Oct 26, 2022

fix(image-utils): make jimp export buffers as png like sharp expo/expo-cli#4576

Merged

dsokal force-pushed the @dsokal/validate-app-icon branch from 0f21407 to 65da104 Compare October 27, 2022 08:57

dsokal requested review from brentvatne and wkozyra95 October 27, 2022 08:57

dsokal force-pushed the @dsokal/validate-app-icon branch from 65da104 to b3e7120 Compare October 27, 2022 08:58

wkozyra95 approved these changes Oct 27, 2022

View reviewed changes

szdziedzic approved these changes Oct 27, 2022

View reviewed changes

dsokal added 4 commits October 28, 2022 13:08

validate icon PNGs before running build

6a40a73

address PR feedback

5d3d105

address PR feedback

83243db

address more PR feedback

6c23ff5

dsokal requested review from wkozyra95 and szdziedzic October 28, 2022 11:20

dsokal force-pushed the @dsokal/validate-app-icon branch from 81ab748 to 6c23ff5 Compare October 28, 2022 11:20

wkozyra95 approved these changes Oct 28, 2022

View reviewed changes

brentvatne approved these changes Nov 2, 2022

View reviewed changes

dsokal merged commit 6270bc1 into main Nov 2, 2022

dsokal deleted the @dsokal/validate-app-icon branch November 2, 2022 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-6619] validate icon PNGs before running build #1477

[ENG-6619] validate icon PNGs before running build #1477

dsokal commented Oct 25, 2022 •

edited

Loading

linear bot commented Oct 25, 2022

github-actions bot commented Oct 25, 2022 •

edited

Loading

szdziedzic Oct 25, 2022

codecov bot commented Oct 26, 2022 •

edited

Loading

wkozyra95 Oct 26, 2022

EvanBacon commented Oct 26, 2022 •

edited

Loading

brentvatne commented Oct 26, 2022

brentvatne left a comment

dsokal commented Oct 27, 2022 •

edited

Loading

brentvatne commented Oct 27, 2022

brentvatne commented Oct 27, 2022

EvanBacon commented Oct 27, 2022

dsokal commented Oct 28, 2022

[ENG-6619] validate icon PNGs before running build #1477

[ENG-6619] validate icon PNGs before running build #1477

Conversation

dsokal commented Oct 25, 2022 • edited Loading

Checklist

Why

How

Test Plan

linear bot commented Oct 25, 2022

github-actions bot commented Oct 25, 2022 • edited Loading

szdziedzic Oct 25, 2022

Choose a reason for hiding this comment

codecov bot commented Oct 26, 2022 • edited Loading

Codecov Report

wkozyra95 Oct 26, 2022

Choose a reason for hiding this comment

EvanBacon commented Oct 26, 2022 • edited Loading

brentvatne commented Oct 26, 2022

brentvatne left a comment

Choose a reason for hiding this comment

dsokal commented Oct 27, 2022 • edited Loading

brentvatne commented Oct 27, 2022

brentvatne commented Oct 27, 2022

EvanBacon commented Oct 27, 2022

dsokal commented Oct 28, 2022

dsokal commented Oct 25, 2022 •

edited

Loading

github-actions bot commented Oct 25, 2022 •

edited

Loading

codecov bot commented Oct 26, 2022 •

edited

Loading

EvanBacon commented Oct 26, 2022 •

edited

Loading

dsokal commented Oct 27, 2022 •

edited

Loading