Check NDK version in Bazel #331
Conversation
hevrard
force-pushed
the
force-ndk-version
branch
from
a6c7e0a to
b9c900c
Compare
This makes sure we refuse to build with an invalid NDK version. Bug: b/158669059
hevrard
force-pushed
the
force-ndk-version
branch
from
b9c900c to
23f5d44
Compare
kokoro/presubmit/build.sh
Outdated
| # Mock the correct NDK version. | ||
| # Our Bazel scripts check that the correct NDK version is installed: rather | ||
| # than downloading the 1+GB NDK just to have a matching version to pass this | ||
| # presubmit test, we "mock" the version by overwriting it manually. | ||
| export ANDROID_NDK_HOME=/opt/android-ndk-r16b | ||
| cat > $ANDROID_NDK_HOME/source.properties <<EOF | ||
| Pkg.Desc = Android NDK | ||
| Pkg.Revision = 21.0.6113669 | ||
| EOF | ||
|
|
||
| # Setup environment. |
There was a problem hiding this comment.
What's going on here? if we are ok with an older ndk, why are we adding a check, and then by-passing it? FYI, we're already downloading the NDK on the build bots for the other OSes.
There was a problem hiding this comment.
This is only for kokoro/presubmit, where we don't really "build" anything. This avoids a 1GB download for the NDK on each presubmit, only to check the NDK version.
I agree this is hacky. Note that we did not download an up-to-date NDK on presubmit before this PR.
@pmuetschard do you think it is better to download 1GB on every presubmit for the sake of passing the version check?
There was a problem hiding this comment.
Pascal and I chatted offline: Pascal is keen on having this rule being called only when we need to compile anything using the NDK.
@pmuetschard , this NDK check needs Bazel's repository_ctx.read() to check the content of the source.properties file, and I guess this is only accessible in a repository rule?
If that is so, I think you told me it's possible to pass a flag to Bazel to set the android argument of gapid_dependencies() to false?
There was a problem hiding this comment.
Bazel has multiple phases of execution. I'm asking for the error failure to happen in a later phase. Alternatively, make it a warning rather than a failure.
You are already generating a dummy rule that we'll depend on. So, change the repository init code to create different dummies depending on whether it should fail or not. If it should fail, the dummy will then fail. That way it will only fail when the dummy is actually invoked.
hevrard
force-pushed
the
force-ndk-version
branch
from
d062494 to
ab68cff
Compare
hevrard
force-pushed
the
force-ndk-version
branch
from
ab68cff to
651192c
Compare
gapidapk/android/apk/rules.bzl
Outdated
| @@ -110,7 +110,7 @@ def gapid_apk(name = "", abi = "", pkg = "", libs = {}, bins = {}): | |||
| "//conditions:default": [], | |||
| }), | |||
| deps = select({ | |||
| "//tools/build:android-" + name: ["@ndk_vk_validation_layer//:" + abi], | |||
| "//tools/build:android-" + name: ["@ndk_version_check//:ndk_version_check", "@ndk_vk_validation_layer//:" + abi], | |||
There was a problem hiding this comment.
Fix the formatting here. Don't have multiple list items on a signle line.
kokoro/presubmit/build.sh
Outdated
| # Mock the correct NDK version. | ||
| # Our Bazel scripts check that the correct NDK version is installed: rather | ||
| # than downloading the 1+GB NDK just to have a matching version to pass this | ||
| # presubmit test, we "mock" the version by overwriting it manually. | ||
| export ANDROID_NDK_HOME=/opt/android-ndk-r16b | ||
| cat > $ANDROID_NDK_HOME/source.properties <<EOF | ||
| Pkg.Desc = Android NDK | ||
| Pkg.Revision = 21.0.6113669 | ||
| EOF | ||
|
|
||
| # Setup environment. |
There was a problem hiding this comment.
Bazel has multiple phases of execution. I'm asking for the error failure to happen in a later phase. Alternatively, make it a warning rather than a failure.
You are already generating a dummy rule that we'll depend on. So, change the repository init code to create different dummies depending on whether it should fail or not. If it should fail, the dummy will then fail. That way it will only fail when the dummy is actually invoked.
This enables to pass presubmit without having the expected NDK.
hevrard
force-pushed
the
force-ndk-version
branch
from
ca62425 to
c64542a
Compare
|
@pmuetschard thanks for the Bazel guidance, I think I reached the setup you described. |
|
@pmuetschard a friendly ping on this one |
|
Make sure to squash. |
|
FYI, our current version of bazel doesn't support 21 yet. I'm building with NDK 20 just fine, so I'm not sure why we think we need 21? |
|
Thanks, we need NDK r21 in order to have access to the unified validation layer, it's documented in https://github.com/google/agi/blob/master/BUILDING.md. Bazel does not officially support NDK > r20, we asked for this support with bazelbuild/bazel#10811. |
As I pointed out, it builds fine for me on 20, so I'm not sure why you say we need it. |
|
It wasn't clear from my message: the unified Khronos verification layer ships only from NDK r21, and if you build with NDK < r21, you don't get it, and things go wrong when you run a report replay -- this is the scenario that prompter to file b/158669059, and hence this fix. |
This makes sure we refuse to build with an invalid NDK version.
Bug: b/158669059