Skip to content

Packaging prototype #5154

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.

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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Packaging prototype #5154

wants to merge 11 commits into from

Conversation

VinayGuthal
Copy link
Contributor

Move the ktx library inside of the main artifact for both firebase-common and firebase-functions. This prototype achieves the following things

  1. Firebase-common builds properly when Firebase.kt is copied from Firebase-common-ktx to firebase-common.
  2. firebase-functions builds propertly when Functions.kt is copied from firebase-functions-ktx to firebase-functions provided there is a dependency on the latest version of firebase-common (which has the new ktx)
  3. All sdks/app outside of firebase-common(or any sdk for that matter) can depend on and use Firebase.kt which is moved to the main artifact.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 11, 2023

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

@google-oss-bot
Copy link
Contributor

1 Warning
⚠️ Did you forget to add a changelog entry? (Add the 'no-changelog' label to the PR to silence this warning.)

Generated by 🚫 Danger

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 11, 2023

Coverage Report 1

Affected Products

  • firebase-common

    Overall coverage changed from ? (837979c) to 55.11% (26628cf) by ?.

    50 individual files with coverage change

    FilenameBase (837979c)Merge (26628cf)Diff
    AutoValue_HeartBeatResult.java?33.33%?
    AutoValue_LibraryVersion.java?58.33%?
    AutoValue_SdkHeartBeatResult.java?0.00%?
    AutoValue_StartupTime.java?20.00%?
    ComponentDiscoveryService.java?0.00%?
    ComponentMonitor.java?100.00%?
    CustomThreadFactory.java?41.18%?
    DataCollectionConfigStorage.java?88.89%?
    DataCollectionDefaultChange.java?100.00%?
    DefaultHeartBeatController.java?95.59%?
    DefaultUserAgentPublisher.java?95.45%?
    DelegatingScheduledExecutorService.java?22.64%?
    DelegatingScheduledFuture.java?69.23%?
    EmulatedServiceSettings.java?0.00%?
    ExecutorsRegistrar.java?100.00%?
    Firebase.kt?55.56%?
    FirebaseApp.java?58.82%?
    FirebaseAppLifecycleListener.java?0.00%?
    FirebaseCommonRegistrar.java?97.87%?
    FirebaseError.java?0.00%?
    FirebaseExecutors.java?0.00%?
    FirebaseInitProvider.java?69.57%?
    FirebaseNetworkException.java?0.00%?
    FirebaseOptions.java?31.94%?
    FirebaseTooManyRequestsException.java?0.00%?
    FirebaseTrace.java?100.00%?
    GlobalLibraryVersionRegistrar.java?75.00%?
    HeartBeatConsumer.java?0.00%?
    HeartBeatConsumerComponent.java?0.00%?
    HeartBeatController.java?0.00%?
    HeartBeatInfo.java?100.00%?
    HeartBeatInfoStorage.java?93.23%?
    HeartBeatResult.java?100.00%?
    KotlinDetector.java?33.33%?
    LibraryVersion.java?100.00%?
    LibraryVersionComponent.java?100.00%?
    LimitedConcurrencyExecutor.java?0.00%?
    LimitedConcurrencyExecutorService.java?0.00%?
    PausableExecutor.java?0.00%?
    PausableExecutorImpl.java?0.00%?
    PausableExecutorService.java?0.00%?
    PausableExecutorServiceImpl.java?0.00%?
    PausableScheduledExecutorService.java?0.00%?
    PausableScheduledExecutorServiceImpl.java?0.00%?
    PublicApi.java?0.00%?
    SdkHeartBeatResult.java?0.00%?
    SequentialExecutor.java?0.00%?
    StartupTime.java?100.00%?
    UiExecutor.java?60.00%?
    UserAgentPublisher.java?0.00%?

  • firebase-functions

    Overall coverage changed from ? (837979c) to 0.00% (26628cf) by ?.

    20 individual files with coverage change

    FilenameBase (837979c)Merge (26628cf)Diff
    ContextProvider.java?0.00%?
    DaggerFunctionsComponent.java?0.00%?
    FirebaseContextProvider.java?0.00%?
    FirebaseContextProvider_Factory.java?0.00%?
    FirebaseFunctions.java?0.00%?
    FirebaseFunctionsException.java?0.00%?
    FirebaseFunctions_Factory.java?0.00%?
    Functions.kt?0.00%?
    FunctionsComponent.java?0.00%?
    FunctionsComponent_MainModule_BindProjectIdFactory.java?0.00%?
    FunctionsMultiResourceComponent.java?0.00%?
    FunctionsMultiResourceComponent_Factory.java?0.00%?
    FunctionsMultiResourceComponent_FirebaseFunctionsFactory_Impl.java?0.00%?
    FunctionsRegistrar.java?0.00%?
    HttpsCallableContext.java?0.00%?
    HttpsCallableOptions.java?0.00%?
    HttpsCallableReference.java?0.00%?
    HttpsCallableResult.java?0.00%?
    HttpsCallOptions.java?0.00%?
    Serializer.java?0.00%?

  • firebase-functions-ktx

    Overall coverage changed from ? (837979c) to 0.00% (26628cf) by ?.

