Fix fcm dependency #3533

VinayGuthal · 2022-03-15T05:10:06Z

No description provided.

…w integrated

google-oss-bot · 2022-03-15T05:24:17Z

Size Report ¹

Affected Products

base
Type Base (e324046) Merge (215d9b4) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.65 kB ? (?)
firebase-annotations
Type Base (e324046) Merge (215d9b4) Diff
apk (aggressive) ? 8.39 kB ? (?)
apk (release) ? 8.88 kB ? (?)
firebase-common
Type Base (e324046) Merge (215d9b4) Diff
aar ? 48.0 kB ? (?)
apk (aggressive) ? 84.2 kB ? (?)
apk (release) ? 680 kB ? (?)
firebase-components
Type Base (e324046) Merge (215d9b4) Diff
aar ? 41.4 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 29.5 kB ? (?)
firebase-datatransport
Type Base (e324046) Merge (215d9b4) Diff
aar ? 5.12 kB ? (?)
apk (aggressive) ? 130 kB ? (?)
apk (release) ? 767 kB ? (?)
firebase-encoders
Type Base (e324046) Merge (215d9b4) Diff
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 15.3 kB ? (?)
firebase-encoders-json
Type Base (e324046) Merge (215d9b4) Diff
aar ? 10.5 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 20.1 kB ? (?)
firebase-encoders-proto
Type Base (e324046) Merge (215d9b4) Diff
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 21.6 kB ? (?)
firebase-installations
Type Base (e324046) Merge (215d9b4) Diff
aar ? 55.0 kB ? (?)
apk (aggressive) ? 85.8 kB ? (?)
apk (release) ? 702 kB ? (?)
firebase-installations-interop
Type Base (e324046) Merge (215d9b4) Diff
aar ? 8.34 kB ? (?)
apk (aggressive) ? 65.0 kB ? (?)
apk (release) ? 651 kB ? (?)
firebase-messaging
Type Base (e324046) Merge (215d9b4) Diff
aar ? 225 kB ? (?)
apk (aggressive) ? 504 kB ? (?)
apk (release) ? 1.22 MB ? (?)
transport-api
Type Base (e324046) Merge (215d9b4) Diff
aar ? 6.57 kB ? (?)
apk (aggressive) ? 8.68 kB ? (?)
apk (release) ? 14.9 kB ? (?)
transport-backend-cct
Type Base (e324046) Merge (215d9b4) Diff
aar ? 53.5 kB ? (?)
apk (aggressive) ? 58.0 kB ? (?)
apk (release) ? 105 kB ? (?)
transport-runtime
Type Base (e324046) Merge (215d9b4) Diff
aar ? 178 kB ? (?)
apk (aggressive) ? 43.8 kB ? (?)
apk (release) ? 82.6 kB ? (?)

Test Logs

Merge (215d9b4): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3533/binary-size/1503766095746568192

Notes

Commit (215d9b4) is created by Prow via merging PR base commit (e324046) and head commit (e79557d).

https://storage.googleapis.com/firebase-sdk-metric-reports/evGWtZLzdn.html

google-oss-bot · 2022-03-15T05:25:55Z

Coverage Report ¹

Affected Products

firebase-messaging
Overall coverage changed from 85.70% (e324046) to 84.82% (215d9b4) by -0.88%.
Filename Base (e324046) Merge (215d9b4) Diff
ByteStreams.java ? 59.72% ?
MessagingAnalytics.java 83.00% 81.38% -1.62%

Test Logs

Notes

Commit (215d9b4) is created by Prow via merging PR base commit (e324046) and head commit (e79557d).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/rrDyWrNDSN.html

mikehardy

Nice - I'm only here because I'm the original poster of #3526 - with this change I cannot imagine the google api-client dependency leaking in to the published artifact anymore, thus I cannot imagine the httpcomponents dependencies transitively leaking in either. Should do the trick. I recognize my approval doesn't carry weight but symbolically, 😄 👍

gsakakihara

It looks like this may need to have verifyGoogleJavaFormat run to fix the test failures. For the annotation changes, I think you might also be able to fix it by adding
compileOnly 'com.google.code.findbugs:jsr305:3.0.2' instead of switching them, though I haven't tested it out and I don't know which one is more preferred.

