Skip to content

chore: AGP 8 updates#11318

Closed
Zazo032 wants to merge 21 commits into
firebase:masterfrom
Zazo032:master
Closed

chore: AGP 8 updates#11318
Zazo032 wants to merge 21 commits into
firebase:masterfrom
Zazo032:master

Conversation

@Zazo032

@Zazo032 Zazo032 commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

Description

Related Issues

Closes #11266

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@Zazo032 Zazo032 requested a review from lesnitsky as a code owner July 19, 2023 09:48
@Zazo032 Zazo032 changed the title Android-related updates chore: Android-related updates Jul 19, 2023
@Zazo032 Zazo032 requested review from Salakar and kroikie as code owners July 19, 2023 12:18

@russellwheatley russellwheatley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Zazo032 - thanks so much for this PR, very much needed 🚀 .

Is it possible you could separate into two PRs? One for android updates and another for pubspec dependency updates? There's a lot of changes and I am concerned about bumping Pigeon up a major version when we've experienced some issues with its integration. The file conflicts will disappear as well because the plugin versions have been bumped since you created this.

for the android updates, I would prefer compileSdkVersion and targetSdkVersion targeted 33. The AGP version 8.0.2 is only tested up to that level.

FYI; requires using Java 17 which is the LTS version and has been since 2021.

Comment thread packages/firebase_auth/firebase_auth_platform_interface/pubspec.yaml Outdated
Comment thread packages/firebase_core/firebase_core_platform_interface/pubspec.yaml Outdated
@Zazo032

Zazo032 commented Jul 27, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I will remove the pubspec.yaml version changes from this PR, and change compileSdkVersion and targetSdkVersion to 33.

@Zazo032

Zazo032 commented Jul 30, 2023

Copy link
Copy Markdown
Contributor Author

@russellwheatley I reverted the pubspec.yaml, compileSdkVersion and targetSdkVersion - feel free to comment if you still find any unwanted change as part of this PR

@russellwheatley russellwheatley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Zazo032 - great work! There is another change required that I've just noticed. compileSdkVersion is deprecated. Could you change them to compileSdk, please?
Screenshot 2023-08-01 at 12 30 28

I would also like any changes to example app code (I noticed changes to app installation and ml model downloader) or pubspec dependencies removed.

Other than those two last bits, this looks almost there. Thanks 😄

@Zazo032

Zazo032 commented Aug 1, 2023

Copy link
Copy Markdown
Contributor Author

TIL compileSdkVersion was deprecated. Do you know why flutterfire plugins use compileSdk 33 but the examples use compileSdk flutter.compileSdkVersion? Should I change those example compileSdk too?

@russellwheatley Just added your comments to the PR. from the example code files, I only left a small typo fix in firebase_ml_model_downloader - let me know if I should revert it too

@russellwheatley

Copy link
Copy Markdown
Member

@Zazo032 - Yes, please change to 33 for example apps. Typo is fine to stay 👍

@Zazo032

Zazo032 commented Aug 2, 2023

Copy link
Copy Markdown
Contributor Author

@russellwheatley done!

@Zazo032

Zazo032 commented Aug 8, 2023

Copy link
Copy Markdown
Contributor Author

@russellwheatley the e2e / ios (tests) (pull_request) check is failing due to a timeout, is there something I can do on my side?

@russellwheatley

Copy link
Copy Markdown
Member

@Zazo032 - are you able to build crashlytics example? I just received this exception which I don't get on master branch:

Duplicate class kotlin.collections.jdk8.CollectionsJDK8Kt found in modules jetified-kotlin-stdlib-1.8.0 (org.jetbrains.kotlin:kotlin-stdlib:1.8.0) and jetified-kotlin-stdlib-jdk8-1.7.10 (org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.10)

@russellwheatley

Copy link
Copy Markdown
Member

Don't worry about iOS CI, it doesn't affect this and it is failing across all CI flows at the moment 👍

@Zazo032

Zazo032 commented Aug 10, 2023

Copy link
Copy Markdown
Contributor Author

@russellwheatley I didn't get that error, what steps should I follow to reproduce it? Looks like it's using 2 different versions, but none of those are defined in the example project

@Zazo032 Zazo032 changed the title chore: Android-related updates chore: AGP 8 updates Aug 24, 2023
@vbuberen

