Fix part of #59: Update Dagger structure & version from 2.28.1 to 2.41 #4931
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
This moves the codebase to using the recommended single top-level Dagger library rather than replicating it in a bunch of different places.
This is needed for downstream work. It also includes ensuring that Guava JRE can never be used (since only Android should ever be referenced by the production app build).
|
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. |
oppiabot
bot
added
the
stale
The new proto target isn't used anywhere so this was missed.
oppiabot
bot
removed
the
stale
|
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. |
oppiabot
bot
added
the
stale
|
Still active. |
oppiabot
bot
removed
the
stale
|
PTAL @adhiamboperes. PTAL @seanlip for OWNERS file codeowners. |
seanlip
approved these changes
May 17, 2024
|
Unassigning @seanlip since they have already approved the PR. |
adhiamboperes
approved these changes
May 24, 2024
|
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
added a commit
that referenced
this pull request
May 26, 2024
## 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>
BenHenning
changed the title
BenHenning
commented
May 27, 2024
|
Thanks @adhiamboperes! Enabling auto-merge since there are no lingering changes requested. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fix part of #59
This PR introduces a simplification to Dagger setup throughout the Bazel build graph by defining a single top-level
dagger_rules()invocation rather than introducing up to one per directory. The new approach actually matches Dagger documentation for how it should be setup: https://github.com/google/dagger/blob/f41033cc448eb7bdb83af2356c8802f1208d1824/examples/bazel/BUILD#L20.Semantically, this mainly serves to centralize the exported Dagger dependencies to a single library. Depending on this new top-level
//:daggerlibrary will still enable Dagger annotation processing and code generation for the library depending on it, so it's not expected to result in any significant performance improvements for the build graph.Note that this PR also makes some attempt at removing cases when Dagger isn't needed as a dependency at all (though this will be done in much more significant detail in downstream PRs), and in some cases Dagger was added just for implicitly depending on javax annotations (which can be depended on directly).
There are also a number of version upgrades happening in this PR in preparation for upgrading to Kotlin 1.6 (in #4937) since older Dagger has some compatibility issues with newer Kotlin:
It was noticed that the tracking for Guava's third-party license was removed. This is due to Guava now being explicitly depended on rather than being managed by rules_jvm_external. #5397 was filed to track properly including all third-party licenses, not just those from Maven. Addressing that issue will result in Guava's license being included again in production builds.
Finally, a
ExplorationProgressListenerbinding was removed fromExplorationProgressModuleTestbecause it was noticed during development that it isn't actually used.Essential Checklist
For UI-specific PRs only
N/A -- build infrastructure-only change.