rlazo · 2022-03-15T14:47:57Z

firebase-messaging/src/main/java/com/google/firebase/messaging/ImageDownload.java

+      byte[] data = new byte[MAX_IMAGE_SIZE_BYTES + 1];
+
+      while ((nRead = connectionInputStream.read(data, 0, data.length)) != -1) {
+        buffer.write(data, 0, nRead);


This can end up reading several chunks of up to MAX_IMAGE_SIZE_BYTES+1, which is not equivalent to ByteStreams.limit. If you check @ ByteStreams implementation it limits the total value read to be up to MAX_IMAGE_SIZE_BYTES+1 with a counter. You would need to implement something similar.

i think it will be equivalent. bcz the reading is not in the for loop and ByteStreams.limit(connectionInputStream, MAX_IMAGE_SIZE_BYTES + 1)); is getting initialized everytime a function is called so the limit will be set everytime not once

gsakakihara

Is there a way to add a previous dependency to continue to use ByteStreams? It used to work without bringing in google-api-client.

VinayGuthal · 2022-03-15T15:21:09Z

Is there a way to add a previous dependency to continue to use ByteStreams? It used to work without bringing in google-api-client.

@gsakakihara this would introduce a dependency on guava and would be a big regression in terms of size..Guava was vendored in before so it was fine. However now it will cause a lot of bloating

gsakakihara · 2022-03-15T15:34:07Z

Is there a way to vendor it in now? Otherwise it seems like that section will need to be fixed since it is definitely a change in behavior.

Also compileOnly 'com.google.code.findbugs:jsr305:3.0.2' doesn't work, but:

implementation 'com.google.errorprone:error_prone_annotations:2.11.0'
implementation 'com.google.code.findbugs:jsr305:3.0.2'

does seem to work, but I'm not sure what other effects on size and such that would have.

VinayGuthal · 2022-03-15T15:42:03Z

Is there a way to vendor it in now? Otherwise it seems like that section will need to be fixed since it is definitely a change in behavior.

Also compileOnly 'com.google.code.findbugs:jsr305:3.0.2' doesn't work, but:

implementation 'com.google.errorprone:error_prone_annotations:2.11.0' implementation 'com.google.code.findbugs:jsr305:3.0.2'

does seem to work, but I'm not sure what other effects on size and such that would have.

I think it should fairly be similar between androidx and javax (especially since this is android sdk androidx might be preferred). However if you want to keep it back to javax, i can add implementation 'com.google.code.findbugs:jsr305:3.0.2' to the gradle and revert the annotation changes

VinayGuthal · 2022-03-15T16:09:47Z

Is there a way to vendor it in now? Otherwise it seems like that section will need to be fixed since it is definitely a change in behavior.

Also compileOnly 'com.google.code.findbugs:jsr305:3.0.2' doesn't work, but:

implementation 'com.google.errorprone:error_prone_annotations:2.11.0' implementation 'com.google.code.findbugs:jsr305:3.0.2'

does seem to work, but I'm not sure what other effects on size and such that would have.

Vendored in

VinayGuthal · 2022-03-15T16:14:16Z

Nice - I'm only here because I'm the original poster of #3526 - with this change I cannot imagine the google api-client dependency leaking in to the published artifact anymore, thus I cannot imagine the httpcomponents dependencies transitively leaking in either. Should do the trick. I recognize my approval doesn't carry weight but symbolically, 😄 👍

Thanks a lot @mikehardy for the review :) Your approval does carry weight!! Also thanks for pointing out this issue allowing us to fix this!

google-oss-bot · 2022-03-15T16:52:01Z

