-
Notifications
You must be signed in to change notification settings - Fork 65
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
WIP: create base and base-x11 images, move common init functionality to them #76
Conversation
My testing of pulseaudio revealed:
|
It's effectively not doing anything though right? I agree on all the rest, most of the current settings are the result of experiments that lead to a "working" environment, we should probably revisit them and choose better defaults. |
I've just pushed a new version that includes a handful of small changes. Most notably, I refactored the build workflow into a reusable component that does the actual work of building/publishing, and a caller component that builds the images in the correct order, including in parallel where possible. Still blocking this merge:
I've been using my images for a week or so at this point and they seem to be working well. If anyone else has taken the time to give them a try, I'd love to hear your thoughts! |
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 really like the direction where this is heading; unfortunately I can't really try this on my test machine because seems like run-gow build
doesn't work for me..
.github/workflows/docker-build.yml
Outdated
docker: | ||
runs-on: ubuntu-latest | ||
base: | ||
uses: zb140/gow/.github/workflows/docker-build-and-publish.yml@refactor |
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.
This will have to be changed before merging
|
||
on: | ||
workflow_call: | ||
inputs: |
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.
Nice, I really like this.. We ended up never using the manual trigger but I like to be able to specify which image needs to be updated!
libnss3 \ | ||
" | ||
|
||
RUN apt-get update -y && \ |
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.
Should this also run apt-get upgrade
to update all the packages in the base?
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.
In the Dockerfile best practices guide, they used to recommend not running apt-get upgrade
inside a container, but they removed that comment about a year ago. I'm not sure why they got rid of it, but I think common practice is still to assume the base images are generally kept up to date. I'm happy either way though -- if we think it's better to run upgrade
I'll make the change.
images/base-x11/Dockerfile
Outdated
|
||
ARG DEBIAN_FRONTEND=noninteractive | ||
|
||
# TODO: should we include any drivers or anything here? The GL stuff that the |
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 think we need to do some research into this, (unless @ABeltramo knows) if every game containers needs the drivers, then it should go here, otherwise, if only xorg requires the drivers, then it should go there.
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 tried looking around on Google and couldn't really come to any obvious conclusions. I'm inclined to merge as-is and we can always move packages around in future revisions as we learn more about what the requirements are. What does everyone think? @ABeltramo @Sparticuz
…them * add some packages to enable AppImages inside containers * introduce a new reusable workflow to make building images in the correct order easier
517fdff
to
b2be2d3
Compare
I believe this change is pretty much ready to go, apart from the Sunshine PR getting merged. It's such a small change, hopefully it will get reviewed and merged soon. In the meantime, we could consider just merging this (which uses the branch with the bugfix directly) and switch back to "main" Sunshine after the merge. |
First off, thanks for all your work on this!! I have tested it, and it works perfectly on my system too. I think there are still a couple of things to fix before merging it: Build imagesUnless I'm missing something, there's no easy way to build images when someone doesn't want to pull from the registry or even for us to debug and test things. docker build -t ghcr.io/games-on-whales/base:edge --build-arg BUILD_OWNER=games-on-whales --build-arg BUILD_TAG=edge images/base
docker build -t ghcr.io/games-on-whales/base-app:edge --build-arg BUILD_OWNER=games-on-whales --build-arg BUILD_TAG=edge images/base-app
docker build -t ghcr.io/games-on-whales/xorg:edge --build-arg BUILD_OWNER=games-on-whales --build-arg BUILD_TAG=edge images/xorg
docker build -t ghcr.io/games-on-whales/pulseaudio:edge --build-arg BUILD_OWNER=games-on-whales --build-arg BUILD_TAG=edge images/pulseaudio
docker build -t ghcr.io/games-on-whales/sunshine:edge --build-arg BUILD_OWNER=games-on-whales --build-arg BUILD_TAG=edge images/sunshine
docker build -t ghcr.io/games-on-whales/steam:edge --build-arg BUILD_OWNER=games-on-whales --build-arg BUILD_TAG=edge images/steam There are possibly two problems here:
I'm not sure what's the best way to "fix" this, at the very least this should be documented, but maybe we should support it in the bash script; what do you guys think? Hardcoding ghcr.ioWe are currently pushing images both to the GitHub registry and Docker hub. |
Great points! For the build, how about a workflow that manually builds an image (including dependencies) that can be run locally with https://github.com/nektos/act ? That way we don't need to build our own dependency handling etc into a build script. I'm happy to add the registry as a build arg. If we build one image with |
That's one way of keeping it consistent, I like it. Can you add a couple of lines to the relative docs page?
The checksum should be based on the content of the images IIRC. |
Yeah, we're just building it once and tagging it for both. I was concerned that if we add a build arg for the registry it might end up getting built twice. But I suppose the "main" build would just pick one and always build with the same registry. I'll work on adding these changes (including the docs update) a bit later today. |
Famous last words 😉 I've had a super hectic couple of days, but I fully expect things to go back to normal-ish by this weekend. |
I actually ended up just making the whole base image name a build argument; this way, we don't have 3 separate args that get combined to make the image name, and it's much easier to build the base images locally and use them instead of our published images. I've also put lots of detail in the docs about how to build any or all of the images locally. Also, I started down the path of using https://github.com/nektos/act to build images locally, but I ran into a few issues:
I ended up adding a couple of environment variables (in I played around locally with building various combinations of base and app images, and it seems to be working well. Let me know what you think! |
First off, sorry for the time it took me to review the latest changes..
I just tried out this by following the few steps outlined in the docs, I like it a lot! I think this is the easiest way, and it makes perfect sense with how Docker works. All works on my test system too, are we finally ready to merge this? 😅 |
I made one small change to prevent an annoying but harmless warning about missing env vars for building your own images even when you're not trying to build them. I think it's ready to go! Unless someone objects, I'll probably go ahead and merge it this afternoon. |
I had so much fun refactoring the compose files, I did some refactoring on the
Dockerfile
s too 😁The biggest improvements are
base
and abase-x11
image. These are used to keep container init logic like creating users, ensuring device access, etc in one central location. They also provide a central place to change the "actual" base distro for all containers at the same time.ubuntu:22.04
. This fixes build issues associated withubuntu:21.04
being EOL.If anyone wants to give these images a try, I've published builds of them on my own GitHub account (eg
ghcr.io/zb140/gow-sunshine:refactor
).This PR is not quite ready for merge as-is. Some things that still need doing:
.github/workflows/docker-build.yml
in a couple of places. Most notably, I had to disable parallel builds because I couldn't figure out how to specify dependencies between matrix entries. If that turns out to be impossible, I'll need to figure out a different solution.Dockerfile
in this branch is currently set up to build the forked branch that contains the Pulseaudio crash fix. We'll probably want to switch that to something else, or else not merge this until after that bug is fixed.