-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
Reusable Docker image #29
Conversation
Must admit i didn't do the local building stuff so i dont really know how it works |
I haven't had a chance to try this yet as I haven't rebased to V2.0. (When was V2.0 "released", even?) From reading, this PR looks great. +1 to using |
they created a v2.0 branch, but nothing is tagged... which is a little confusing, because how do you even know what v2.0 is if they're pushing new patches to that branch??? anyways, maybe it's not meant to be "released" yet? |
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.
Commenting mostly out of curiosity, feel free to ignore
@@ -13,7 +11,6 @@ RUN west update | |||
# West Zephyr export | |||
RUN west zephyr-export |
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.
Might be out-of-scope, and I'm not familiar with west
, but might it make sense to not have these as multiple layers1? Only bringing up since this is about improving the image building process.
Footnotes
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.
Oh, interesting. I combined these RUN
lines but it did not make a difference in the image size; apparently it isn't doing too much in the way of transient files? I also don't know enough about west
to say which of those commands are likely to bust the layer cache, so I'm inclined to keep them individually cache-able.
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.
Ha, my very next run demonstrated this in action:
=> CACHED [4/7] RUN west init -l config
=> [5/7] RUN west update
The rule you linked to makes the tradeoff clear and I'm ok optimizing for speed in this case.
Putting easy cacheable and non-cacheable commands in the same layer (by using one RUN command) might have negative performance issues.
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.
O sorry, it shouldn't do anything for the size/speed, should just effect the layers that Docker generates. I was thinking it may be a case similar to https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#apt-get (with subcommands like init and update).
Can avoid issues where earlier stages succeed & cached, resulting in later stages to have a(n) inconsistent/un-reproducible/non-updating state. I.e. they're actually coupled stages and should be apart of the same RUN
Dockerfile
Outdated
@@ -1,7 +1,5 @@ | |||
FROM zmkfirmware/zmk-build-arm:2.4 |
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 had to apply #25 for v2 branch
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.
same, i just omitted it here since there's an open PR for it.
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.
Pr has since been merged btw
Co-authored-by: Ed Flanagan <ed@flanagan.xyz>
Let's get this merged so we can add podman support (revised PR #10). |
🎉 |
Works great. Josh, can you test if more a more qualified url |
@allanwind Looks good:
|
Thanks bud! |
I'm sure there are considerations that my own preferred workflow doesn't account for, so consider this PR more as a starting point for further discussion. But in brief, it's not strictly necessary to rebuild the Docker image if all you've updated is your keymap. This PR creates a reusable Docker image that can persist between firmware builds by simply mounting the
config
andfirmware
directories at runtime.Pros:
zmk
container is left around and needs to be manually removed. Usingdocker run --rm ...
ensures the container is always cleaned up regardless of success.Cons:
setup.sh
every time is that zmk/west are always up to date. Making the Docker rebuild an optional step puts the responsibility on the end user to update those deps by manually rebuilding the image.make
to the mix, although if someone is already willing to run Docker locally I think it's a safe bet they have some familiarity withmake
.Ultimately you could just instruct people to run
make all
(or even justmake
) and they'd get the same experience they do currently; this just adds a little more control over the build process for people who want it.