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

build(deps): Bump android Appcompat to 1.4.1 #33072

Closed

Conversation

gabrieldonadel
Copy link
Collaborator

@gabrieldonadel gabrieldonadel commented Feb 9, 2022

Summary

Currently we are using Appcompat in version 1.0.2 which is almost 4 years old now, this PR updates it to version 1.4.1.

Using Appcompat 1.0.2 was also causing a crash on RNTester due to an error where FontFamily's method was not found (Related to #33065)

Closes #31620

Changelog

[Android] [Changed] - Bump android Appcompat to 1.4.1

Test Plan

Use ./scripts/test-manual-e2e.sh to test both RNTester and a new app

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 9, 2022
@@ -323,7 +323,7 @@ public View getContentView() {
}

public boolean isShowing() {
return mDialog.isShowing();
return mDialog != null && mDialog.isShowing();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RNTester was crashing without this extra check
image

@react-native-bot react-native-bot added the Platform: Android Android applications. label Feb 9, 2022
@analysis-bot
Copy link

analysis-bot commented Feb 9, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 172f990
Branch: main

@analysis-bot
Copy link

analysis-bot commented Feb 9, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,188,751 +68,175
android hermes armeabi-v7a 7,790,036 +69,059
android hermes x86 8,559,456 +68,782
android hermes x86_64 8,511,320 +68,318
android jsc arm64-v8a 9,857,741 +70,108
android jsc armeabi-v7a 8,843,894 +70,978
android jsc x86 9,825,019 +70,689
android jsc x86_64 10,420,988 +70,237

Base commit: 172f990
Branch: main

@gabrieldonadel
Copy link
Collaborator Author

@cortinico I believe this commit cd79317 broke CI, in particular the test_android_template workflow https://app.circleci.com/pipelines/github/facebook/react-native/12051/workflows/2592c9f6-217f-4595-9981-82f4097fecf3/jobs/234753

@gabrieldonadel
Copy link
Collaborator Author

Ohh never mind, I guess you already have a PR fixing this

@facebook-github-bot
Copy link
Contributor

@ShikaSD has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

Ohh never mind, I guess you already have a PR fixing this

Yup you're right. We're working on it, hopefully it will be out in the next hours. Sorry for the disruption 👍

@ShikaSD
Copy link
Contributor

ShikaSD commented Feb 9, 2022

Hi, thanks for upgrading it, do you mind updating the BUCK file for the dependency as well?

url = "mvn:androidx.appcompat:appcompat:aar:1.0.2",

@gabrieldonadel
Copy link
Collaborator Author

Hi, thanks for upgrading it, do you mind updating the BUCK file for the dependency as well?

url = "mvn:androidx.appcompat:appcompat:aar:1.0.2",

Sure @ShikaSD, I can update that. My only question is regarding the sha1 value, should I run some command that updates this automatically or do I need to get it from somewhere else?

@ShikaSD
Copy link
Contributor

ShikaSD commented Feb 9, 2022

Should I run some command that updates this automatically or do I need to get it from somewhere else?

It can be retrieved from google maven repo, I don't think we have any command that upgrades it automatically. If you struggle to find the hash, you can upgrade the version and then wait for the CI to fail, it should have a new hash there.

@gabrieldonadel
Copy link
Collaborator Author

It can be retrieved from google maven repo, I don't think we have any command that upgrades it automatically. If you struggle to find the hash, you can upgrade the version and then wait for the CI to fail, it should have a new hash there.

Ohh got it! Thanks @ShikaSD

@gabrieldonadel
Copy link
Collaborator Author

Hi, thanks for upgrading it, do you mind updating the BUCK file for the dependency as well?

Done 😄

@facebook-github-bot
Copy link
Contributor

@ShikaSD has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ShikaSD
Copy link
Contributor

ShikaSD commented Feb 9, 2022

I think CI is broken after the latest commit, it cannot find some androidx classes:

stderr: : warning: unknown enum constant androidx.annotation.RestrictTo.Scope.LIBRARY_GROUP_PREFIX

/root/react-native/ReactAndroid/src/main/java/com/facebook/react/views/unimplementedview/ReactUnimplementedView.java:22: error: cannot access androidx.core.widget.TintableCompoundDrawablesView
    mTextView.setLayoutParams(

@cortinico
Copy link
Contributor

#33068 has been marged. You should be able to rebase and get a green CI from that 👍

@gabrieldonadel
Copy link
Collaborator Author

I think CI is broken after the latest commit, it cannot find some androidx classes:

Yeah, I had quite a hard time installing BUCK in my machine to investigate this because I'm using an M1 mac, I finally got it working, I'll start investigating why this is falling now

@gabrieldonadel
Copy link
Collaborator Author

I think CI is broken after the latest commit, it cannot find some androidx classes:

stderr: : warning: unknown enum constant androidx.annotation.RestrictTo.Scope.LIBRARY_GROUP_PREFIX

/root/react-native/ReactAndroid/src/main/java/com/facebook/react/views/unimplementedview/ReactUnimplementedView.java:22: error: cannot access androidx.core.widget.TintableCompoundDrawablesView
    mTextView.setLayoutParams(

Any suggestion on how to fix this @ShikaSD @cortinico ?

I believe the cannot access androidx.core.widget.TintableCompoundDrawablesView error is build caused by some sort of version mismatch in the BUCK file, but I can't quite figure it out

@cortinico
Copy link
Contributor

Any suggestion on how to fix this @ShikaSD @cortinico ?

Yup, you'll need to add to the BUCK file you attached the following rule:

fb_native.android_prebuilt_aar(
    name = "appcompat-resources-binary",
    aar = ":appcompat-resources-binary-aar",
)

and edit the "appcompat" target with:

fb_native.android_library(
    name = "appcompat",
    visibility = ["PUBLIC"],
    exported_deps = [
        ":annotation",
        ":appcompat-binary",
+       ":appcompat-resource-binary",
        ":collection",
        ":core",
        ":cursoradapter",
        ":fragment",
        ":legacy-support-core-utils",
        ":vectordrawable",
        ":vectordrawable-animated",
    ],
)

If you see the other targets have similar definition

@gabrieldonadel
Copy link
Collaborator Author

Yup, you'll need to add to the BUCK file you attached the following rule

Adding that actually ends up breaking another place now
image

@gabrieldonadel
Copy link
Collaborator Author

Yup, you'll need to add to the BUCK file you attached the following rule

Adding that actually ends up breaking another place now

I managed to fix this by bumping android:core version as well

@facebook-github-bot
Copy link
Contributor

@ShikaSD has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@gabrieldonadel gabrieldonadel deleted the feat/bump-appcompat branch February 10, 2022 18:06
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gabrieldonadel in 6b61995.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 10, 2022
rasaha91 pushed a commit to rasaha91/react-native-macos that referenced this pull request Jul 7, 2022
Summary:
Currently we are using Appcompat in version 1.0.2 which is almost 4 years old now, this PR updates it to version 1.4.1.

Using Appcompat 1.0.2 was also causing a crash on RNTester due to an error where FontFamily's method was not found (Related to facebook#33065)

Closes facebook#31620

## Changelog

[Android] [Changed] - Bump android Appcompat to 1.4.1

Pull Request resolved: facebook#33072

Test Plan: Use `./scripts/test-manual-e2e.sh` to test both RNTester and a new app

Reviewed By: cortinico

Differential Revision: D34107105

Pulled By: ShikaSD

fbshipit-source-id: 966e4687b09ae50a88ee518622f073d72e8c6550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants