-
Notifications
You must be signed in to change notification settings - Fork 26
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
build(deps): workflow concurrency limit, JDK11, command-line-tools latest, API31 #178
Conversation
@@ -1,4 +1,7 @@ | |||
: ${1?"Usage: $0 NDK Version (ex 21.3.6528147)"} | |||
|
|||
echo "installing NDK $1" | |||
echo "y" | $(sudo $ANDROID_SDK_ROOT/tools/bin/sdkmanager --install "ndk;$1" --sdk_root=${ANDROID_SDK_ROOT}) | grep -v = || true | |||
echo $PATH | |||
sdkmanager --list_installed|grep ndk |
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.
- sudo just isn't necessary
- we need a relative PATH to get the new command line tools version of sdkmanager
@@ -5,6 +5,10 @@ on: | |||
- cron: "0 0 * * *" | |||
pull_request: | |||
|
|||
concurrency: | |||
group: ${{ github.workflow }}-${{ github.ref }} |
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 you implement concurrency limits anywhere else I think this is the simplest definition for it, github.ref is the branch or tag name that triggered it, workflow is the workflow name, and that's the basic unit I want to cancel if a new run is triggered, in all the repos I work on
- name: Install NDK | ||
run: .github/scripts/install_ndk_macos.sh r22 22.0.7026061 | ||
run: .github/scripts/install_ndk.sh 22.0.7026061 |
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.
we don't need a mac-specific NDK install now because the sdkmanager is updated and works same on linux+macos
java-version: "11" # minimum for Android API31 | ||
|
||
- name: Install Android Command Line Tools | ||
uses: mikehardy/setup-android@main |
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.
my only reason to fork the action was to update the sdkmanager version it installed, I'll PR that back up stream
println "**************************************************************************************************************" | ||
println "\n\n\n" | ||
println "ERROR: Anki-Android-Backend builds with JVM version 11, 14, or 16." | ||
println " Incompatible major version detected: '" + jvmVersion + "'" |
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.
There was a really subtle error on mac where we ran gradle with shell bash -l {0}
or something, and that wiped out the PATH so the new JDK wasn't being used and it kept getting the default 8
Would never have known it if it were not for this verification block I implemented originally in AnkiDroid!
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.
🤦 just got caught in a different part of the workflow with that bash shell cruft. I grepped and deleted all of it, there is no need in these workflows to specify a shell, it is correct and minimal for us to just run in default shells
API31 can't be attempted until JDK11 is in place
331c9a6
to
32a315f
Compare
This PR began life as an attempt to update sqlite dep from 2.1.0 to 2.2.0, but that needs targetSdkVersion 31
...which needs JDK11
...which needs an up to date set of command line tools
...which alters the way the NDK should install just a bit (pathing, mostly but also simplifies it)
And it required so many workflow runs that I implemented workflow concurrency limits in the process
I'll leave some notes in the actual files where things are interesting, but it's pretty thoroughly tested, will merge if CI is good