Test Logs

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

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 11, 2023

Unit Test Results

  48 files   -     6    48 suites   - 6   1m 26s ⏱️ - 5m 39s
  78 tests  - 392    78 ✔️  - 392  0 💤 ±0  0 ±0 
156 runs   - 784  156 ✔️  - 784  0 💤 ±0  0 ±0 

Results for commit 1723142. ± Comparison against base commit 8fff029.

This pull request removes 470 and adds 78 tests. Note that renamed tests count towards both.
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testBindsService_oAndTargetingO
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testNoWrappedIntent
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testNullIntent
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_OTargetingO_highPriority
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_fallsBackToBindService
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[19]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[21]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[22]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[23]
…
com.google.firebase.CoroutinesPlayServicesTests ‑ Task#await() resolves to the same result as Task#getResult()
com.google.firebase.CoroutinesPlayServicesTests ‑ Task#await() throws an Exception for failing Tasks
com.google.firebase.DataCollectionPostNDefaultEnabledTest ‑ isDataCollectionDefaultEnabled_shouldDefaultToTrue
com.google.firebase.DataCollectionPostNDefaultEnabledTest ‑ isDataCollectionDefaultEnabled_whenPrefsFalse_shouldReturnFalse
com.google.firebase.DataCollectionPostNDefaultEnabledTest ‑ isDataCollectionDefaultEnabled_whenPrefsTrue_shouldReturnTrue
com.google.firebase.DataCollectionPostNDefaultEnabledTest ‑ setDataCollectionDefaultEnabledFalse_shouldUpdateSharedPrefs
com.google.firebase.DataCollectionPostNDefaultEnabledTest ‑ setDataCollectionDefaultEnabled_shouldNotAffectOtherFirebaseAppInstances
com.google.firebase.DataCollectionPreNDefaultEnabledTest ‑ isDataCollectionDefaultEnabled_shouldDefaultToTrue
com.google.firebase.DataCollectionPreNDefaultEnabledTest ‑ isDataCollectionDefaultEnabled_whenPrefsFalse_shouldReturnFalse
com.google.firebase.DataCollectionPreNDefaultEnabledTest ‑ isDataCollectionDefaultEnabled_whenPrefsTrue_shouldReturnTrue
…

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

1 similar comment
@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

1 similar comment
@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 11, 2023

Size Report 1

Affected Products

  • firebase-common

    TypeBase (837979c)Merge (26628cf)Diff
    aar75.7 kB88.4 kB+12.6 kB (+16.7%)
    apk (aggressive)112 kB123 kB+11.8 kB (+10.6%)
    apk (release)1.26 MB1.64 MB+377 kB (+29.8%)
  • firebase-common-ktx

    TypeBase (837979c)Merge (26628cf)Diff
    aar13.3 kB3.12 kB-10.2 kB (-76.6%)
    apk (aggressive)123 kB123 kB+76 B (+0.1%)
    apk (release)1.64 MB1.64 MB+820 B (+0.0%)
  • firebase-functions

    TypeBase (837979c)Merge (26628cf)Diff
    aar47.1 kB51.9 kB+4.81 kB (+10.2%)
    apk (aggressive)400 kB408 kB+8.49 kB (+2.1%)
    apk (release)1.82 MB2.20 MB+382 kB (+21.0%)
  • firebase-functions-ktx

    TypeBase (837979c)Merge (26628cf)Diff
    aar6.33 kB3.60 kB-2.73 kB (-43.1%)
    apk (aggressive)408 kB408 kB+80 B (+0.0%)
    apk (release)2.19 MB2.20 MB+2.94 kB (+0.1%)

Test Logs

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

@VinayGuthal VinayGuthal marked this pull request as draft July 11, 2023 19:35
@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

The public api surface has changed for the subproject firebase-common_ktx:
error: Removed package com.google.firebase.ktx [RemovedPackage]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@@ -99,7 +99,7 @@ class LibraryVersionTest : BaseTestCase() {
@Test
fun `library version should be registered with runtime`() {
val publisher = Firebase.app.get(UserAgentPublisher::class.java)
assertThat(publisher.userAgent).contains(LIBRARY_NAME)
assertThat(publisher.userAgent).contains("fire-fun-ktx")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only happens if the ktx sdk is present, right? or should it work even if there's no common-ktx artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently i think with the new approach irrespective of anything this will be logged.. I think we should stop supporting this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to register what was discussed offline: there's value keep reporting the ktx usage in the user agent string.

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-functions_ktx:
error: Removed package com.google.firebase.functions.ktx [RemovedPackage]

The public api surface has changed for the subproject firebase-functions:
error: Added class com.google.firebase.functions.FunctionsKt [AddedClass]

The public api surface has changed for the subproject firebase-common:
error: Added class com.google.firebase.Firebase [AddedClass]
error: Added class com.google.firebase.FirebaseKt [AddedClass]

The public api surface has changed for the subproject firebase-common_ktx:
error: Removed package com.google.firebase.ktx [RemovedPackage]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.firebase.ktx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mark everything in this file as deprecated?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants