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

Remove kt_download_local_dev_dependencies() from WORKSPACE file once rules_kotlin() is released #1535

Closed
miaboloix opened this issue Jul 28, 2020 · 9 comments · Fixed by #5400
Closed

Remove kt_download_local_dev_dependencies() from WORKSPACE file once rules_kotlin() is released #1535

miaboloix opened this issue Jul 28, 2020 · 9 comments · Fixed by #5400
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@miaboloix
Copy link
Contributor

Is your feature request related to a problem? Please describe.
In order for Bazel to properly build Oppia Android, rules_kotiln must support data-binding. The current release of rules_kotlin does not include bazelbuild/rules_kotlin#346, which adds this necessary support. Thus, we have created our own fork of rules_kotlin (https://github.com/oppia/rules_kotlin) and released this fork (https://github.com/oppia/rules_kotlin/releases/tag/legacy-1.4.0-rcx-oppia-exclusive-rc01) in order to import the most recent version of rules_kotlin in Oppia Android's WORKSPACE file. However, in doing this, it has become clear that our release is missing certain dependencies that the official rules_kotlin release includes. In order to include those dependencies, we added the following lines to our WORKSPACE file:
load("@io_bazel_rules_kotlin//kotlin:dependencies.bzl", "kt_download_local_dev_dependencies")
kt_download_local_dev_dependencies()

However, once rules_kotlin releases a version with data-binding support, these lines will not be necessary.

Describe the solution you'd like
Once rules_kotlin releases a version with data-binding support, we should delete the two lines mentioned above.
@miaboloix miaboloix self-assigned this Jul 28, 2020
@miaboloix miaboloix added this to the Stage 2 of Bazel Migration milestone Aug 10, 2020
@FareesHussain
Copy link
Contributor

FareesHussain commented Jan 5, 2021 •
edited
Loading

@miaboloix
I would like to work on this as the above mentioned pull request is merged and released a version bazelbuild / Kotlin Rules 1.5.0 (Alpha 2)
image

@BenHenning
Copy link
Member

Hey @FareesHussain. I think @jcqli might be working on this as part of #2431.

@BenHenning BenHenning assigned jcqli and unassigned miaboloix Jan 8, 2021
@FareesHussain
Copy link
Contributor

Hey @FareesHussain. I think @jcqli might be working on this as part of #2431.

Sure, I did'nt notice that miaboloix was already assigned

@aggarwalpulkit596
Copy link
Contributor

@BenHenning this is done right?

@aggarwalpulkit596
Copy link
Contributor

I mean the underlying work for this has been done we just need to remove the local dependency from workspace file now

@jcqli
Copy link
Contributor

jcqli commented Jan 19, 2021 •
edited
Loading

I mean the underlying work for this has been done we just need to remove the local dependency from workspace file now

@aggarwalpulkit596 Removing kt_download_local_dev_dependencies() gives an error that @com_google_protobuf//:javalite_toolchain can't be found. @com_google_protobuf should be coming from our rules_kotlin so it might be an ordering issue, but in the current state we'll need to keep kt_download_local_dev_dependencies.

@rt4914
Copy link
Contributor

rt4914 commented Nov 18, 2021

@jcqli Unassigning you from this issue because of inactivity, please re-assign yourself if you are currently working on this.

@BenHenning
Copy link
Member

(Closing to trigger the TODO workflow).

@github-actions github-actions bot reopened this Jan 12, 2022
@github-actions
Copy link

The issue is reopened because of the following unresolved TODOs:

oppia-android/WORKSPACE

Line 33 in 87a7acc

# TODO(#1535): Remove once rules_kotlin is released because these lines become unnecessary

