Skip to content
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

feat: add support for a persistent ccache #66

Closed

Conversation

henning-schild
Copy link
Contributor

Closes: #64

The implementation is very simple. It boils down to installing the package dev-util/ccache in the container and adjusting $PATH if a ccache.conf is found under /var/cache/ccache. That config file existing indicates that docker run was invoked with a --volume mount and the intention to use ccache.

In the documentation i tried to cover some basics on how it could be used and monitored.

Now let me know what you think, what the actions say and if and how it could be tested. ccache itself probably does not require much testing, the biggest problem for users might be getting the filesystem permissions correct so that the user "distcc" with uid 240 can read and write the cache. Luckily the uid is known and fix with acct-user/distcc.

@henning-schild henning-schild force-pushed the henning/staging0 branch 2 times, most recently from 970b791 to d6a02e0 Compare September 4, 2024 18:56
Copy link
Owner

@KSmanis KSmanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting in the effort! Please bear with me as the review process might take a while

docker-entrypoint-tcp.sh Outdated Show resolved Hide resolved
docker-entrypoint-tcp.sh Outdated Show resolved Hide resolved
docker-entrypoint-tcp.sh Show resolved Hide resolved
@henning-schild
Copy link
Contributor Author

CI is unfortunately horribly slow with these changes. And dev-util/ccache-4.10.1 pulls in dev-libs/blake3 which fails to compile in a i686 container. I was playing a bit with https://wiki.gentoo.org/wiki/Binary_package_guide but the packages in question are by enlarge not available, especially not across all arches.

Does you setup have a persistent buildpkg directory so we only pay the compilation price when receiving updates?

@Goosegit11

This comment was marked as off-topic.

@Goosegit11 Goosegit11 mentioned this pull request Sep 23, 2024
@KSmanis
Copy link
Owner

KSmanis commented Sep 23, 2024

Does you setup have a persistent buildpkg directory so we only pay the compilation price when receiving updates?

Indeed, there is a caching mechanism [1] that will bootstrap on the first master build, but the issue is the i686 build, which fails due to the assembler. You may try running the emerge command using --security=insecure [2] or enabling QEMU [3], although I don't think the latter would help to be honest.

[1] https://github.com/KSmanis/docker-gentoo-distcc/blob/master/Dockerfile#L3
[2] https://hub.docker.com/r/docker/dockerfile
[3] https://github.com/KSmanis/docker-gentoo-distcc/blob/master/.github/workflows/build.yml#L38

README.md Outdated Show resolved Hide resolved
docker-entrypoint-tcp.sh Outdated Show resolved Hide resolved
@henning-schild
Copy link
Contributor Author

Indeed, there is a caching mechanism [1] that will bootstrap on the first master build, but the issue is the i686 build, which fails due to the assembler. You may try running the emerge command using --security=insecure [2] or enabling QEMU [3], although I don't think the latter would help to be honest.

QEMU does the trick, see https://github.com/henning-schild/docker-gentoo-distcc/actions/runs/11013357776

I will include the patch into my next PR update

@henning-schild henning-schild force-pushed the henning/staging0 branch 5 times, most recently from 0ed4d27 to d75ab42 Compare September 25, 2024 14:55
dev-libs/blake3 required by dev-util/ccache does not build on amd64
and produces asssmebler errors

Signed-off-by: Henning Schild <[email protected]>
Instead of creating a new group put the user in the existing group. The
tcp version of the container will be running as distcc:distcc, using the
same group will allow sharing directories rw.

This patch prepares for the introduction of a persistent cache.

Related-to: KSmanis#64
Signed-off-by: Henning Schild <[email protected]>
@henning-schild henning-schild force-pushed the henning/staging0 branch 2 times, most recently from 694d5f7 to 225d2ab Compare October 1, 2024 12:52
Dockerfile Show resolved Hide resolved
@KSmanis
Copy link
Owner

KSmanis commented Oct 12, 2024

@henning-schild I went through the changes carefully and barring minor tweaks the PR is in good standing. Nevertheless, I experimented with creating dedicated ccache tags instead of a "master switch" in the feat/add-ccache-support branch, where I also incorporated a rudimentary ccache smoke test.

The drawback of the "master switch" approach is that an anonymous volume is always created, regardless of whether the user requested it or not--and there is no way around it. The drawback of the "dedicated tags" approach is that it's quite more complex maintenance-wise. I'm still undecided about which one to go with, but I'm inclined to go with the "dedicated-tags" approach because it's cleaner UX-wise.

Let me know if you have any suggestions/improvements.

@henning-schild
Copy link
Contributor Author

It seems the branch has a mix of my commits, a rebase and your changes. But i could not see a PR, so i will try and comment the patches in that branch.

@henning-schild
Copy link
Contributor Author

The drawback of the "master switch" approach is that an anonymous volume is always created, regardless of whether the user requested it or not--and there is no way around it.

Maybe the VOLUME statement in the Dockerfile is not needed after all. I have to admit that i still do not understand its benefit.

And i think it would be ideal to have exactly one container image and use tags for versions not variants. With only a very tiny set of config options, all passed via env. Because i think it might just be confusing to have the variants. To me the existing two are already confusing and having them differ in the tag and not the image name is also weird.

Can i download a build based on that code to try my bind-mount use-case, or would i have to build myself?

@KSmanis
Copy link
Owner

KSmanis commented Oct 13, 2024

The drawback of the "master switch" approach is that an anonymous volume is always created, regardless of whether the user requested it or not--and there is no way around it.

Maybe the VOLUME statement in the Dockerfile is not needed after all. I have to admit that i still do not understand its benefit.

Without a VOLUME statement there is no cache persistence out of the box (i.e., if the container stops, all data is lost), unless you explicitly specify either a named volume mount or a host bind mount, which is an extra configuration step for the end user. With the VOLUME statement, the end user gets an anonymous volume holding their cache without any interaction.

And i think it would be ideal to have exactly one container image and use tags for versions not variants. With only a very tiny set of config options, all passed via env. Because i think it might just be confusing to have the variants. To me the existing two are already confusing and having them differ in the tag and not the image name is also weird.

This approach is ubiquitous in the Docker world; tags are not restricted to versioning.

Can i download a build based on that code to try my bind-mount use-case, or would i have to build myself?

Of course, please do, I haven't tested it with a bind mount. All CI artifacts are published at ghcr.io and Docker Hub

@henning-schild
Copy link
Contributor Author

amd64-tcp-ccache-feat-add-ccache-support-5972dac confirmed working for the bind-mount

when using the cache you likely anyhow name the volume at run, at least i do that for my bind-mount, and if i used a volume i would probably want to give it a name ... so i really do not see the benefit of VOLUME and the additional image-variant.

But that is just me, merge whatever you want i am happy once ccache support will be there for a tcp server and i can bind-mount.

@KSmanis KSmanis closed this in ed2bbcd Oct 13, 2024
@KSmanis
Copy link
Owner

KSmanis commented Oct 13, 2024

@henning-schild thanks for all the contributions, much appreciated!

@henning-schild
Copy link
Contributor Author

@KSmanis thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add ccache support
3 participants