-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Add cache to container build #35697
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
base: main
Are you sure you want to change the base?
Add cache to container build #35697
Conversation
correct permissions on copied files
Avoid copying .git directory into the container
instead of running two jobs compiling the same code, run one and reuse layers
drop platforms from dryrun
|
replaces #34876 and #27998 to a lesser degree. I've tried to trim down CI time further but I can either optimize for local or CI builds because dockers caching system allows to push layers to registry but it does not allow to share mounted cache in any way. I've tried using one action to do so but it did not work at all and maybe for the better as juggling the cache wouldn't be too fun if something went wrong. It would be possible to optimize for layers here (which I've sort of done) and have them pushed to registry on nightly builds then pulled during every other one but it then removes the mountable cache completely as when both are used they are unreliable (cache is empty and layers apply as if it worked). |
| - uses: docker/setup-qemu-action@v3 | ||
| - uses: docker/setup-buildx-action@v3 | ||
| annotations: | | ||
| org.opencontainers.image.authors="[email protected]" |
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 add this to the Dockerfiles instead:
LABEL org.opencontainers.image.authors="[email protected]"Ref: https://docs.docker.com/reference/build-checks/maintainer-deprecated/
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.
Note, Dockerfiles do already have a maintainer label:
Line 45 in 8085c75
| LABEL maintainer="[email protected]" |
It's probably better to use org.opencontainers.image.authors instead, but definitely not two labels for the same thing.
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 moved it to tag generation to avoid having implicitly marking user built containers as maintained by gitea, but I can move back into the container if it's not a concern.
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.
Hmm makes sense your way I guess. Would just like to avoid duplication.
It's not required and grows over time. Also exclude containerfiles from the copy so changes in them won't trigger layer invalidation if it's not needed
|
By the way, to speed up the "github actions release", I can see a much faster approach:
(The container build cache can still be supported for developers who need to build the images by themselves) |
|
How would you keep it sane though? It is a good idea ( |
|
Just an idea 😄 not sure whether it brings enough value |
|
I'd say it does but inverse (binary from container) would be simpler to do (safe for windows builds). I can look at it later. Is the |
Agree with you. That checkout came from "Docker multi-stage (#2927)", it seems no real use case for it.
If removing it can make the whole system simpler, maybe it's fine to remove it. If removing it doesn't simplify, maybe it can still be kept for a while. Update: it just conflicts the GITEA_VERSION used by Makefile:
|
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.
Overall LGTM.
The new approach looks pretty cool and much clearer than before.
| # Setup repo | ||
| COPY . ${GOPATH}/src/code.gitea.io/gitea | ||
| WORKDIR ${GOPATH}/src/code.gitea.io/gitea | ||
| COPY --exclude=.git/ . . |
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.
| COPY --exclude=.git/ . . | |
| # Use COPY but not "mount" because some directories like "node_modules" contain platform-depended contents and these directories need to be ignored. | |
| # ".git" directory will be mounted later separately for getting version data. | |
| # TODO: in the future, maybe we can pre-build the frontend assets on one platform and share them for different platforms, the benefit is that it won't be affected by webpack plugin compatibility problems, then the working directory can be fully mounted and the COPY is not needed. | |
| COPY --exclude=.git/ . . |
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.
@silverwind Maybe you are also interested in this TODO (to see whether it is feasible in the future)
IIRC sometimes the webpack (or esbuild, whatever) plugins have platform compatibility problems, for example: lacking riscv64 support, etc.
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.
Yes, frontend assets only need to be built once. Could run make frontend outside docker, and then if the frontend files have a fresh modified timestamp, Make will skip the target next time.
If that works, we could migrate to rspack, which as of today still does not support RISC-V.
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.
Hm... I'll look at it later. It sounds doable if I figure out how to convince build system to take amd64 frontend for other architectures. I did play with separate stage for frontend when I was trying to optimize for registry cache.
Or prebuilding on host and copying it over would also work and would be simpler to pull off at the cost of not being able to run docker build . for everything.
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.
(no need to be in this PR's scope 😄 )
| EXPOSE 22 3000 | ||
|
|
||
| RUN apk --no-cache add \ | ||
| RUN apk add --no-cache \ |
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.
Can be reverted (and keep consistent with Dockerfile.rootless)
| # Setup repo | ||
| COPY . ${GOPATH}/src/code.gitea.io/gitea | ||
| WORKDIR ${GOPATH}/src/code.gitea.io/gitea | ||
| COPY --exclude=.git/ . . |
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.
| COPY --exclude=.git/ . . | |
| # See the comments in Dockerfile | |
| COPY --exclude=.git/ . . |
|
|
||
| COPY --from=build-env /tmp/local / | ||
| COPY --from=build-env --chown=root:root /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea | ||
| COPY --from=build-env --chmod=755 --chown=root:root /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea |
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.
--chmod=755 is not needed because there is a complete chmod 755 above
| COPY --from=build-env --chmod=755 --chown=root:root /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea | |
| COPY --from=build-env --chown=root:root /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea |
| echo "git:*" | chpasswd -e | ||
|
|
||
| COPY --from=build-env /tmp/local / | ||
| COPY --chmod=755 --from=build-env /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea |
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.
--chmod=755 is not needed because there is a complete chmod 755 above
| COPY --chmod=755 --from=build-env /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea | |
| COPY --from=build-env /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea |
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 can revert these file mode changes now 😄
Using 0644 can help us to confirm our chmod really work as expected.
| /.air | ||
| /.go-licenses | ||
| Dockerfile | ||
| Dockerfile.rootless |
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.
Why no leading slash? Imho it's better to exactly ignore these files in the root, not ignore them anywhere in the tree.
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.
Actually I think maybe they don't need to be ignored? I guess there should be nothing wrong by keeping them
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.
IMO they should be ignored - if they aren't you end up rebuilding entire container because of a change in a comment in Dockerfile, which I just find annoying if nothing meaningful changes. When working on the dockerfile it's just annoying as let's say you changed package in the assembly stage or directives for binds/cache in RUN make step. Now the COPY digest differs so it ends up running again and has to build gitea from scratch despite nothing of relevance to the build being changed.
You don't hit this unless you're tinkering with the container builds and it was much worse pre-cache since building took 15-30 minutes on my machine due to having to download packages over and over.
| Dockerfile | ||
| Dockerfile.rootless |
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.
| Dockerfile | |
| Dockerfile.rootless | |
| /Dockerfile | |
| /Dockerfile.rootless |
In any case, let's match exactly.
add mount cache directives to container builds, which speeds up local builds bypassing node and go package download entirely on second build and caching go compilation.
drop job level split on regular/rootless, which allows to reuse the previously made stage for rootless, skipping duplicate builds in CI.