support using podman instead of docker #10
Conversation
There was a problem hiding this comment.
I suggest you write it like this instead:
container_cmd=$(command -v podman || command -v docker)
and I am new to podman so maybe I am missing something but I had to also set:
[ "$container_cmd" = "podman" ] && from='--from docker.io/zmkfirmware/zmk-build-arm:stable '
"$container_cmd" build ${from}--tag zmk .
The makefile is the way to go to bring build times down, though, so I suggest we get that merged first and then apply similar changed to it instead of the shell script.
|
Also it's not "instead of" but "preferred" if available. |
|
sounds good, I'll rework this merge request once the makefile thing is merged
On October 22, 2022 11:58:58 PM PDT, Allan Wind ***@***.***> wrote:
***@***.*** commented on this pull request.
…
I suggest you write it like this instead:
```
container_cmd=$(command -v podman || command -v docker)
```
and I am new to podman so maybe I am missing something but I had to also set:
```
[ "$container_cmd" = "podman" ] && from='--from docker.io/zmkfirmware/zmk-build-arm:stable '
"$container_cmd" build ${from}--tag zmk .
```
The makefile is the way to go to bring build times down, though, so I suggest we get that merged first and then apply similar changed to it instead of the shell script.
--
Reply to this email directly or view it on GitHub:
#10 (review)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
hmm, looks like something that would have been fixed by https://github.com/KinesisCorporation/Adv360-Pro-ZMK/pull/25/files, it's likely that I hadn't rebased this branch after that was merged... Anyways please take a look at the latest rev of this patch, it builds successfully for me using podman. (fwiw, podman can be used as a drop-in for docker, but had many benefits over using docker) |
|
No, I already had that change. There may be a way to tell podman to check non-qualified FROM using docker.io. I was also able to work-around it by adding an alias to /etc/containers/registries.conf.d/kinesis.conf which is not ideal: |
|
hmm, strange. which version of podman are you using? on 4.3 (and 4.2.x, which is what I used before today...), it's fine for me without any changes to |
|
I guess another "workaround" is to use the qualified FROM using docker.io to the Dockerfile... but again, this is working for me with podman so I'm not sure why it's not for you :( |
There was a problem hiding this comment.
Great.! The variable name is a little long, and it only really matters if you want to manually override it on the command line. DOCKER might be easier to remember than CONTAINER_CMD. podman is docker-compatible so although it's a little weird to call the variable DOCKER when it's set to podman perhaps it's better?
|
podman 3.0.1 (which is what ships with Debian 11) |
|
Please update the Dockerfile's FROM to read: @joshfrench confirmed it worked for docker in PR #29. |
This makes sure that all versions of podman can resolve the image successfully
yeah it's a little weird, but sometimes ergonomics > weirdness, amirite :P
aahh! maybe that's a "feature" of the older podman (inability to resolve non-qualified FROM 🤷🏽 ) thanks for all of the feedback, I've incorporated everything in the latest rev |
|
yeah I like the idea of preferring podman over docker if both are available, I can fixup the patch to do that
On October 24, 2022 10:41:25 AM PDT, Allan Wind ***@***.***> wrote:
***@***.*** approved this pull request.
…
Should we swap the order of podman and docker so podman is preferred if both are available? You can override it so so either way good to go as far as I am concerned.
--
Reply to this email directly or view it on GitHub:
#10 (review)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
|
Sorry i dropped the ball with this one, wasn't 100% sure on all this local building stuff as it's not really in my workflow, but very much like the look of the changes that have been made. will test locally asap to confirm all is good at my end then will look to merge :) |
|
Can I suggest you also add in the readme that this will support podman too? |
done 😄 |
|
This looks good to me now :) |
# This is the 1st commit message: Updated keymap # The commit message KinesisCorporation#2 will be skipped: # Updated keymap # The commit message KinesisCorporation#3 will be skipped: # feat: Add basic changes to make me feel more at home # The commit message KinesisCorporation#4 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#5 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#6 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#7 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#8 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#9 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#10 will be skipped: # Reduce brightness of leds # The commit message KinesisCorporation#11 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#12 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#13 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#14 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#15 will be skipped: # Updated adv360.keymap # The commit message KinesisCorporation#16 will be skipped: # Updated adv360.keymap
podman has many advantages over docker (e.g. rootless by default), this allows falling back to podman if the docker command is not present.