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

Android CHIP demo app #1412

Merged
merged 1 commit into from
Jul 7, 2020
Merged

Conversation

vidhis88
Copy link
Contributor

@vidhis88 vidhis88 commented Jul 2, 2020

Scan CHIP QR code and display the SetupPayload info.

Video: https://drive.google.com/file/d/1BqqjPsQItrdNCNtfjfR5W4zMKUVkiTpy/view

@pan-apple
Copy link
Contributor

Are the following files build artifacts being accidentally checked in?

src/android/CHIPTool/app/src/main/jniLibs/arm64-v8a/libcrypto.so
src/android/CHIPTool/app/src/main/jniLibs/arm64-v8a/libSetupPayloadParser.so
src/android/CHIPTool/app/src/main/jniLibs/x86/libSetupPayloadParser.so
src/android/CHIPTool/app/src/main/jniLibs/x86/libcrypto.so

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Are the following files build artifacts being accidentally checked in?

src/android/CHIPTool/app/src/main/jniLibs/arm64-v8a/libcrypto.so
src/android/CHIPTool/app/src/main/jniLibs/arm64-v8a/libSetupPayloadParser.so
src/android/CHIPTool/app/src/main/jniLibs/x86/libSetupPayloadParser.so
src/android/CHIPTool/app/src/main/jniLibs/x86/libcrypto.so

Maybe we could use some $NDK_PATH in our build scripts for the crypto bit. For the PayloadParser, it sounds as if our build should create that file and we should not need to check it in.

src/android/CHIPTool/app/build.gradle Outdated Show resolved Hide resolved
Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Marking as changes requested: I believe we should not checkin .so files and we should instead require user paths if we cannot autodetect (e.g. chip build output directory or NDK path).

Copy link
Contributor

@gerickson gerickson left a comment

Choose a reason for hiding this comment

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

Looks good, but agreed we do not want to check in the .so binaries.

@randyrossi
Copy link
Contributor

Same goes for .jar files. Any artifacts of the build should not be checked in.

@woody-apple
Copy link
Contributor

@vidhis88 @randyrossi any update here?

@vidhis88 vidhis88 force-pushed the qrdemo-android branch 2 times, most recently from 8903cc2 to 703aa72 Compare July 6, 2020 21:07
@vidhis88
Copy link
Contributor Author

vidhis88 commented Jul 6, 2020

Updated the PR to remove the .so and .jar files.

Since we don't yet have a build job for the Android app, I wanted to allow anyone checking out the Android demo app to be able to run it without having to manually build the .so and .jar files by including them.
But I do see the point of muddying git history and have updated the PR accordingly.

@vidhis88 vidhis88 requested review from gerickson and andy31415 July 6, 2020 21:23
Scan CHIP QR code and display the SetupPayload info.
@woody-apple woody-apple merged commit 6dbfdb6 into project-chip:master Jul 7, 2020
@vidhis88 vidhis88 deleted the qrdemo-android branch July 7, 2020 16:03
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.

7 participants