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 #4644: Set custom protobuf artifact for macOS #4675

Merged
merged 4 commits into from
Oct 31, 2022

Conversation

adhiamboperes
Copy link
Collaborator

@adhiamboperes adhiamboperes commented Oct 27, 2022

Explanation

Fix #4644:

Execution failed for task ':model:generateProto'.

Could not resolve all files for configuration ':model:protobufToolsLocator_protoc'.
Could not find protoc-3.8.0-osx-aarch_64.exe (com.google.protobuf:protoc:3.8.0).
Searched in the following locations:
https://repo.maven.apache.org/maven2/com/google/protobuf/protoc/3.8.0/protoc-3.8.0-osx-aarch_64.exe`

Changed the conditional check for the protobuf_platform property from checking for property presence to checking for the current os. This is because project.rootProject.hasProperty('protobuf_platform') does not actually read the properties file as it is meant to, therefore the if condition was never met and the artifact was never set correctly.

I tested the existing implementaion by adding in build.gradle:
println project.rootProject.property('protobuf_platform')
This throws an error:
Caused by: groovy.lang.MissingPropertyException: Could not get unknown property 'protobuf_platform' for root project 'oppia-android' of type org.gradle.api.Project.
Which shows that the current solution for reading the property from local.properties file does not work correctly.

This solution changes how the property is read by loading the local.properties file with inputstream.

Essential Checklist

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

Changed the conditional check for the 'protobuf_platform' property from checking for property presence to checking for the current os. This is because `project.rootProject.hasProperty('protobuf_platform')` does not actually read the properties file as it is meant to, therefore the if condition was never met and the artifact was never set correctly.

Changed how the property is read by loading the local.properties file with inputstream.
@adhiamboperes adhiamboperes changed the title Fix #4644 : Set custom protobuf artifact for m1 architecture Fix #4644 : Set custom protobuf artifact for macOs architecture Oct 28, 2022
@adhiamboperes adhiamboperes changed the title Fix #4644 : Set custom protobuf artifact for macOs architecture Fix #4644 : Set custom protobuf artifact for macOs Oct 28, 2022
@adhiamboperes adhiamboperes changed the title Fix #4644 : Set custom protobuf artifact for macOs Fix #4644 : Set custom protobuf artifact for macOS Oct 28, 2022
@BenHenning BenHenning self-assigned this Oct 29, 2022
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.

Nice catch! I just had one comment, but otherwise need to see CI fully passing before this can be merged. PTAL @adhiamboperes at the new comment.

model/build.gradle Outdated Show resolved Hide resolved
This should help prevent potential resource leaks should the file grow big.
@adhiamboperes
Copy link
Collaborator Author

adhiamboperes commented Oct 31, 2022

Nice catch! I just had one comment, but otherwise need to see CI fully passing before this can be merged. PTAL @adhiamboperes at the new comment.

I added a close operation to the inputStream. PTAL @BenHenning at the new commit.

@adhiamboperes adhiamboperes removed their assignment Oct 31, 2022
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 @adhiamboperes! This LGTM.

model/build.gradle Outdated Show resolved Hide resolved
@BenHenning BenHenning changed the title Fix #4644 : Set custom protobuf artifact for macOS Fix #4644: Set custom protobuf artifact for macOS Oct 31, 2022
@BenHenning BenHenning merged commit e4e4de6 into oppia:develop Oct 31, 2022
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.

Does not build with the default setup on M1.
2 participants