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: Model module builds with both Bazel + Gradle #1481

Merged
merged 39 commits into from
Aug 5, 2020

Conversation

miaboloix
Copy link
Contributor

@miaboloix miaboloix commented Jul 17, 2020

Explanation

Fixes part of #59: Model module builds with both Bazel + Gradle
As part of the first step towards migrating to Bazel, I have added a root WORKSPACE file that imports the external dependencies necessary to begin building with Bazel. I have also added both a root BUILD file and a /model module BUILD file.

To build the /model module in Bazel: bazel build //model

Note:

  1. Gradle seems to fail when Bazel build artifacts are present. Be sure to remove them before building Gradle by running bazel clean
  2. Bazel convention states build files should be named BUILD but because we are working alongside Gradle, following this convention causes naming conflicts with existing Gradle files. So, to solve this, all BUILD files will be named BUILD.bazel until we are completely moved off of Gradle. See Change Bazel BUILD file names post-Gradle #1532.
  3. Here is a link to instructions on how to set up Bazel: https://docs.google.com/document/d/1V0RMehfi3GLVsZ6dyin1nTo4_TO28LACp4wPlEnvnM0/edit?usp=sharing.

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.

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! Do we need the top-level BUILD.bazel file?

WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
model/BUILD.bazel Show resolved Hide resolved
model/BUILD.bazel Outdated Show resolved Hide resolved
model/BUILD.bazel Outdated Show resolved Hide resolved
model/src/main/proto/preprocess_protos.bzl Outdated Show resolved Hide resolved
model/src/main/proto/preprocess_protos.bzl Outdated Show resolved Hide resolved
model/src/main/proto/preprocess_protos.bzl Outdated Show resolved Hide resolved
@BenHenning
Copy link
Member

Also: branch names should follow the format:

<action-verb>-and-additional-details

E.g. update-state-fragment-tests or stabilize-app-startup-state-controller-tests

@BenHenning BenHenning assigned miaboloix and unassigned BenHenning Jul 18, 2020
@miaboloix miaboloix assigned BenHenning and unassigned miaboloix Jul 21, 2020
@miaboloix miaboloix assigned anandwana001 and unassigned miaboloix Jul 28, 2020
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM
just a nit update, update the PR description from model_lib to model as we had updated the android_library name = "model",

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.

Just one comment.

model/src/main/proto/format_import_proto_library.bzl Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned miaboloix and unassigned BenHenning Jul 29, 2020
@BenHenning
Copy link
Member

@miaboloix I count 5 open threads that require a response and potentially some follow-up. Please re-assign once done.

Also regarding your earlier question, let's keep the branch name as-is & use the established convention for future branches.

@miaboloix
Copy link
Contributor Author

LGTM
just a nit update, update the PR description from model_lib to model as we had updated the android_library name = "model",

Done!

@miaboloix
Copy link
Contributor Author

miaboloix commented Jul 29, 2020

Thanks @miaboloix! Do we need the top-level BUILD.bazel file?

Technically not until we start working on the binary. Just deleted it!

@miaboloix miaboloix assigned BenHenning and unassigned miaboloix Jul 29, 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! This LGTM!

@BenHenning BenHenning assigned miaboloix and unassigned BenHenning Jul 30, 2020
@anandwana001 anandwana001 removed their assignment Jul 30, 2020
@miaboloix miaboloix merged commit 61f99cc into develop Aug 5, 2020
@miaboloix miaboloix deleted the stage1_bazel branch August 5, 2020 14:15
miaboloix added a commit that referenced this pull request Aug 9, 2020
…+ Gradle [BLOCKED: #1481] (#1482)

* Working on having one module build with bazel

* Created initial app-level WORKSPACE file

* Added proto_library rule to build model app module

* Added newline at end of WORKSPACE file

* Created macro to process proto files

* Both Bazel and Gradle now build /model successfully

* Fixed typo in BUILD

* Added missing end of file empty lines

* Added Robolectric dependencies and general build rule

* Source files building in both systems - added a TODO for test files

* Turned two kt_android_library rules into one

* Added dependencies for test files

* Added a test manifest for android_local_test()

* Added Firebase dependencies and git_repository for tools_android

* Refactored google-services.json, imported new dependencies

* Fixed Bens nits

* Fixed space in WORKSPACE comment

* Got a demo test working in both Java and Kotlin

* Added rules_java dependencies for protocol buffers

* Added java_proto_library rules, each proto file now has its own rule

* Remove unnecessary srcs attribute for android_library

* Rename bzl file macro

* Changed to java_lite

* Each library now has its own build rule

* Utility is now one rule

* Changed visibility for utility_lib

* Added duplicate google-services.json file to please Gradle

* Changed event_logger.proto to oppia_logger.proto

* Added missing EOF newlines

* Fixed bug in import statements for exploration.proto, topic.proto, and question.proto

* Deleted DemoJava

* CHanged rules_kotlin version

* Added Firebase dependencies

* Removed AsyncResultTest example

* Removed unnecessary comments in WORKSPACE and moved rules_kotlin

* Added re-naming TODOs

* Renamed java_proto rules java_proto_lite

* Added doc comments to model/BUILD.bazel and format_import_proto_library

* Added comments to WORKSPACE file

* Fixed nits

* Added comment to kt_android_library() rule

* Fixed more nits

* Added Firebase comment

* Changed library name to model

* Formatted WORKSPACE comment

* Formatted TODO statement

* Changed format_import_proto_library comment

* Added EOF newline

* Created Issue and linked TODO in WORKSPACE

* Edited model BUILD file top comment

* Addressed nits

* Fixed nits and added comments

* Fixing nits

* Deleted unnecessary dependencies and testing example file

* Fixed manifest issues

* Move google json file

* Restore app version of json

* Edit TODO

* Deleted TODO

* Changed Firebase comment

* Added targetSDK to manifests

* Changed crashlytics_lib to crashlytics

* Fixed nits and added TODOs

* Changed targetSDK to 29

* Formatted TODO

* Moved google-services.json

* Added gogle-services.json back to app

* Formatted dependencies and removed unused dependency

* Updated dependency list

* Fixed the duplicate google-services.json issue

* Created Issue and added TODO

* Changed targetSdk to 28
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.

3 participants