@VinayGuthal: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests	`e79557d`	link	`/test smoke-tests`
device-check-changed	`e79557d`	link	`/test device-check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

VinayGuthal added 3 commits March 10, 2022 23:26

declare app check as heartbeat consumer since app check backend is no…

0b676af

…w integrated

fix tests

17fb70e

update

81e3f78

google-oss-bot added the size/M label Mar 15, 2022

revert heartbeat changes

4db75ba

gJF

ed3f3e7

argzdev mentioned this pull request Mar 15, 2022

Messaging fails build with latest 29.2.0 release, 29.1.0 is fine [solved, 29.2.1 works] #3526

Closed

mikehardy approved these changes Mar 15, 2022

View reviewed changes

gsakakihara self-requested a review March 15, 2022 14:08

gsakakihara approved these changes Mar 15, 2022

View reviewed changes

rlazo reviewed Mar 15, 2022

View reviewed changes

gsakakihara requested changes Mar 15, 2022

View reviewed changes

gJF

c36f795

vendor dependency

68a6fa2

google-oss-bot added size/L and removed size/M labels Mar 15, 2022

make package private

e79557d

gsakakihara approved these changes Mar 15, 2022

View reviewed changes

russellwheatley mentioned this pull request Mar 15, 2022

feat: bump Firebase Android SDK to 29.3.0 firebase/flutterfire#8283

Merged

10 tasks

rlazo approved these changes Mar 15, 2022

View reviewed changes

VinayGuthal merged commit 87ae78e into master Mar 15, 2022

VinayGuthal deleted the fix_fcm_dependency branch March 15, 2022 16:43

firebase locked and limited conversation to collaborators Apr 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fcm dependency #3533

Fix fcm dependency #3533

VinayGuthal commented Mar 15, 2022

google-oss-bot commented Mar 15, 2022 •

edited

`base`

`firebase-annotations`

`firebase-common`

`firebase-components`

`firebase-datatransport`

`firebase-encoders`

`firebase-encoders-json`

`firebase-encoders-proto`

`firebase-installations`

`firebase-installations-interop`

`firebase-messaging`

`transport-api`

`transport-backend-cct`

`transport-runtime`

google-oss-bot commented Mar 15, 2022 •

edited

`firebase-messaging`

mikehardy left a comment

gsakakihara left a comment

rlazo Mar 15, 2022

VinayGuthal Mar 15, 2022

gsakakihara left a comment

VinayGuthal commented Mar 15, 2022

gsakakihara commented Mar 15, 2022

VinayGuthal commented Mar 15, 2022

VinayGuthal commented Mar 15, 2022

VinayGuthal commented Mar 15, 2022

google-oss-bot commented Mar 15, 2022

Fix fcm dependency #3533

Fix fcm dependency #3533

Conversation

VinayGuthal commented Mar 15, 2022

google-oss-bot commented Mar 15, 2022 • edited

Size Report 1

Affected Products

base

firebase-annotations

firebase-common

firebase-components

firebase-datatransport

firebase-encoders

firebase-encoders-json

firebase-encoders-proto

firebase-installations

firebase-installations-interop

firebase-messaging

transport-api

transport-backend-cct

transport-runtime

Test Logs

Notes

google-oss-bot commented Mar 15, 2022 • edited

Coverage Report 1

Affected Products

firebase-messaging

Test Logs

Notes

mikehardy left a comment

Choose a reason for hiding this comment

gsakakihara left a comment

Choose a reason for hiding this comment

rlazo Mar 15, 2022

Choose a reason for hiding this comment

VinayGuthal Mar 15, 2022

Choose a reason for hiding this comment

gsakakihara left a comment

Choose a reason for hiding this comment

VinayGuthal commented Mar 15, 2022

gsakakihara commented Mar 15, 2022

VinayGuthal commented Mar 15, 2022

VinayGuthal commented Mar 15, 2022

VinayGuthal commented Mar 15, 2022

google-oss-bot commented Mar 15, 2022

google-oss-bot commented Mar 15, 2022 •

edited

Size Report ¹

`base`

`firebase-annotations`

`firebase-common`

`firebase-components`

`firebase-datatransport`

`firebase-encoders`

`firebase-encoders-json`

`firebase-encoders-proto`

`firebase-installations`

`firebase-installations-interop`

`firebase-messaging`

`transport-api`

`transport-backend-cct`

`transport-runtime`

google-oss-bot commented Mar 15, 2022 •

edited

Coverage Report ¹

`firebase-messaging`