Skip to content

add coroutines-play-services as a transitive dep to firebase-common-ktx #4044

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Sep 23, 2022
Merged

add coroutines-play-services as a transitive dep to firebase-common-ktx #4044

merged 7 commits into from
Sep 23, 2022

Conversation

thatfiredev
Copy link
Member

@thatfiredev thatfiredev commented Aug 30, 2022 •
edited
Loading

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2022 •
edited
Loading

Unit Test Results

   395 files  +  5     395 suites  +5   18m 44s ⏱️ +43s
4 715 tests +15  4 693 ✔️ +17  22 💤 ±0  0 ❌  - 2 
4 731 runs  +15  4 709 ✔️ +17  22 💤 ±0  0 ❌  - 2 

Results for commit 095f048. ± Comparison against base commit ed10eb5.

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 30, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-appcheck-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)349 kB356 kB+7.56 kB (+2.2%)
    apk (release)1.54 MB1.92 MB+378 kB (+24.6%)

  • firebase-appdistribution-api-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)102 kB110 kB+7.56 kB (+7.4%)
    apk (release)1.25 MB1.63 MB+376 kB (+30.1%)

  • firebase-common-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)101 kB109 kB+7.56 kB (+7.5%)
    apk (release)1.24 MB1.62 MB+375 kB (+30.2%)

  • firebase-config-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)115 kB118 kB+3.46 kB (+3.0%)
    apk (release)1.30 MB1.67 MB+376 kB (+29.0%)

  • firebase-crashlytics-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)233 kB237 kB+3.46 kB (+1.5%)
    apk (release)1.45 MB1.83 MB+376 kB (+25.9%)

  • firebase-database-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)351 kB354 kB+3.47 kB (+1.0%)
    apk (release)1.70 MB2.08 MB+378 kB (+22.3%)

  • firebase-dynamic-links-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)351 kB355 kB+3.47 kB (+1.0%)
    apk (release)1.54 MB1.92 MB+376 kB (+24.4%)

  • firebase-firestore-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)512 kB512 kB+814 B (+0.2%)
    apk (release)4.26 MB4.26 MB+4.22 kB (+0.1%)

  • firebase-functions-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)415 kB418 kB+3.47 kB (+0.8%)
    apk (release)1.77 MB2.15 MB+376 kB (+21.3%)

  • firebase-inappmessaging-display-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)1.52 MB1.52 MB+3.46 kB (+0.2%)
    apk (release)5.17 MB5.55 MB+374 kB (+7.2%)

  • firebase-inappmessaging-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)681 kB684 kB+3.47 kB (+0.5%)
    apk (release)3.92 MB4.29 MB+376 kB (+9.6%)

  • firebase-installations-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)103 kB111 kB+7.56 kB (+7.3%)
    apk (release)1.27 MB1.64 MB+374 kB (+29.5%)

  • firebase-messaging-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)456 kB460 kB+3.46 kB (+0.8%)
    apk (release)1.70 MB2.08 MB+379 kB (+22.3%)

  • firebase-ml-modeldownloader-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)160 kB163 kB+3.46 kB (+2.2%)
    apk (release)1.38 MB1.76 MB+378 kB (+27.3%)

  • firebase-perf-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)1.05 MB1.05 MB+3.47 kB (+0.3%)
    apk (release)3.03 MB3.41 MB+377 kB (+12.4%)

  • firebase-storage-ktx

    TypeBase (ed10eb5)Merge (d77b846)Diff
    apk (aggressive)350 kB354 kB+3.46 kB (+1.0%)
    apk (release)1.57 MB1.95 MB+377 kB (+24.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/2YdE3XekrE.html

@thatfiredev
Copy link
Member Author

thatfiredev commented Aug 30, 2022 •
edited
Loading

I'm slightly concerned with the increased size shown in the bot report above. Seems like adding the library as a transitive dependency adds ~390kB to every ktx module - to me it is a big bump for such a small addition.
I'll check if we can expose only the .await() function instead of the whole library to see if it makes any difference in terms of size.

Update: No size difference, probably because we're including the library anyway. I'll proceed with the size bump, I guess that's the price to pay to get coroutines support out of the box.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 30, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-database

    Overall coverage changed from 50.17% (ed10eb5) to 50.15% (d77b846) by -0.02%.

    FilenameBase (ed10eb5)Merge (d77b846)Diff
    DoubleNode.java100.00%88.24%-11.76%

  • firebase-firestore

    Overall coverage changed from 44.25% (ed10eb5) to 44.26% (d77b846) by +0.00%.

    FilenameBase (ed10eb5)Merge (d77b846)Diff
    DeleteMutation.java90.48%95.24%+4.76%

Test Logs

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

@thatfiredev thatfiredev marked this pull request as ready for review September 9, 2022 15:50
@thatfiredev thatfiredev changed the title [WIP] add coroutines-play-services as a transitive dep to firebase-common-ktx add coroutines-play-services as a transitive dep to firebase-common-ktx Sep 9, 2022
@thatfiredev
Copy link
Member Author

thatfiredev commented Sep 13, 2022 •
edited
Loading

I'm holding this until #3974 gets merged. I think it would be better if we release with the latest version of Kotlin + latest version of kotlinx-coroutines-play-services .

@thatfiredev
Merge branch 'master' of https://github.com/firebase/firebase-android…
e044828 
…-sdk into rpf-add-coroutines-common-ktx

� Conflicts:
�	build.gradle
@thatfiredev
Copy link
Member Author

@vkryachko Can I merge this?

Also since it bumps firebase-common, does it mean we'll have to release all SDKs? Or only the ones that have methods returning Tasks?
But since it's a fireconf launch, I guess releasing them all wouldn't be so bad.

@vkryachko
Copy link
Member

vkryachko commented Sep 22, 2022 •
edited
Loading

Sure, go ahead and merge. Yeah, we will have to release everyone with tasks. Can you coordinate with @VinayGuthal on the release?

@thatfiredev
Copy link
Member Author

@vkryachko will do. Thanks!

@thatfiredev thatfiredev removed the request for review from rlazo September 22, 2022 14:09
@thatfiredev
Copy link
Member Author

@vkryachko GitHub won't let me merge this, it says "Required statuses must pass before merging", I'm not sure why.

@vkryachko
Copy link
Member

CI Tests / Unit Tests (:firebase-common) (pull_request) Failing after 4m

You should click "Rerun failed jobs" inside the failed workflow

@thatfiredev
Copy link
Member Author

@vkryachko I checked the logs and noticed that the failing check was the flaky test which got fixed recently. I just had to sync with master, thanks for pointing me in the right direction.

@thatfiredev thatfiredev merged commit 4f24be3 into master Sep 23, 2022
@thatfiredev thatfiredev deleted the rpf-add-coroutines-common-ktx branch September 23, 2022 00:45
@thatfiredev thatfiredev mentioned this pull request Sep 23, 2022
Merged
@firebase firebase locked and limited conversation to collaborators Oct 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vkryachko vkryachko vkryachko approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thatfiredev @google-oss-bot @vkryachko