Skip to content
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

feat(android): Add support for AGP 8 in example, add compileOptions to build.gradle #1503

Merged
merged 20 commits into from
Jul 20, 2023

Conversation

Skyost
Copy link
Contributor

@Skyost Skyost commented May 10, 2023

Description

This PR adds support of namespace property to support Android Gradle Plugin (AGP) 8. Projects with AGP < 4.2 will not supported anymore. It fixes the following error :

Namespace error

See flutter/flutter#125181.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs:, chore: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation and added dartdoc comments with ///, where necessary.
  • I have updated/added relevant examples in example.

Breaking Change

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

Migration instructions

You can (and you should !) upgrade your Android project to use AGP 8. In your build.gradle :

buildscript {
  // ...

  dependencies {
    classpath 'com.android.tools.build:gradle:8.0.1'
    // ...
  }
}

Also don't forget to add a namespace in the android section of app/build.gradle :

android {
  // ...
  namespace 'your.namespace'
}

Related Issues

@Skyost Skyost changed the title fix!: Add support of namespace property to support Android Gradle Plugin (AGP) 8 fix(android)!: Add support of namespace property to support Android Gradle Plugin (AGP) 8 May 10, 2023
@Skyost Skyost changed the title fix(android)!: Add support of namespace property to support Android Gradle Plugin (AGP) 8 fix(android): Add support of namespace property to support Android Gradle Plugin (AGP) 8 May 13, 2023
@Gustl22
Copy link
Collaborator

Gustl22 commented May 14, 2023

As far as I remember the namespace feature didn't work with flutter < [3.7.12](https://github.com/flutter/flutter/releases/tag/3.7.12) (after clearing the gradle cache). Do we have to raise the minimum Flutter version in pubspec.yaml in order to make it working?

@Skyost
Copy link
Contributor Author

Skyost commented May 15, 2023

I don't think it's needed because we're checking if namespace is available first :

if (project.android.hasProperty('namespace')) {
  namespace 'xyz.luan.audioplayers'
}

@vbuberen
Copy link

As far as I remember the namespace feature didn't work with flutter < [3.7.12](https://github.com/flutter/flutter/releases/tag/3.7.12) (after clearing the gradle cache). Do we have to raise the minimum Flutter version in pubspec.yaml in order to make it working?

No, it is not required and not connected with Flutter version.
namespace doesn't work if the Android Gradle Plugin is older than 4.2 version as there were no such property: flutter/flutter#125181 (comment)

Ideally, users shouldn't be using such old AGP version. In Plus Plugins I decided to not go with compatibility check, like the one suggested in the comment above, but release a major version bump to encourage people to update their projects.

@Skyost
Copy link
Contributor Author

Skyost commented May 16, 2023

@vbuberen That's what I did first, but I reverted it in 42d3150 as I thought it was not my role to take such a decision.

@cian-bayer
Copy link

Hi, any eta on when this will be merged and released? I just upgraded my flutter project to AGP 8 and I believe audioplayers missing the namespace parameter is the last thing blocking my project from building for android. Thanks

@Gustl22 Gustl22 self-requested a review May 17, 2023 09:47
@Skyost
Copy link
Contributor Author

Skyost commented May 17, 2023

@cian-bayer Feel free to use the fork if you don't want to wait. In your pubspec.yaml :

audioplayers:
  git:
    url: https://github.com/Skyost/audioplayers.git
    path: packages/audioplayers/

@Gustl22
Copy link
Collaborator

Gustl22 commented May 17, 2023

Changing AGP version fails, due to flutter/flutter@3a6c4eb is not released yet. Then we also need to move to flutter 3.10.0 first, which depends on flame-engine/flame@b41622d to be released.

@@ -1,3 +1,6 @@
org.gradle.jvmargs=-Xmx1536M
android.useAndroidX=true
android.enableJetifier=true
android.defaults.buildfeatures.buildconfig=true
android.nonTransitiveRClass=false
android.nonFinalResIds=false
Copy link
Collaborator

@Gustl22 Gustl22 May 17, 2023

Choose a reason for hiding this comment

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

Are these changes needed? According to the release notes the defaults have changed to improve build performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're not. In fact, they happened when I used the Android Studio migration tool. I can remove them if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that would be nice 😊

@vbuberen
Copy link

Changing AGP version fails, due to flutter/flutter@3a6c4eb is not released yet. Then we also need to move to flutter 3.10.0 first, which depends on flame-engine/flame@b41622d to be released.

There is no need to bump to AGP 8.0 with this PR. The suggested change is only to add a namespace property which is required by projects that already migrated to AGP 8.0, thus, add a compatibility for those that decided to jump on this version already.

@Gustl22
Copy link
Collaborator

Gustl22 commented May 18, 2023

@Skyost can you open a second PR with the changes in the example project and just update the audioplayers_android platform here (without the example). Or the other way around.
Otherwise we are blocked by: flutter/flutter@3a6c4eb
We then can leave the PR for example open until flutters fix in integration tests is released.

Edit: already started another PR to consider the AGP only without testing it.

@Skyost
Copy link
Contributor Author

Skyost commented May 20, 2023

@Gustl22

started another PR to consider the AGP only without testing it.

That's okay, thanks ! I'll remove the changes to the gradle.properties file according to #1503 (review).

@vbuberen
Copy link

Otherwise we are blocked by: flutter/flutter@3a6c4eb We then can leave the PR for example open until flutters fix in integration tests is released.

Edit: already started another PR to consider the AGP only without testing it.

I think there is some misunderstanding from your side on what is needed to support AGP 8. Only adding namespace property (which appeared since AGP 4.2 as I shared in one of comments above) is needed and there is no need to bump AGP in the package to 8 for now, like you did in another PR. Thus, there is nothing blocking from adding this support and still having integration tests working.

