Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
## 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 bazel-contrib/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 #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: 1. 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. 2. 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 - [x] 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: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## 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. --------- Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com> Co-authored-by: Sean Lip <sean@seanlip.org>
- Loading branch information
3 people
authored
May 26, 2024
1 parent
5433b7f
commit e57643b