Add cross-platform NDK repository support#122
Conversation
eaafff8 to
4ab8198
Compare
4ab8198 to
151b919
Compare
|
@ted-xie ping here. Some form of this would be helpful and am happy to adjust the design! |
|
Sorry, I didn't see this until you closed the PR. I had some comments. |
| doc = "A repository rule that integrates the Android NDK from a workspace. Uses an anchor label to locate the NDK and requires the host platform and Clang resource directory to be specified. For local NDK installations, use local_android_ndk_repository instead.", | ||
| implementation = _android_ndk_repository_impl, | ||
| attrs = _COMMON_ATTR | { | ||
| "anchor": attr.string( |
There was a problem hiding this comment.
I'm confused how this is supposed to work. The NDK distributables are separated by platform: https://developer.android.com/ndk/downloads
So how would you be able to have multiple platforms' NDKs installed in the same directory tree?
There was a problem hiding this comment.
I'm not sure what you mean. This is a stand-in for the value of ANDROID_NDK_HOME. Could you describe the dependency tree you're referring to?
There was a problem hiding this comment.
Let's say you're on a Mac host building for Linux RBE. On your Mac, you'd have a Mac NDK installed. Where would the files for the Linux NDK come from? You'd need to tell the Linux RBE where those are, somehow.
There was a problem hiding this comment.
I have installed the Linux NDK to another directory on the Mac to support this
There was a problem hiding this comment.
Okay. This would be an important detail to leave in a comment or doc somewhere, since it's not obvious.
On this topic: Can you think of a better way to have an end-to-end test for this feature? Maybe use rules_bazel_integration_test?
There was a problem hiding this comment.
Sorry, I didn't have time to address this before you merged (thank you by the way). I've created #128
ted-xie
left a comment
There was a problem hiding this comment.
I'm kind of wondering if this PR is needed at all. Is it possible to register multiple NDKs in a single main workspace and have Bazel automatically pick the correct NDK based on execution platform?
rules.bzl
Outdated
|
|
||
| return _android_ndk_repository_impl(ctx, ndk_path) | ||
|
|
||
| local_android_ndk_repository = repository_rule( |
There was a problem hiding this comment.
Let's just keep this rule name as android_ndk_repository. Same with its impl function at L208.
rules.bzl
Outdated
|
|
||
| return _android_ndk_repository_impl(ctx, ndk_path) | ||
|
|
||
| remote_android_ndk_repository = repository_rule( |
There was a problem hiding this comment.
The "remote" part of this rule is kind of inaccurate, since you still have to store the exec configuration's NDK locally. Maybe rename this to "exec_configuration_android_ndk_repository", or something more concise.
There was a problem hiding this comment.
Done
I disagree but ultimately do not care. local to me mapped to the fact that the repository rule looked for a path directly on the host where remote referred to another repository. Though as you see they ultimately do the same thing after locating the desired ndk root.
There was a problem hiding this comment.
When we say "remote execution", that means a machine that is not the one that launched Bazel.
The "remote" repository as you had originally named it was not remote at all. It would have been confusing terminology for future users of this feature.
examples/basic/WORKSPACE
Outdated
|
|
||
| # --SNIP--: Everything below this lines goes into the example WORKSPACE snippet in the release notes. | ||
| load("@rules_android_ndk//:rules.bzl", "android_ndk_repository") | ||
| load("@rules_android_ndk//:rules.bzl", "local_android_ndk_repository") |
There was a problem hiding this comment.
Please change the loads for local_android_ndk_repository back to android_ndk_repository.
I would rather the decision be based on the Bazel configuration than some explicit command line flag. I know of no other way to accomplish what you're suggesting what you're suggesting. It's otherwise an easy way for users to run the wrong invocation and run into confusion or for there to be additional infrastructure added to consuming repositories. |
|
Code looks good. I would have also liked some documentation for this new rule you've introduced, and a real integration test. Please consider those as future work to polish off this feature. |
This change enables workflows where the host Bazel platform differs from the exec platforms via remote execution. When creating a
local_repositoryto an NDK for another platform, the repository rules are now flexible and platform-agnostic enough to support correctly matching foreign exec platforms if explicitly specified.