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

Fix #3277: Created BUILD.bazel for domain/state #3694

Merged

Conversation

yash10019coder
Copy link
Contributor

@yash10019coder yash10019coder commented Aug 17, 2021

Explanation

Fixes #3277
Created a BUILD.bazel file in domain state

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.

@yash10019coder
Copy link
Contributor Author

yash10019coder commented Aug 17, 2021

@fsharpasharp @anandwana001 @prayutsu created a PR but unable to add dependencies in BUILD.bazel please help

@yash10019coder
Copy link
Contributor Author

All the dependencies in the domain/utility classes are of model and it has no bazel packages for it therefore I'm haveing trouble including the dependencies of the domain/utility classes

@BenHenning
Copy link
Sponsor Member

@yash10019coder you'll need to add dependencies from https://github.com/oppia/oppia-android/blob/develop/model/BUILD.bazel, e.g.: //model:exploration_java_proto_lite for each of the protos that are used by these classes. I think Exploration gives you most of them, but you might need a few more.

…bazel/state in global of bazel/domain in kt_android_library
@yash10019coder
Copy link
Contributor Author

@yash10019coder you'll need to add dependencies from https://github.com/oppia/oppia-android/blob/develop/model/BUILD.bazel, e.g.: //model:exploration_java_proto_lite for each of the protos that are used by these classes. I think Exploration gives you most of them, but you might need a few more.

@BenHenning I have added these dependencies in my latest commit but I still get some errors and how did you found out that we have to put this in dependencies //model:exploration_java_proto_lite three is no mention of the java files in it

and I'm attaching the screenshot of the error

Screenshot from 2021-08-21 10-13-22

@prayutsu
Copy link
Contributor

@yash10019coder you'll need to add dependencies from https://github.com/oppia/oppia-android/blob/develop/model/BUILD.bazel, e.g.: //model:exploration_java_proto_lite for each of the protos that are used by these classes. I think Exploration gives you most of them, but you might need a few more.

@BenHenning I have added these dependencies in my latest commit but I still get some errors and how did you found out that we have to put this in dependencies //model:exploration_java_proto_lite three is no mention of the java files in it

and I'm attaching the screenshot of the error

Screenshot from 2021-08-21 10-13-22

@yash10019coder To find out what deps need to be added, you can check the imports of the file StateDeck.kt as imports tell us what other classes are we using in that file.
If you look at the error carefully, it says unresolved reference which means that Bazel does not know about those particular classes when it tries to build these newly introduces libraries. If you want to tell Bazel about these classes, then you need to add those classes in the deps as well.

Copy link
Contributor

@prayutsu prayutsu left a comment

Choose a reason for hiding this comment

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

@yash10019coder Left some comments, PTAL.

</set>
</option>
</component>
</project>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please restore this file.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It appears this file was removed again--please restore it.

visibility = ["//:oppia_api_visibility"],
deps = [
"//model:exploration_java_proto_lite"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Add all missing deps that are being highlighted in the build error.
Ditto for other added libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prayutsu that I have understood but the main problem is how to find which proto is to be added
like AnswersAndResponse.java is being used in StateDeck but now how can I find the particular of this in bazel/model

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to find the proto file that contains the message AnswerAndResponse in it and then you can specify the library of that file here.
To find the .proto file just press Ctrl + Shift + R and type the keywords that you want to search in the whole codebase.

Copy link
Contributor

@prayutsu prayutsu left a comment

Choose a reason for hiding this comment

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

@yash10019coder Followed up on our comment.

visibility = ["//:oppia_api_visibility"],
deps = [
"//model:exploration_java_proto_lite"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to find the proto file that contains the message AnswerAndResponse in it and then you can specify the library of that file here.
To find the .proto file just press Ctrl + Shift + R and type the keywords that you want to search in the whole codebase.

@yash10019coder
Copy link
Contributor Author

@prayutsu @fsharpasharp made a new commit PTAL
I'm still getting errors althought I have added all the dependencies
attaching the SS below

Screenshot from 2021-08-22 10-10-23
Screenshot from 2021-08-22 10-10-37
Screenshot from 2021-08-22 10-10-51
Screenshot from 2021-08-22 10-11-12
Screenshot from 2021-08-22 10-11-21
Screenshot from 2021-08-22 10-11-28

@oppiabot oppiabot bot assigned fsharpasharp and prayutsu and unassigned yash10019coder Aug 22, 2021
@oppiabot
Copy link

oppiabot bot commented Aug 22, 2021

Unassigning @yash10019coder since a re-review was requested. @yash10019coder, please make sure you have addressed all review comments. Thanks!

Also removed the internal keyword from StateDeck.kt StateGraph.kt and StateList.kt
Copy link
Contributor Author

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

@BenHenning resolved the comments

@yash10019coder
Copy link
Contributor Author

@BenHenning PTAL

@oppiabot oppiabot bot assigned BenHenning and unassigned yash10019coder Sep 21, 2021
@oppiabot
Copy link

oppiabot bot commented Sep 21, 2021

Unassigning @yash10019coder since a re-review was requested. @yash10019coder, please make sure you have addressed all review comments. Thanks!

Copy link
Sponsor 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 @yash10019coder. Just 2 new comments.

Similar to my comment in your other PR, please make sure to respond via a comment to all conversation threads from reviewers to confirm you saw them & addressed/didn't address them.

Copy link
Sponsor 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 @yash10019coder. Justhad one follow-up.

@BenHenning BenHenning removed their assignment Sep 29, 2021
@yash10019coder
Copy link
Contributor Author

@BenHenning removed unused and unncesscarty imports from the bazel build file PTAL

Added some deps in glob due to failing tests these changes happened due to merge conflict
Copy link
Sponsor 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 @yash10019coder. This LGTM!

@BenHenning
Copy link
Sponsor Member

@prayutsu it seems you requested changes. Could you PTAL?

@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Oct 5, 2021
Copy link
Contributor

@prayutsu prayutsu left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @yash10019coder

@prayutsu prayutsu assigned BenHenning and unassigned prayutsu Oct 5, 2021
@prayutsu
Copy link
Contributor

prayutsu commented Oct 5, 2021

@BenHenning This is ready to merge now 😄

@oppiabot oppiabot bot added the PR: LGTM label Oct 5, 2021
@BenHenning
Copy link
Sponsor Member

Thanks @prayutsu & @yash10019coder.

@BenHenning BenHenning merged commit 25a7670 into oppia:develop Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a BUILD.bazel file for domain/state
5 participants