Copy link
Copy Markdown
Contributor

Duplicate class kotlin.collections.jdk8.CollectionsJDK8Kt found in modules jetified-kotlin-stdlib-1.8.0 (org.jetbrains.kotlin:kotlin-stdlib:1.8.0) and jetified-kotlin-stdlib-jdk8-1.7.10 (org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.10)

Would like to join the discussion here as recently had similar problems on a few projects while moving from Kotlin 1.7.x to 1.8.x. In 1.8 there was this change: https://kotlinlang.org/docs/whatsnew18.html#updated-jvm-compilation-target due to which all projects need to declare stdlib dependency without specifying jdk7 or jdk8 from now on (even though, it was possible much earlier).
The problem is inconsistent as doesn't happen for every single setup. I guess the reason is that some project dependencies still have jdk7 or jdk8 specified in their build configs and the mechanism created by Jetbrains doesn't work in some setups.

In the end, for one big project I came up with this option of Kotlin declaration to be 100% sure that builds won't fail:

implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
    constraints {
        implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version") {
            because("kotlin-stdlib-jdk7 is now a part of kotlin-stdlib and to not fail builds due duplicate classes")
        }
        implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version") {
            because("kotlin-stdlib-jdk8 is now a part of kotlin-stdlib and to not fail builds due duplicate classes")
        }
    }

So I would suggest to either declare Kotlin this way or just drop -jdk8 which is one of changes in this PR and see if there are issues with build left.

@russellwheatley

Copy link
Copy Markdown
Member

@vbuberen - Thank you for the insight, I was able to build once I included the above mentioned code snippet.

@Zazo032 - all the UI packages have now been moved to a separate repo, do you mind updating the PR and removing those changes?

The next step is to implement the following:

Inside example/android build.gradle, add the following line if it does not exist:

buildscript {
	// The below line
    ext.kotlin_version = '1.9.0'
    repositories {
        google()
        mavenCentral()
    }

Inside the example/android/app build.gradle, add the following:

dependencies {
  // ... whatever dependencies are specified

  implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
  constraints {
    implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version") {
      because("kotlin-stdlib-jdk7 is now a part of kotlin-stdlib, we constrain to remove duplicate classes")
    }
    implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version") {
      because("kotlin-stdlib-jdk8 is now a part of kotlin-stdlib, we constrain to remove duplicate classes")
    }
  }
}

Thank you for your patience, and sorry for only just returning to this!

@vbuberen

vbuberen commented Sep 3, 2023

Copy link
Copy Markdown
Contributor

@russellwheatley Would like to get back to the discussion of Kotlin update again as my previous comment was posted based on what I did in apps projects, which were just consumers of plugins.

Yesterday, I did some additional testing in context of Plus Plugins which I maintain and would like to mention that while the suggested solution to resolve Kotlin versions works well for example apps, when a plugin with Kotlin 1.8.x or 1.9.x is added to some project with older Kotlin version the same issue duplicate classes will appear again. So the consuming project has to also have the code snippet I shared above along with new Kotlin version to build Android part. Thus, be aware of this.

Here is my comment on this matter in Plus Plugins: fluttercommunity/plus_plugins#2134 (comment)

@vbuberen

Copy link
Copy Markdown
Contributor

Hey @Zazo032. Are you Ok if I intercept this PR and update it myself to move this topic forward?

@Zazo032

Zazo032 commented Sep 28, 2023

Copy link
Copy Markdown
Contributor Author

@vbuberen hey! sorry for not following up on this - I'll be out until the second half of October, if you want to prioritise this, feel free to push any changes to the PR (I can check those changes from the phone). If not, I can address your suggestions once I'm back

@vbuberen

Copy link
Copy Markdown
Contributor

Thanks for your reply. Will work on updating this PR. Asked as yesterday met Russel in real life and remembered about this PR.

@russellwheatley

Copy link
Copy Markdown
Member

Hey @vbuberen, nice to meet you in Prague 😄. Feel free to take over the PR, I will gladly review! Thanks 👍

@vbuberen vbuberen mentioned this pull request Oct 13, 2023
10 tasks
@russellwheatley

Copy link
Copy Markdown
Member

Closing out in favour of: #11699

@firebase firebase locked and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Firebase packages don't fully support AGP 8

4 participants