Fix part of #59: Migrate to rules_jvm_external 5.1 #4925

BenHenning · 2023-03-27T20:36:56Z

Explanation

Fix part of #59

At a high-level, this PR migrates the project to using rules_jvm_external 5.1 (from the current version 4.1). The main benefit of this is a much simpler maven_install.json file (>50% reduction in file size). This is especially appealing as downstream PRs for #59 make a lot of changes to third-party dependencies, causing maven_install.json to be regenerated each time. A smaller and more compact format results in fewer actual deltas needing to be checked in between changes.

Introducing support for the new format has required a lot of changes elsewhere in the project, however, including:

A new Maven install Moshi model needing to be defined.
LicenseFetcher was extended to include support for verifying a URL's artifact validity (i.e. by using an HTTP HEAD request) for better robustness and performance when compiling licenses. This is because of a significant difference in the new maven_install.json format: we no longer know which Maven repositories correspond to which artifacts, so we need to test each artifact against all repositories when checking for licenses. This also has some repercussions on warnings from Bazel's own file downloader (see ability to silence warnings when using multiple repositories bazelbuild/rules_jvm_external#349).
Per the previous point, MavenDependenciesRetriever required substantial reworking to properly fetch pom information from dependencies. For robustness, it was also updated to use ScriptBackgroundCoroutineDispatcher to parallelize the fetching with some basic retry built into it (since I noticed on one of my workstations much more flakiness in the HEAD requests and license fetches).
LicenseFetcher was also renamed to MavenArtifactPropertyFetcher since it's actually used for more than just license fetching (besides the new HEAD check, it's also used for fetching an artifact's POM file when processing the list of licenses for the artifact).

Separate details worthy of noting:

The new version of rules_jvm_external introduces better support for duplicate dependency error handling and strict visibility.
- Both of these are just nice-to-have robustness checks against the existing //third_party wrapping setup (so they won't impact developers directly unless changes are made to the //third_party wrapping Bazel code).
- It also provides the ability to selectively override specific dependency targets (which is needed in Fix part of #59: Update Dagger structure & version from 2.28.1 to 2.41 #4931 to customize Guava in the build graph).
Source Jars are no longer being fetched to cut down on the amount of files that need to be downloaded. This may be reverted in the future.
Some minor dependency management was cleaned up in WORKSPACE.
The new MavenArtifactPropertyFetcherImpl & model changes don't have test files for the same reasons the old LicenseFetcherImpl and models didn't.
A lot of testing rework was needed for the license checks & update pathways since the means of representing licenses has fundamentally changed with the new maven_install.json format. The new retry functionality mentioned above also has resulted in a bit more output for these scripts (which results in changes to their tests).
The new MavenCoordinate class added as part of MavenDependenciesRetriever also had a bunch of tests added since this is now a public class of the retriever (and was added to simplify a lot of the coord manipulation work).
Some minor build time warnings were addressed during development.
Some of the new licenses for Crashlytics transitive dependencies refer to broken links that I cannot be confident in replacements for, but it seems reasonable to assume all of Crashlytics falls under Apache & the Crashlytics ToS based on the existing Crashlytics GitHub repository.

Important: maven_dependencies.textproto was significantly changed due to two reasons:

The order of the output for GenerateMavenDependenciesList is different due to a combination of the reworking of MavenDependenciesRetriever and the new maven_install.json format.
Per my suspicion, I think the previous implementation wasn't including transitive dependencies correctly. The new maven_install.json format makes it much easier to track these dependencies, and so we'll now be including many more third party license information in released versions of the app.

It is not expected that there are any actual version differences in this PR (intentionally since it would be very hard to verify that, and this allows maven_install.json to be easier to review since it's generated file). This was verified by looking at the conflicts section of both the old and new maven_install.json files, and the Maven dependencies CI check.

Essential Checklist

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

N/A -- This is a build system & script infrastructure change. It's expected to have no user-facing functional side effects.

The check & related scripts required fairly involved reworking since the Maven install file (from which it sources Maven URL context) changed in format as part of the upgrade for rules_jvm_external. This has actually led to what seems to be more correct analysis of libraries that the build depends on, so more licenses have been added to the maven_dependencies.textproto tracking file. One unused Crashlytics dependency was removed since it was referencing an old license that doesn't exist anymore (and the artifact should be replaced in full by more recent Firebase Crashlytics dependencies that we are already using).

This addresses an underlying bug with the command executor that can, in some cases, break compute_affected_tests. It also refines some of its internal mechanisms for much better performance on expensive PRs. It also prepares the base support needed for merge queues, but the CI workflows aren't being updated in this change.

This prepares for merge queues (but doesn't quite configure the workflow for them--that will happen in a different PR), and improves how tests are computed for stacked PRs.

…xternal Conflicts: scripts/src/java/org/oppia/android/scripts/common/BUILD.bazel scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesListCheck.kt scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt scripts/src/java/org/oppia/android/scripts/maven/GenerateMavenDependenciesList.kt scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt scripts/src/javatests/org/oppia/android/scripts/maven/GenerateMavenDependenciesListTest.kt

Also, update TODO check script to have nicer output, and support generating the exemption textproto file for easier updates in the future.

…xternal

oppiabot · 2023-04-08T03:24:49Z

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

The new proto target isn't used anywhere so this was missed.

…xternal

oppiabot · 2023-05-19T01:50:43Z

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

BenHenning · 2023-05-22T18:49:08Z

Still active.

oppiabot · 2023-05-29T19:42:47Z

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

Conflicts: scripts/assets/todo_open_exemptions.textproto

BenHenning · 2024-05-17T01:12:31Z

@seanlip PTAL for owners group codeowners changes (I think this is mainly needed for maven_install.json which is auto-generated).

seanlip

LGTM for one codeowner file: scripts/assets/maven_dependencies.textproto

adhiamboperes

Thanks @BenHenning. I have two questions, PTAL.

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt

…xternal

BenHenning

Thanks @adhiamboperes!

Self-reviewed latest changes & followed-up to review comments. PTAL @adhiamboperes.

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt

adhiamboperes

Thanks @BenHenning! This LGTM.

oppiabot · 2024-05-24T21:52:19Z

Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

BenHenning · 2024-05-26T19:14:25Z

Thanks @adhiamboperes! Looks like this is ready to be merged, so syncing with the latest develop and enabling auto-merge.

BenHenning added 5 commits March 27, 2023 13:36

Migrate to rules_jvm_external 5.1.

9ca7e2b

Fix JVm dep build warnings.

7238b59

Use base SHA for computing affected tests.

c325bc9

This prepares for merge queues (but doesn't quite configure the workflow for them--that will happen in a different PR), and improves how tests are computed for stacked PRs.

BenHenning changed the base branch from develop to upgrade-compute-affected-tests March 29, 2023 19:11

BenHenning added 4 commits March 29, 2023 12:25

Post-merge lint fix.

47f9c02

Fix updated TODOs.

8ae5aa8

Also, update TODO check script to have nicer output, and support generating the exemption textproto file for easier updates in the future.

Merge branch 'upgrade-compute-affected-tests' into update-rules-jvm-e…

de3cf14

…xternal

BenHenning changed the title ~~Migrate to rules_jvm_external 5.1~~ Migrate to rules_jvm_external 5.1 [Blocked: #4929] Mar 29, 2023

BenHenning added 2 commits March 31, 2023 19:58

Merge branch 'develop' into upgrade-compute-affected-tests

aaa0299

Merge branch 'upgrade-compute-affected-tests' into update-rules-jvm-e…

d520fa9

…xternal

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 8, 2023

oppiabot bot closed this Apr 15, 2023

BenHenning added 3 commits May 11, 2023 15:12

Merge branch 'develop' into upgrade-compute-affected-tests

8a19b4e

Fix breakage from develop.

773f7ff

The new proto target isn't used anywhere so this was missed.

Merge branch 'upgrade-compute-affected-tests' into update-rules-jvm-e…

311951f

…xternal

BenHenning reopened this May 12, 2023

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 12, 2023

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 19, 2023

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 22, 2023

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 29, 2023

BenHenning closed this Jun 2, 2023

BenHenning added the TODO(Ben): Revive label Jun 3, 2023

Merge branch 'develop' into upgrade-compute-affected-tests

525a601

Conflicts: scripts/assets/todo_open_exemptions.textproto

BenHenning assigned adhiamboperes and seanlip May 17, 2024

seanlip approved these changes May 17, 2024

View reviewed changes

seanlip removed their assignment May 17, 2024

adhiamboperes requested changes May 20, 2024

View reviewed changes

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt Show resolved Hide resolved

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt Outdated Show resolved Hide resolved

adhiamboperes assigned BenHenning and unassigned adhiamboperes May 20, 2024

Merge branch 'develop' into upgrade-compute-affected-tests

3dc6b7b

Base automatically changed from upgrade-compute-affected-tests to develop May 22, 2024 21:56

BenHenning added 4 commits May 22, 2024 21:59

Merge branch 'develop' into upgrade-compute-affected-tests

acd1c0d

Address reviewer comment.

c672af0

Merge branch 'upgrade-compute-affected-tests' into update-rules-jvm-e…

1b19f5b

…xternal

Follow-up variable rename for clarity.

3e923e2

BenHenning commented May 22, 2024

View reviewed changes

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt Show resolved Hide resolved

scripts/src/java/org/oppia/android/scripts/license/MavenDependenciesRetriever.kt Outdated Show resolved Hide resolved

BenHenning assigned adhiamboperes and unassigned BenHenning May 22, 2024

adhiamboperes approved these changes May 24, 2024

View reviewed changes

adhiamboperes assigned BenHenning and unassigned adhiamboperes May 24, 2024

oppiabot bot added the PR: LGTM label May 24, 2024

Merge branch 'develop' into update-rules-jvm-external

7951d46

BenHenning enabled auto-merge (squash) May 26, 2024 19:14

BenHenning changed the title ~~Fix part of #59: Migrate to rules_jvm_external 5.1 [Blocked: #4929]~~ Fix part of #59: Migrate to rules_jvm_external 5.1 May 26, 2024

BenHenning disabled auto-merge May 26, 2024 19:15

BenHenning enabled auto-merge (squash) May 26, 2024 19:15

BenHenning merged commit e57643b into develop May 26, 2024
44 checks passed

BenHenning deleted the update-rules-jvm-external branch May 26, 2024 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #59: Migrate to rules_jvm_external 5.1 #4925

Fix part of #59: Migrate to rules_jvm_external 5.1 #4925

BenHenning commented Mar 27, 2023 •

edited

oppiabot bot commented Apr 8, 2023

oppiabot bot commented May 19, 2023

BenHenning commented May 22, 2023

oppiabot bot commented May 29, 2023

BenHenning commented May 17, 2024 •

edited

seanlip left a comment

adhiamboperes left a comment

BenHenning left a comment

adhiamboperes left a comment

oppiabot bot commented May 24, 2024

BenHenning commented May 26, 2024

Fix part of #59: Migrate to rules_jvm_external 5.1 #4925

Fix part of #59: Migrate to rules_jvm_external 5.1 #4925

Conversation

BenHenning commented Mar 27, 2023 • edited

Explanation

Essential Checklist

For UI-specific PRs only

oppiabot bot commented Apr 8, 2023

oppiabot bot commented May 19, 2023

BenHenning commented May 22, 2023

oppiabot bot commented May 29, 2023

BenHenning commented May 17, 2024 • edited

seanlip left a comment

Choose a reason for hiding this comment

adhiamboperes left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

adhiamboperes left a comment

Choose a reason for hiding this comment

oppiabot bot commented May 24, 2024

BenHenning commented May 26, 2024

BenHenning commented Mar 27, 2023 •

edited

BenHenning commented May 17, 2024 •

edited