Here is an example of a similar change, but without bumping to AGP 8: https://github.com/fluttercommunity/plus_plugins/pull/1698/files

According to the following discussion : #1503 (review).
@Gustl22
Copy link
Collaborator

Gustl22 commented May 21, 2023

there is no need to bump AGP in the package to 8 for now, like you did in another PR.

@vbuberen Is there a reason to not upgrade the build gradle tools in audioplayers_android package? I think it just works as before.

Only adding namespace property (which appeared since AGP 4.2 as I shared in one of comments above) is needed

Here is an example of a similar change, but without bumping to AGP 8

Why should we adding the namespace in the example without removing it in the AndroidManifest.xml. Then it doesn't have any benefit in my opinion and we should wait changing it, until the integration tests also support the namespace property.

@vbuberen
Copy link

Why should we adding the namespace in the example without removing it in the AndroidManifest.xml. Then it doesn't have any benefit in my opinion and we should wait changing it, until the integration tests also support the namespace property.

Because adding namespace would allow people who already moved their projects to AGP 8 to build their projects fine without errors saying that one of their dependencies is missing namespace property. Having package id in AndroidManifest has no effect on namespace, so shouldn't be considered as benefit/no benefit, but allows integration tests to work till the integration_test package supports the new namespace property.
Thus, having both allows to have a broader compatibility in the end without being blocked because integration_test don't understand namespace property as of now.

@vbuberen
Copy link

vbuberen commented May 22, 2023

Is there a reason to not upgrade the build gradle tools in audioplayers_android package? I think it just works as before.

I had some build issues with AGP 8 back then. Maybe it is not the case anymore as there were quite a few updates to Flutter and tooling in last 3-4 weeks. And my statement in previous comments meant that adding a namespace doesn't require to bump to AGP 8 as well.

@Gustl22
Copy link
Collaborator

Gustl22 commented May 22, 2023

Because adding namespace would allow people who already moved their projects to AGP 8 to build their projects fine without errors saying that one of their dependencies is missing namespace property. Having package id in AndroidManifest has no effect on namespace, so shouldn't be considered as benefit/no benefit, but allows integration tests to work till the integration_test package supports the new namespace property.

I think we are discussing two different things. I did not mean to change the AndroidManifest in audioplayers_android. Changing something in the example doesn't have any consequences for the package user / app developer. But example is used to do the integration tests. So I propose to nothing change there yet, like done in #1514.

At some day in the future we should removing the namespace from AndroidManifest in both the example and the audioplayers_android package.

I had some build issues with AGP 8 back then. Maybe it is not the case anymore as there were quite a few updates to Flutter and tooling in last 3-4 weeks. And my statement in previous comments meant that adding a namespace doesn't require to bump to AGP 8 as well.

I think the AGP version in audioplayers_android is overriden by the one of the user's app anyways (here example) so it wouldn't matter bumping it, maybe (?), but leaving it for now as done in flutter packages

@cian-bayer
Copy link

FWIW, I tried building for android from @Skyost 's branch like this:

  audioplayers:
    git:
      url: https://github.com/Skyost/audioplayers.git
      path: packages/audioplayers/

and got this error:

Launching lib/environment/main_dev/main_dev-local-release.dart on sdk gphone64 arm64 in release mode...
main_dev-local-release.dart:1

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring project ':audioplayers_android'.
> Could not create an instance of type com.android.build.api.variant.impl.LibraryVariantBuilderImpl.
   > Namespace not specified. Please specify a namespace in the module's build.gradle file like so:

     android {
         namespace 'com.example.namespace'
     }

     If the package attribute is specified in the source AndroidManifest.xml, it can be migrated automatically to the namespace value in the build.gradle file using the AGP Upgrade Assistant; please refer to https://developer.android.com/studio/build/agp-upgrade-assistant for more information.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 7s

and I made sure to run flutter pub get, and my pubspec.lock was pointing to the correct, most recent push in their fork: ebc4e86293412e93084337b8d8b82ce07687ef1d

Flutter doctor:

XXXX@XXXX project-directory % flutter doctor
Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 3.7.12, on macOS 13.3 22E252 darwin-arm64, locale en-US)
[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.2)
[✓] Xcode - develop for iOS and macOS (Xcode 14.3)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2022.2)
[✓] VS Code (version 1.78.2)
[✓] Connected device (3 available)
[✓] HTTP Host Availability

• No issues found!

Gustl22 added a commit that referenced this pull request May 23, 2023
Cherry-pick of #1503

Co-authored-by: Hugo Delaunay <Skyost@users.noreply.github.com>
@Gustl22 Gustl22 marked this pull request as draft May 23, 2023 06:44
@Gustl22 Gustl22 changed the title fix(android): Add support of namespace property to support Android Gradle Plugin (AGP) 8 ffeatix(android): Add support for AGP 8 with namespace property in example May 23, 2023
@Gustl22 Gustl22 changed the title ffeatix(android): Add support for AGP 8 with namespace property in example feat(android): Add support for AGP 8 with namespace property in example May 23, 2023
@Gustl22 Gustl22 marked this pull request as ready for review July 19, 2023 15:01
@Gustl22 Gustl22 requested a review from spydon July 20, 2023 14:59
@Gustl22 Gustl22 changed the title feat(android): Add support for AGP 8 with namespace property in example feat(android): Add support for AGP 8 in example, add compileOptions to build.gradle Jul 20, 2023
@Gustl22 Gustl22 enabled auto-merge (squash) July 20, 2023 15:29
@Gustl22 Gustl22 disabled auto-merge July 20, 2023 22:07
@Gustl22 Gustl22 enabled auto-merge (squash) July 20, 2023 22:47
@Gustl22 Gustl22 merged commit 7c08e4e into bluefireteam:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants