Skip to content

Configure Java toolchains per project - #41306

Closed
UNIDY2002 wants to merge 1 commit into
react:mainfrom
UNIDY2002:rn-73-jdk-setup-fix
Closed

Configure Java toolchains per project#41306
UNIDY2002 wants to merge 1 commit into
react:mainfrom
UNIDY2002:rn-73-jdk-setup-fix

Conversation

@UNIDY2002

@UNIDY2002UNIDY2002 commented Nov 3, 2023

Copy link
Copy Markdown
Contributor

Summary:

A follow-up fix on #40560

The fix introduced by #40757 worked under the false assumption that :app gets evaluated before any other library. However, some libraries, such as @amplitude/analytics-react-native (whose project name is :amplitude_analytics-react-native), might be evaluated before :app and miss the configureJavaToolChains() process.

This PR changes configureJavaToolChains() into a per-project implementation, and invokes the function during both library and app configurations. This can ensure that all projects get their Java toolchains configured promptly.

Changelog:

[ANDROID] [FIXED] - Configure Java toolchains per project

Test Plan:

Test with @amplitude/analytics-react-native and ensure that the android app builds in new arch.

@facebook-github-botfacebook-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 Nov 3, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a17,657,093-2
androidhermesarmeabi-v7an/a--
androidhermesx86n/a--
androidhermesx86_64n/a--
androidjscarm64-v8a21,034,736-4
androidjscarmeabi-v7an/a--
androidjscx86n/a--
androidjscx86_64n/a--

Base commit: 80d816a
Branch: main

@UNIDY2002

Copy link
Copy Markdown
ContributorAuthor

Wait... This fix is having issue on old arch...

@UNIDY2002

Copy link
Copy Markdown
ContributorAuthor

Libraries on old arch does not apply the RNGP, and will not get their Java toolchains configured. Therefore, this PR is inapplicable.

Closing it for now.

@UNIDY2002
UNIDY2002 deleted the rn-73-jdk-setup-fix branch November 6, 2023 02:23
@cortinico

cortinico commented Nov 20, 2023

Copy link
Copy Markdown
Contributor

However, some libraries, such as @amplitude/analytics-react-native (whose project name is :amplitude_analytics-react-native), might be evaluated before :app and miss the configureJavaToolChains() process.

Projects are not evaluated alphabetically (order is arbitrary). So you should not need this fix at all in the first place

EDIT: You were right. This is a bug (see #41620). Fix is forthcoming

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Shared with MetaApplied 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.

5 participants

@UNIDY2002@facebook-github-bot@analysis-bot@cortinico@cipolleschi