-
Notifications
You must be signed in to change notification settings - Fork 107
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
APP-4053 tflite and more APIs for android builds #3647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question about what this PR should contain -- should web/cmd/droid/droid.go
also be included? make droid-rdk.aar
won't work without it
|
||
droid-rdk.aar: | ||
# creates an android library that can be imported by native code | ||
gomobile bind -v -target $(APK_ARCH) -androidapi 28 -tags no_cgo -o $@ ./web/cmd/droid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying this PR, I try to do make droid-rdk.aar
and I get the following error:
gomobile bind -v -target android/arm64,android/amd64 -androidapi 28 -tags no_cgo -o droid-rdk.aar ./web/cmd/droid
no exported names in the package "./web/cmd/droid"
There are no files in web/cmd/droid in this PR, is droid.go
supposed be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack -- was going to do that as a follow up PR to make the review smaller
thanks for catch
I'll add it, + test the command before resubmitting
@@ -8,6 +8,7 @@ import ( | |||
_ "go.viam.com/rdk/components/encoder/register" | |||
_ "go.viam.com/rdk/components/gantry/register" | |||
_ "go.viam.com/rdk/components/generic/register" | |||
_ "go.viam.com/rdk/components/gripper/register" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this changed? There is no change to the gripper code in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it from register/all_cgo.go
(I think we needed it for building a sample)
the original no_cgo
port painted with a broad brush, and I've been selectively adding things back as needed
gostream/stream_cgo.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this gostream change supposed to be included too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes -- all of gostream is currently excluded from normal RDK builds in no_cgo
mode (it isn't registered). that is pending x264 build for android
the goal of this change is to remove malgo as a dependency of gostream, because we're not planning to port yet
this should be zero-impact for normal (non-android) builds because stream_cgo.go
is included in them
(added confusion here is that 'no_cgo' now essentially means android, and includes some cgo when we know how to cross compile it)
closing -- done elsewhere |
What changed
(This PR is a partial merge from our fork to shrink the diff)
Why
Expanding android support