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.

Sign up for GitHub

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

Jump to bottom

Migrate from Gradle to Bazel #59

Open
BenHenning opened this issue Aug 16, 2019 · 12 comments
Open

Migrate from Gradle to Bazel #59

BenHenning opened this issue Aug 16, 2019 · 12 comments
Assignees
@BenHenning
Labels
enhancement End user-perceivable enhancements. Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Priority: Important This work item is really important to complete for its milestone, but it can be scoped out. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

BenHenning commented Aug 16, 2019 •
edited
Loading

This isn't an immediate priority, but Gradle has many pitfalls which makes development more challenging:

  • There's no control over which resources/manifests are included in the final binary beyond production/test dependencies on modules (which makes creating test modules/libraries more challenging)
  • Replacing component implementations (e.g. for test mocks/fakes) is non-trivial with Gradle
  • Gradle builds can be a bit slow
  • Gradle changes definitions often, so workarounds to any of these problems are likely to break quickly

Bazel allows for more granular control over how dependencies are manged, and in a structured way that will have long-lived support. Bazel includes native support for Android Studio (https://docs.bazel.build/versions/master/bazel-and-android.html) and can support Maven dependencies just like Gradle (https://docs.bazel.build/versions/master/generate-workspace.html).
@BenHenning BenHenning added Type: Improvement Priority: Important This work item is really important to complete for its milestone, but it can be scoped out. labels Aug 16, 2019
@BenHenning BenHenning added this to the Proof of concept milestone Aug 16, 2019
@BenHenning BenHenning self-assigned this Aug 16, 2019
@BenHenning
Copy link
Member Author

NB: This is currently targeting proof of concept, but it might get pushed back to the prototype milestone.

@BenHenning
Copy link
Member Author

This is blocked on #4.

@BenHenning BenHenning changed the title Migrate from Gradle to Bazel Migrate from Gradle to Bazel [Blocked: #4] Aug 16, 2019
@BenHenning
Copy link
Member Author

One other benefit: we can structure the project more granularly by directory rather than thinking about module-level separation, or needing to fit a specific structure enforced by Gradle.

@BenHenning
Copy link
Member Author

Another benefit: Bazel allows you to force libraries/targets to be test only, making it impossible to accidentally include them in user-facing binaries.

@BenHenning
Copy link
Member Author

Another benefit is easier integration with Java proto lite without requiring the hacky proto module workaround that we're currently using. This will be especially nice since it lets us keep the proto files we use close to the code that uses it, rather than having to keep all protos together in their own proto-only module.

@BenHenning BenHenning modified the milestones: Proof of concept, Prototype Aug 23, 2019
@BenHenning
Copy link
Member Author

Although we should try to get this done soon, it's not a strict requirement for proof-of-concept and we're a bit time constrained for that milestone. Let's work on this on a best-effort basis in the prototype milestone.

@BenHenning
Copy link
Member Author

NB: keeping this as an important priority for the prototype.

@BenHenning BenHenning mentioned this issue Aug 23, 2019
Closed
This was referenced Aug 28, 2019
Merged
Merged
Open
@BenHenning BenHenning changed the title Migrate from Gradle to Bazel [Blocked: #4] Migrate from Gradle to Bazel Sep 5, 2019
@BenHenning
Copy link
Member Author

@droidizer is looking into what an integration with Bazel would look like.

Sowmya: let me know if you'd like me to provide more guidance on how Bazel projects are laid out, and how we might be able to translate our existing project over. Initially, I suggest considering:

  1. Gradle modules don't need to have parity with Bazel structures (and probably shouldn't--Bazel does a better job at representing packages as libraries)
  2. Each directory should have its own BUILD file (and subsequently comprise one or more libraries)
  3. The top-level BUILD file should include the android_binary targets for the Oppia application

There are a bunch of other things to figure out, including how to handle tests. I think we'll also want to reorganize the project to match a more Bazel-like structure. We can chat about this later once you have a chance to look into Bazel more & consider how we should approach migrating.

@BenHenning BenHenning assigned droidizer and unassigned BenHenning Sep 5, 2019
@BenHenning BenHenning mentioned this issue Sep 6, 2019
Merged
6 tasks
@BenHenning BenHenning added Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. Z-ibt Temporary label for Ben to keep track of issues he's triaged. issue_user_developer labels Sep 16, 2022
@BenHenning BenHenning removed this from the Beta milestone Sep 16, 2022
@seanlip seanlip added enhancement End user-perceivable enhancements. and removed issue_type_enhancement labels Mar 28, 2023
@adhiamboperes adhiamboperes mentioned this issue Jun 24, 2023
Merged
6 tasks
@BenHenning BenHenning mentioned this issue Feb 9, 2024
Merged
6 tasks
BenHenning added a commit that referenced this issue Mar 15, 2024
@BenHenning @adhiamboperes
Fix part of #5312, part of #59: Introduce better script execution sup…
e2f94e4 
…port (#5313)

## Explanation
Fixes part of #5312
Fixes part of #59

This PR helps prepare for changes coming in #5315 and #4929 (the latter
of which is the start of the main upcoming Bazel migration PR chain) by
introducing one main scripts-based change:
``ScriptBackgroundCoroutineDispatcher``: a Kotlin coroutine dispatcher
for executing asynchronous tasks in scripts that also supports proper
Java executor service shutdown (so that scripts don't hang). This
dispatcher is multi-threaded to help simplify executing large numbers of
parallel background tasks.

All scripts have been migrated over to running their primary operations
within the context of this new dispatcher. Relevant script utilities
have been updated to use it, including ``CommandExecutor`` (though this
is mainly a placeholder change for the main executor changes which are
coming in #4929).

Miscellaneous details to note:
1. A bunch of 'e.g.' typos were fixed in
``GenerateMavenDependenciesList.kt`` and
``wiki/Updating-Maven-Dependencies.md``. These aren't functionally
needed, they were just something I noticed while developing.
2. ``kotlinx-coroutines-core`` was updated from 1.4.1 to 1.4.3 in order
to work around Kotlin/kotlinx.coroutines#2371
which was causing flakiness in one of the new dispatcher tests.
3. ``testClose_pendingTaskLongerThanCloseTimeout_taskIsNotRun``
intentionally takes ~2 seconds to run in order to provide some assurance
that, without cancellation, the task _would_ run and the test _would_
fail (this has been manually verified in a few different situations of
the dispatcher and/or test changing; some changes won't result in a
failure due to how cancellation works internally for executor service &
the converted coroutine dispatcher).

Note that historically these changes were originally part of #4929, but
they were split out so that they could be used by #5315 (which ended up
being convenient to include prior to #4929).

## 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
This PR doesn't include any user-facing changes since it only impacts
scripts.

---------

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
This was referenced May 16, 2024
Merged
Merged
Merged
BenHenning added a commit that referenced this issue May 22, 2024
@BenHenning @adhiamboperes @seanlip
Fix part of #59, part of #3926: Upgrade compute affected tests (#4929)
24b1721 
## Explanation
Fix part of #59
Fix part of #3926

As part of developing downstream PRs for #59, it was discovered that PRs
which change a LOT of files (such as #4937) can run into problems where
ComputeAffectedTests simply times out trying to compute the entire list
of affected targets.

**Critical performance and compatibility fixes**

There have been past efforts to optimize the affected tests workflows
(bucketing, breaking up some of the computations), but it was discovered
that the most expensive part of the process is running the
``rbuildfiles`` query to figure out which BUILD files were affected by
changed files. It was known this was an expensive step in the past, but
it wasn't clear until this PR exactly how to address it. This PR changes
the script to now:
- Filter ``rbuildfiles`` to only run under first-party targets (since it
shouldn't be possible for third-party BUILD files to be affected by
first-party changes). This reduces the search graph.
- Introduce Bazel command sharding for this step like the script already
does for others. This breaks up the command to run on a subset of files
multiple times, combining the output rather than running a single
command with a large input. It seems that ``rbuildfiles`` does not scale
linearly with its input size, so this drastically improves the script's
performance. It's thought that this approach is also more logically
correct due to more correct sibling matching semantics, but it's a bit
hard to reason about ``bazel query`` behavior at times so I can't be
100% confident in this. Nevertheless, the existing tests pass and I
haven't seen any issues from using these changes in downstream PRs.

Separately, another issue was discovered wherein some commands
(including certain Bazel commands) can actually cause
``CommandExecutorImpl`` to soft-lock and always time out. This was due
to an issue in the previous implementation wherein it would wait to read
a command's stdout until after the timeout has been completed (i.e. it
assumed the process would finish). This isn't correct, however: stdout
is blocking I/O, and some commands are implemented to only continue
execution after their standard output is read. The new implementation
makes use of coroutine actors to consume stdout and stderr at the same
time as waiting for execution to ensure all commands can continue
execution and that they finish within the desired timeout. Note that the
new ``ScriptBackgroundCoroutineDispatcher`` was actually introduced (in
#5313) specifically to support this new ``CommandExecutorImpl``
implementation, though it has since been found to have lots of other
nice benefits by providing scripts with a reliable mechanism for
performing asynchronous operations without having to manage their own
execution dispatcher.

Command execution for Bazel commands has also been updated to time out
after 5 minutes rather than the previous 1 minute. Despite the
optimizations and robustness improvements above, some commands still
take quite some time to run for especially large and complex cases.
While this change may result in a slower failure turnaround in cases
when commands are soft-locked, it should result in better CI and script
robustness in the long-term.

**Better support for chained PRs & possibly merge queues**

ComputeAffectedTests was also updated to use a merge base commit rather
than a reference to the develop branch (where this new commit hash is
provided by the CI workflow). The idea behind this is that the merge
base commit is:
- More logically correct (as ``ComputeAffectedTests`` is meant to run in
contexts where a branch wants to be merged into a destination).
- Better compatible with chained PRs. This allows for **significantly**
better CI performance in chained PR situations since now only the tests
relevant to the child PR will be run rather than all tests for the child
& its parental hierarchy (see downstream PRs' CI runs to see this
working in action).
- Hopefully better compatibility with merge queues (#3926). This hasn't
been verified, but the flexibility in customizing the destination for
affected tests should be the main prerequisite to properly supporting
merge queues.

**Other changes**

``GitClient`` was updated to have a peace-of-mind check to ensure the
base commit (provided as explained in the previous section) matches the
merge base of the current branch. This should always be true (except
maybe in merge queues--this will need to be verified). Note that this is
only a soft warning, not an assertion failure.

``RepositoryFile`` was cleaned up slightly to be a bit more consistent
with other directory management approaches done in scripts. I can see
this being refactored more in the future. Callsite behavior isn't
expected to be affected by these changes.

Some script tests were updated to have consistent formatting (which
required updating the TODO exemptions). ``TodoIssueResolvedCheckTest``
and ``TodoOpenCheckTest`` also had some of their test file management
cleaned up a bit.

**A note on testing**

These are inherently difficult things to test. I've verified what I
could via CI and general observation, but I've also largely relied on
existing tests to catch regressions (and many were caught during changes
to the script). Since these are mainly implementation and not behavioral
changes, I'm comfortable with the level of testing that was done.

## 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 an infrastructure-only change.

---------

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
Co-authored-by: Sean Lip <sean@seanlip.org>
This was referenced May 23, 2024
Merged
Merged
BenHenning added a commit that referenced this issue May 26, 2024
@BenHenning @adhiamboperes @seanlip
Fix part of #59: Migrate to rules_jvm_external 5.1 (#4925)
e57643b 
## 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 added a commit that referenced this issue May 27, 2024
@BenHenning @adhiamboperes @seanlip
Fix part of #59: Update Dagger structure & version from 2.28.1 to 2.41 (
6b724ac 
#4931)

## 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
``//:dagger`` library 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:
- Dagger is being upgraded to 2.41 from the current 2.28.1.
- The implicit Kotlin stdlib-jdk8 dependency is being bumped from 1.4.10
to 1.5.32 (this could be significant, but note that we are already using
the 1.5.0 base Kotlin SDK). Note that this resulted in some additional
Proguard exemptions for production builds. These *should* be okay to
ignore, but they will be reevaluated in #4937.
- Guava is being specially handled to force the Android version of Guava
(and specifically 31.0.1 which is needed by the newer version of
Dagger). Without this override the JRE version would be used which would
cause Desugaring and runtime problems in production builds.
- Other dependencies were updated or explicitly listed, as needed, for
the Dagger & Guava changes:
  - com.google.errorprone:error_prone_annotations: 2.9.0 -> 2.11.0
  - com.google.guava:failureaccess: 1.0.1 (currently used version)
  - com.google.j2objc:j2objc-annotations: 1.3 (currently used version)
- org.checkerframework:checker-compat-qual: 2.5.5 (currently used
version)
  - org.checkerframework:checker-qual: 3.13.0 -> 3.21.3

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 ``ExplorationProgressListener`` binding was removed from
``ExplorationProgressModuleTest`` because it was noticed during
development that it isn't actually used.

## 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 -- build infrastructure-only change.

---------

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
Co-authored-by: Sean Lip <sean@seanlip.org>
adhiamboperes added a commit that referenced this issue Jun 12, 2024
@BenHenning @adhiamboperes @seanlip
Fix #4119, #4120, and part of #59: Upgrade to Kotlin 1.6.10 (#4937)
4c7f811 
## Explanation
Fixes #4119
Fixes #4120
Fixes part of #59

This PR finishes the migration of the codebase to Kotlin 1.6 (addressing
both #4119 and #4120).

Kotlin 1.6 is needed as part of moving rules_kotlin to 1.7.x (which is,
in turn, needed in conjunction with Bazel 6.x to enable strict
dependency checking which significantly simplifies modularization which
is planned for downstream PRs). This PR doesn't actually finish the
movement to that version of rules_kotlin, but it does finish moving the
codebase to a new enough (and no longer pre-release) version of
rules_kotlin to allow using Kotlin 1.6 (over Kotlin 1.4 that the
codebase currently uses): version 1.5.0.

Previous PRs (#5400 and #5402) prepared for the changes here by
addressing large categories of build warnings that have either arisen
from this migration, or from past work. Note that another large category
of warnings have also been addressed in this PR: by moving to Kotlin
1.6, there's no longer a runtime incompatibility between the Kotlin SDK
and the reflection APIs (which was causing a _lot_ of warning output
previously). Between all three PRs, the output is now very clean and
free of nearly all build warnings.

To try and keep the warnings clean long-term, this PR introduces
warnings-as-errors for both Java and Kotlin code. However, please note
some caveats:
- Dagger generated code doesn't follow the Java warnings-as-error flag,
so those warnings were cleaned up manually (and will need to be
generally watched for, unfortunately).
- The version of rules_kotlin used in this PR doesn't directly support
turning on the functionality, but does internally (so a small patch file
has been added to augment rules_kotlin). When the codebase is updated to
rules_kotlin 1.7.x this patch will no longer be needed.
- To ease development, a build configuration flag was added to disable
failure upon encountering build warnings (per
https://bazel.build/run/bazelrc and
https://bazel.build/docs/configurable-attributes#query-and-cquery as an
example), though this needs to be opted into:
  ```sh
  bazel build --config=ignore_build_warnings <target>
  ```

Some other details to note:
- Version 1.6.10 is specifically picked in order to ensure Jetpack
Compose compatibility (for preparation of the work being prototyped in
#5401 to be compatible with the Oppia Android build environment).
- The vast majority of code in this PR is updating parameterized tests
to use a cleaner repeatable annotation pattern that wasn't available in
Kotlin 1.4.
- This upgrade absolutely does have runtime implications, but we're
relying very heavily on existing automated tests to ensure correctness
and no regressions.
- This PR doesn't make an effort to move toward newer Kotlin language
features except where forced (API deprecations) or largely wanted (the
repeatable annotation change).
- android-spotlight and kotlitex have been updated to support newer
versions of Kotlin (as both are custom forks managed in the broader
Oppia GitHub organization).
- Gradle files have been updated to match the same dependency versions
as Bazel (where it was obvious to make changes; some might still be a
bit off).
- The Gradle build configuration was also updated to use Kotlin 1.6.x
(otherwise there would be build incompatibilities with Bazel). I think
this is the last upgrade we can do for Gradle without upgrading AGP
(which will cause us significant issues with the model module, so we're
planning on instead dropping Gradle support).
- API changes that needed to be addressed in this PR due to deprecations
include: ``String.captialize``, ``String.toLowerCase``,
``String.toUpperCase``, ``SendChannel.offer``, and ``Char.toInt``.
- New API changes that have been leveraged in this PR:
``Flow.lastOrNull`` and ``Deferred.asListenableFuture`` (to replace
``SettableFuture`` for safety; this also resulted in nice
simplifications in ``CoroutineExecutorService``).
- The JVM coroutines dependency needed to be split out from Maven and
manually imported with some empty internal Java class files since it
otherwise has some issues being desugared:
bazelbuild/bazel#13553. This is a problem with
the Desugarer used in Bazel 4.x (and maybe later versions, so this
solution will probably need to kept for a while).
- Some Proguard rule updates were needed due to Kotlin SDK changes--see
the Proguard file & comments for specifics.
- Due to dependency changes, the KitKat main dex file was also trimmed
down. I'm fairly certain that it's already crashing on startup, so I
don't care much about this change--it just needs to build. We plan to
remove KitKat entirely eventually, anyway: #5012.
- Jetifier (that is, automatic conversion from support libraries to
Jetpack/AndroidX) support was disabled in Gradle. We don't have it
enabled in Bazel, and it could potentially encourage strange one-version
violations if it was ever actually needed. This is a safer (and likely
more performant) change to make.
- Moshi was updated to 1.13 to support the upgrade in Kotlin. This did
result in a small configuration change due to its annotation processor
being moved. Note that Moshi 1.14 couldn't be supported since it
requires Kotlin 1.7+ which requires rules_kotlin 1.7+. This will be an
option to upgrade in the future.
- Some improvements and fixes were made in
``FilterPerLanguageResources`` (I think it was outputting something
incorrectly before and that's now been fixed as part of a broader
logical reworking of the filtering logic).
- ``com.android.support:support-annotation`` was removed as a dependency
since it was never used in Bazel, and shouldn't be used (since it's
support library and not AndroidX).
- The updates to Moshi and Kotlin dependencies resulted in a bunch of
other transitive dependency updates.
- Note that Gradle doesn't have ``allWarningsAsErrors`` enabled since it
would require fixing more warnings than is exposed in Bazel, and we're
using Bazel builds as the general source of truth for code quality.

## 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 an infrastructural change. While it could inadvertently
affect user-facing code, it shouldn't based on the current passing state
of automated tests.

---------

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
Co-authored-by: Sean Lip <sean@seanlip.org>
BenHenning added a commit that referenced this issue Jun 13, 2024
@BenHenning @adhiamboperes @seanlip
Fix #5370, part of #59: Migrate to Bazel 6.5.0 (#4886)
add9f74 
## Explanation

Fixes #5370
Fixes part of #59

This PR updates the project to use Bazel 6.5.0 instead of 4.0.0.

Note that most of the changes done so far in addressing #59 are centered
around the concept of simplifying the Bazel maintenance as much as
possible so that it's not too much more difficult than Gradle by the
time we fully remove Gradle support from the project. While Bazel will
always require more effort, there are many things that can be done to
narrow the gap. This is a major step in that process since Bazel 4.x
required using a custom Android toolchain
(https://github.com/oppia/oppia-bazel-tools) which is not at all user
friendly. Plus, there are many compatibility and performance
improvements in later versions of Bazel that we want to be able to
incorporate within the broader Oppia Android project.

Bazel 6.x was specifically chosen because:
- Bazel 4.x was missing support for the new D8 version which made it
impossible to upgrade past 29.0.2 build tools
bazelbuild/bazel#13989.
- Bazel 5.x had some additional compatibility issues with the D8 change,
so we weren't able to use it, either:
bazelbuild/bazel#15957.
- Bazel 7.x (which wasn't released when this work was originally done)
introduces new bzlmod support that causes some additional build
headaches that can be figured out later.
- Bazel 6.5.0 specifically was chosen since it's the latest 6.x version
(as of this edit) and seems to work correctly with existing unit tests.

Some other important details to note:
- rules_kotlin 1.7.x is needed at a minimum for Bazel 5.x+ support.
However, an additional fix was needed
(bazelbuild/rules_kotlin#940) in order to fix a
deviation in functionality that occurred starting in Bazel 5.x's
java_plugin support which led to some file duplication in rules_kotlin
(that was fortunately easy to fix). Unfortunately, this change wasn't
backported to 1.7.x so this PR makes use of a custom patch to
rules_kotlin 1.7.1 (https://github.com/oppia/rules_kotlin) that includes
the needed change. We'll get this change properly once we can upgrade to
1.8.x, though that will also require updating Kotlin itself to 1.8.x due
to bazelbuild/rules_kotlin#1019.
- Bazel 6.x (maybe 5.x) requires at least build tools 30.0.0 since it
completely removed support for the old D8 compat dexer. 32.0.0 was
chosen in this PR as it's simply a newer, more up-to-date build tools
(and removes D8 completely). With this upgrade to Bazel 6.x we'll be
able to update the build tools version more often (so long as it doesn't
introduce AGP incompatibilities since we can't upgrade Gradle).
- As of Bazel 6.x, we're able to reenable Java header compilation and
incremental dexing, both of which should have _significant_ performance
improvements for incremental builds of the app (and in fact we will have
build errors if we disable incremental dexing).
- In CI, we opted to **not** support build tools 29.0.2 or old builds of
the app. Instead, we'll rely on build tools failing for certain PRs as
an indicator that those PRs will require an update (once this PR is
merged) in order to have CI run correctly. This is a lot easier than
trying to figure out how to support before/after changes with some
fairly complex environment differences.
- There are a bunch of version updates that were needed to support the
minimum version of Kotlin for rules 1.7.x (1.6 I think) as well as JDK
11 (which I think was needed for Bazel 5.x), and these have largely been
taken care of in previous PRs to this one (though the JDK 11 update in
CI was done in this PR, along with wiki documentation updates to address
#5370). One such case of a necessary version upgrade:
google/dagger#2511.
- There was a change needed for the databinding java_plugin declaration
to specify that it generates an API (in order for it to be used
correctly in builds).
- rules_java needed to be updated to support the newer version of Bazel.
- The desugaring hack needed for kotlinx-coroutines-core-jvm was removed
since it's no longer needed with the build tools & Bazel upgrade
introduced in this PR.
- This includes one small change in third-party to change all
single-export wrappers that don't have additional plugins being enabled
to aliases instead. This is more semantically correct as the wrappers
may lose information (which caused problems when investigating adding
Jetpack Compose support in #5401). While this isn't directly required
for the Bazel upgrade, this is the last PR needed for Jetpack Compose
support so it's being added here for simplicity.
- ``.bazelrc`` was updated to configure tools, tests, and builds to all
use the remote JDK 11 available via Bazel rather than ever using the
user's local JDK. This should improve build hermeticity and consistency
across different user environments (see
https://bazel.build/docs/bazel-and-java).
- Setup docs were updated to remove setting up JDK 11 (or Java at all
for Linux & Mac) now that the user no longer needs to install Java (see
previous point) except for Windows. The Python instructions were also
removed since Bazel 6.x includes fixes for Android tools that previously
depended on Python 2.x.
- CI was unchanged for Java setup since, as far as I can tell, it's
still needed for sdkmanager.

There was also some small cleanup in unit_tests.yml that I noticed when
updating CI versions.

## 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 infrastructure change. It shouldn't impact the
end user experience.

---------

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
Co-authored-by: Sean Lip <sean@seanlip.org>
Vishwajith-Shettigar added a commit to Vishwajith-Shettigar/oppia-android that referenced this issue Jun 15, 2024
@Vishwajith-Shettigar
Revert "Fix oppia#5370, part of oppia#59: Migrate to Bazel 6.5.0 (opp…
40bce79 
…ia#4886)"

This reverts commit add9f74.
@BenHenning BenHenning mentioned this issue Jun 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BenHenning BenHenning

Labels
enhancement End user-perceivable enhancements. Issue: Needs Break-down Indicates that an issue is too large and should be broken into smaller chunks. Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Priority: Important This work item is really important to complete for its milestone, but it can be scoped out. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
[Task Force] Migrate to Bazel - Android
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
6 participants
@droidizer @seanlip @BenHenning @fsharpasharp @miaboloix @Broppia