-
Notifications
You must be signed in to change notification settings - Fork 3.7k
support adb-shell style cpp_rpc #8223
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
support adb-shell style cpp_rpc #8223
Conversation
|
Android Application test log: adb shell test log: |
|
please fix the format problem using clang-format-10 |
.gitignore
Outdated
| cmake-build-debug | ||
| cmake-build* |
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.
your pr should not be related with this .gitignore
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.
Should I modify .gitignore in another 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.
Yes, if you think it is a must
apps/cpp_rpc/rpc_env.cc
Outdated
| std::string android_base_ = "/data/data/" + std::string(cwd) +"/cache"; | ||
| struct stat statbuf; | ||
| if (stat(android_base_.data(), &statbuf) == -1 || !S_ISDIR(statbuf.st_mode)){ | ||
| android_base_ = "."; |
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.
If failed, android_base_ becomes ., will ./rpc work well on android?
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 current working directory is writable, Yes .
Another option is force base_ to /data/local/tmp/rpc. But I don't think this is a good choice. So I keep the original implementation .
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.
how about add one simple code comment to indicate users if failed, should make sure the directory is writable and also consider to change base_ to /data/local/tmp/rpc. This could help users find one solution more quickly
|
@FrozenGene All problems solved |
apps/cpp_rpc/rpc_env.cc
Outdated
| base_ = "/data/data/" + std::string(cwd) + "/cache/rpc"; | ||
| std::string android_base_ = "/data/data/" + std::string(cwd) + "/cache"; | ||
| struct stat statbuf; | ||
| // Check if application data directory exist. If not exist usually mean tvm_rpc run from adb |
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.
Nitty comment. If not exist, usually means we run tvm_rpc from adb shell terminal
apps/cpp_rpc/rpc_env.cc
Outdated
| // Check if application data directory exist. If not exist usually mean tvm_rpc run from adb | ||
| // shell terminal. | ||
| if (stat(android_base_.data(), &statbuf) == -1 || !S_ISDIR(statbuf.st_mode)) { | ||
| // Tmp directory always writable for 'shell' user. |
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 always writable...
|
@FrozenGene Thanks for correcting my spelling mistake [Lol] |
|
Thanks @fantasyRqg |
* support adb-shell style cpp_rpc * fix review problems, apache#8223 * add comment & use /data/local/tmp dir in shell terminal case * fix spelling errors * fix spelling errors Co-authored-by: rqg <[email protected]> Co-authored-by: rqg <[email protected]>
* support adb-shell style cpp_rpc * fix review problems, apache#8223 * add comment & use /data/local/tmp dir in shell terminal case * fix spelling errors * fix spelling errors Co-authored-by: rqg <[email protected]> Co-authored-by: rqg <[email protected]>
Fix problem mentioned at #7390
Support
tvm_rpcdirectly use inadb shellterminal@vinx13 @tqchen