-
Notifications
You must be signed in to change notification settings - Fork 607
Add a android log implementation #10938
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10938
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit bc62fa4 with merge base 9aedbeb ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
"DEFAULT": _select_pal({ | ||
"minimal": ["default/minimal.cpp"], | ||
"posix": ["default/posix.cpp"], |
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'm wondering if this should be another "android" entry
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.
@larryliu0820 that would require users to manually set -c executorch.pal_default=android. I think what @kirklandsign is doing here is the best way forward because it's automatic. However, it might also cause some confusion for users since executorch.pal_default
is now ignored
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.
However, it might also cause some confusion for users since executorch.pal_default is now ignored
That makes sense, but ovr_config//os:android is automatic.
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
runtime/platform/targets.bzl
Outdated
deps = [ | ||
":pal_interface", | ||
"fbsource//third-party/toolchains:log", |
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.
This will break in OSS buck right? Can you also hide it under ovr_config android?
Summary: Pull Request resolved: #10938 Differential Revision: D74865527 Pulled By: kirklandsign
1063301
to
0c7b64d
Compare
This pull request was exported from Phabricator. Differential Revision: D74865527 |
Summary: Pull Request resolved: #10938 Differential Revision: D74865527 Pulled By: kirklandsign
0c7b64d
to
e5c234d
Compare
This pull request was exported from Phabricator. Differential Revision: D74865527 |
Summary: Pull Request resolved: #10938 Differential Revision: D74865527 Pulled By: kirklandsign
e5c234d
to
9a62d7b
Compare
This pull request was exported from Phabricator. Differential Revision: D74865527 |
Summary: Pull Request resolved: #10938 Differential Revision: D74865527 Pulled By: kirklandsign
9a62d7b
to
01a99a5
Compare
This pull request was exported from Phabricator. Differential Revision: D74865527 |
Summary: Pull Request resolved: #10938 Differential Revision: D74865527 Pulled By: kirklandsign
01a99a5
to
bc62fa4
Compare
This pull request was exported from Phabricator. Differential Revision: D74865527 |
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.
build changes lgtm
No description provided.