-
Notifications
You must be signed in to change notification settings - Fork 22
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
Use variables to allow overriding build tools #108
Use variables to allow overriding build tools #108
Conversation
0a951e3
to
150ea59
Compare
|
||
# Run the docker image. | ||
# TODO Would be better to just specify the file here? | ||
run_docker: build_docker | ||
./scripts/rundocker.sh | ||
env DOCKER="$(DOCKER)" ./scripts/rundocker.sh |
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.
Equivalently, you can export DOCKER
where it's set above like you would in a shell script. Otherwise this is basically the same as what I have locally to allow DOCKER=podman
. I kind of like the export
approach so you don't have to think about the places you want to pass it down, but there aren't many places it happens here so probably not a big deal.
@@ -21,6 +21,11 @@ ANDROIDNDKVER := 21.4.7075529 | |||
|
|||
SDK := ${ANDROID_HOME}/android-sdk-$(PLATFORM) | |||
|
|||
ADB := adb | |||
DOCKER := docker |
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.
Since we're handling DOCKER
as an environment variable and not just a make variable, I think it might be nice to change this to DOCKER ?= docker
. Then you could do export DOCKER=podman
in your shell and not have to pass it in every time.
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.
Looking this over again, I think it's fine the way you have it. However, I did find a couple spots you missed. Also, this will need to be rebased now since rundocker.sh is quite a bit different.
@@ -68,7 +73,7 @@ src/kolibri: clean | |||
|
|||
.PHONY: p4a_android_distro | |||
p4a_android_distro: needs-android-dirs | |||
p4a create --arch=$(P4A_ARCH) | |||
$(P4A) create --arch=$(P4A_ARCH) |
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.
You missed a couple of instances of p4a
below.
In particular, this is useful for a situation where we want to use podman instead of docker, or where adb is not in $PATH.
150ea59
to
ad389de
Compare
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 took the liberty of doing the rebase and fixing the p4a
issue since I was trying to do something on top of this. From my perspective this is good to merge.
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.
Looks good to me as well! Tested with podman
.
In particular, this is useful for a situation where we want to use
podman
instead ofdocker
, or whereadb
is not in$PATH
.