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.

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

Fixes part of #59: Build app module views + view models with Bazel #1581

Merged
merged 337 commits into from
Aug 19, 2020

Conversation

miaboloix
Copy link
Contributor

@miaboloix miaboloix commented Aug 9, 2020

Explanation

Fixes part of #59: Build app module view models with Bazel [BLOCKED: #1568]

In order to build app module in Bazel, parts of the module must be built before others in order to avoid circular dependencies and in order for data-binding to work.

In this PR, I am building the module's view models and views - which require certain listeners, resources, and annotation files (built in their own libraries). You will notice that I am building a resources library that excludes all layout resource files. This is necessary because layout files depend on view models, views, and binding adapters while binding adapters depend on view models.

See the graph below:
Screen Shot 2020-08-09 at 5 06 48 PM
This is a visual representation of the libraries app module will have and how they will depend on eachother. The libraries in green are libraries implemented in this PR. The libraries in grey are implemented in PRs to follow.

Here is a quick summary of each library's function:

  • listeners: Builds any listener files needed for views or view models
  • annotations: Builds any scope annotation or helper files needed for the views or view models
  • resources: Builds non-layout resource files
  • views: Builds view files
  • view models: Builds view model files
  • databinding_adapters: Builds the custom binding adapter files under org/oppia/app/databinding/
  • recyclerview_adapters: Builds the custom binding adapter files under org/oppia/app/recyclerview/
  • databinding_resources: Builds layout files
  • app: The complete app module library

NOTES:

  1. Gradle does not build with this implementation yet but there are plans to write a custom macro to fix this issue.
  2. If you have not set up Bazel for data-binding in Oppia Android, please follow the instructions laid out in this document.

To build views, run

~/bazel-databinding-new/bazel-bin/src/bazel build //:views --android_databinding_use_v3_4_args --experimental_android_databinding_v2 --override_repository=android_tools=/tmp/android_tools --java_header_compilation=false

To build view_models, run

~/bazel-databinding-new/bazel-bin/src/bazel build //:view_models --android_databinding_use_v3_4_args --experimental_android_databinding_v2 --override_repository=android_tools=/tmp/android_tools --java_header_compilation=false

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

miaboloix added 30 commits July 24, 2020 11:30
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @miaboloix! This looks pretty close to done. Just had a few more comments.

@BenHenning BenHenning assigned miaboloix and unassigned BenHenning Aug 18, 2020
@miaboloix miaboloix assigned BenHenning and unassigned miaboloix Aug 19, 2020
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @miaboloix! Overall LGTM, but the questions piece needs to be finished.

@BenHenning BenHenning assigned miaboloix and unassigned BenHenning Aug 19, 2020
@BenHenning
Copy link
Member

Also @miaboloix is this still blocked on #1568 per the title?

@miaboloix miaboloix changed the title Fixes part of #59: Build app module views + view models with Bazel [BLOCKED: #1568] Fixes part of #59: Build app module views + view models with Bazel Aug 19, 2020
@miaboloix
Copy link
Contributor Author

Also @miaboloix is this still blocked on #1568 per the title?

#1568 was merged, so it is unblocked now! I will rename it.

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @miaboloix, LGTM!

@BenHenning BenHenning assigned miaboloix and unassigned BenHenning Aug 19, 2020
@miaboloix miaboloix merged commit 71def98 into develop Aug 19, 2020
@miaboloix miaboloix deleted the build-app-viewmodels-bazel branch August 19, 2020 21:39
@anandwana001
Copy link
Contributor

@miaboloix Are we intentionally removing the vcs.xml file?
commit - 7a20fe1

@miaboloix
Copy link
Contributor Author

miaboloix commented Aug 20, 2020

@miaboloix Are we intentionally removing the vcs.xml file?
commit - 7a20fe1

No - this is a mistake. I meant to remove the file from the change list but I must have deleted it instead. Can I just revert this commit?

@miaboloix miaboloix restored the build-app-viewmodels-bazel branch August 20, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants