Skip to content

Conversation

@szymonrybczak
Copy link
Collaborator

Summary:

Recently there was created discussion about removing package property from library's AndroidManifest.xml, and use namespace in build.gradle. But it turned out that it breaks compatibility and if you wish to retain the highest compatibility it's better to don't remove the package property from AndroidManifest.xml, and keep both these fields. And that's totally fine, but if we have package property and we're on RN version > 71 we'll see the warning about deprecation (that you should specify namespace via build.gradle).

In this PR I added filtering deprecation warning from gradlew commands output.

TODO:

  • Test changes on Windows.

Test Plan:

  1. Clone the repository and do all the required steps from the Contributing guide
  2. Create app with RN version > 71
  3. Install library that have in AndroidManifest.xml package property, e.g. yarn add react-native-pager-view
  4. Run this command
node /path/to/react-native-cli/packages/cli/build/bin.js run-android 

You shouldn't see warning regarding that setting package property in AndroidManifest.xml is deprecated.

@szymonrybczak szymonrybczak requested a review from cortinico June 19, 2023 21:17
@szymonrybczak szymonrybczak changed the title fix: remove depreciation warning in run/build-android commands fix: filter depreciation warning in run/build-android commands Jun 19, 2023

export const filterGradlewOutput = (output: string) => {
const regex = new RegExp(
/package="([^"]+)" found in source AndroidManifest\.xml:(.*?)Setting the namespace via a source AndroidManifest.xml's package attribute is deprecated\.(.*?)Please instead set the namespace \(or testNamespace\) in the module's build\.gradle file, as described here: https:\/\/developer\.android\.com\/studio\/build\/configure-app-module#set-namespace(.*?\n)*This migration can be done automatically using the AGP Upgrade Assistant, please refer to https:\/\/developer\.android\.com\/studio\/build\/agp-upgrade-assistant for more information\./gms,
Copy link
Member

Choose a reason for hiding this comment

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

This is a single line, we can match only a part of it. The wording may change between versions—not sure if it happened ever though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I'm not sure what do you mean, because if we only check the first sentence "package="com.swmansion.gesturehandler.react" found in source AndroidManifest.xml:", how would we replace other part of this warning?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

I have to admit that I'm not a great fan of this approach, but I understand if we end up using it as we have not many other options.

A couple of things to consider first:

  1. The Manifest Merger source code actually has a flag to turn this logging off. I'm unsure if we can toggle it easily via AGP though:
    https://github.com/jrodbx/agp-sources/blob/0839a8e5ead11df9e8489a5aa4edda7e9d9cf984/7.3.1/com.android.tools.build/manifest-merger/com/android/manifmerger/ManifestMerger2.java#L198-L214

  2. It should be possible inside RNGP to use Artifact's API to manipulate the Manifest. That should allow us to remove those extra package= directives so that by the time the Manifest hits the Manifest Merger, they're removed.

@thymikee
Copy link
Member

Yeah, we treat this as a band-aid. Will you explore these solutions—which sound way better—on your own, or you need some kind of help?

@cortinico
Copy link
Member

or you need some kind of help?

Happy to receive a PR or collaborate on those. Option n.2 sounds the most feasible for me.
Also not extremely urgent as this needs to land before 0.73 which is still a bit far (can be backported to 0.72/0.71 eventually)

@szymonrybczak
Copy link
Collaborator Author

@cortinico I can help with this topic and fixing this 👍

@cortinico cortinico marked this pull request as draft June 20, 2023 13:20
@cortinico
Copy link
Member

Moving to draft as we explore alternatives.
The option to suppress some build logs could still be useful in the future also for other scenarios as well

@szymonrybczak
Copy link
Collaborator Author

@cortinico would you like to somehow move this forward?

@cortinico
Copy link
Member

@cortinico would you like to somehow move this forward?

For the sake of 0.73, I'll explore a different alternative so we probably won't need this right now

@szymonrybczak
Copy link
Collaborator Author

Let's close it then.

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.

3 participants