@BenHenning BenHenning mentioned this issue Jan 13, 2022
Merged
6 tasks
@Broppia Broppia added issue_type_infrastructure Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 29, 2022
@BenHenning BenHenning added Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. issue_user_developer labels Sep 15, 2022
@BenHenning BenHenning added the Z-ibt Temporary label for Ben to keep track of issues he's triaged. label Sep 15, 2022
@BenHenning BenHenning removed this from the Stage 2 of Bazel Migration milestone Sep 16, 2022
BenHenning added a commit that referenced this issue Feb 28, 2023
@BenHenning
Migrates to Bazel 5.0 & Kotlin 1.6.
b05b3e4 
This is early work to migrate the codebase to Bazel 5.x (and maybe 6.x),
as well as Kotlin to 1.6 (and maybe 1.7). The Kotlin migration is needed
due to an incompatibility with older rules_kotlin and Bazel 5.x. Other
dependencies needed updating, as well, including Dagger and rules_java.
Issue #1535 is being fixed as part of these upgrades.

As part of working through the Dagger issues that were encountered, it
was noticed that the codebase was actually setting up Dagger
incorrectly. This has been corrected: the codebase now defines a single
top-level dagger_rules().

This commit is not yet the final solution--the build doesn't yet work
and requires investigation before the rest of the work can be completed.

This commit partly continues the migration work from #4092 though it's
not yet finished. What remains includes:
- Migrating to latest Bazel & Kotlin versions
- If possible, modularize all third-party deps like oppia-proto-api
- Enable strict Kotlin & Java deps
- Enable warnings-as-errors for Kotlin & Java (and fix all issues)
- Ensure Guava-Android is used instead of JRE
- Resolve all Maven conflicts
- Remove dependence on Oppia's custom Android tools
- Remove support on legacy D8 (needs verification)
- Remove disabling incremental dexing (for faster builds)
- Disabling header compilation being disabled (for faster builds)
@BenHenning BenHenning mentioned this issue Feb 28, 2023
Merged
6 tasks
@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_infrastructure labels Mar 28, 2023
@BenHenning BenHenning mentioned this issue May 23, 2024
Merged
6 tasks
BenHenning added a commit that referenced this issue May 28, 2024
@BenHenning @adhiamboperes @seanlip
Fix #1535, part of #4120: Upgrade to rules_kotlin v1.5.0 beta 3 (#5400)
64ba622 
## Explanation
Fixes #1535
Fixes part of #4120

This PR preps for the codebase-wide migration to Kotlin 1.6 (done in
#4937) by first upgrading rules_kotlin from 1.5.0 alpha 2 to 1.5.0 beta
3. This upgrade, while small, produces a few nice benefits:
- It removes the need for extra WORKSPACE dependency setup (thus
addressing #1535).
- It moves a bunch of rules_kotlin macros to new locations which results
in changing nearly every ``BUILD.bazel`` file in the codebase.

The changes are straightforward to review in isolation, so this PR acts
as a means to help reduce the complexity of #4937 by pulling ahead these
groups of ``BUILD.bazel`` changes while also moving rules_kotlin the
smallest number of versions (ahead of the more major upgrades which will
occur downstream).

Note that this change is needed because:
- We'll need to upgrade to a newer version of rules_kotlin in order to
bring in Kotlin 1.6 support.
- rules_kotlin moved these macros and using the old ones results in a
_lot_ of build warnings that spam the local console when trying to
build.

Separately, this PR includes some minor trailing space cleanup in
wiki/Oppia-Bazel-Setup-Instructions.md that was noticed when editing the
file (since my local development environment auto-strips trailing
spaces).

## 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 should only affect build infrastructure, and barely impact
resulting binary builds.

---------

Co-authored-by: Adhiambo Peres <[email protected]>
Co-authored-by: Sean Lip <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
[Task Force] Migrate to Bazel - Android
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #1535, part of #4120: Upgrade to rules_kotlin v1.5.0 beta 3 oppia/oppia-android
8 participants
@rt4914 @seanlip @BenHenning @aggarwalpulkit596 @miaboloix @jcqli @FareesHussain @Broppia