-
Notifications
You must be signed in to change notification settings - Fork 3
Install ccache latest released version instead of version from package manager #90
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
Conversation
mathbunnyru
left a comment
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 want to suggest building ccache from source:
- we will have more uniform setup
- latest version will be available everywhere
- ccache will behave the same way
- it should work well with OS upgrades, and there will be no dependency how ccache is built in their repo
- it's what Clio does 🙂
From the implementation perspective you now have:
FROM something as base
FROM base as gcc
FROM base as clang
What you can do is:
FROM something as base
FROM base as gcc-base
FROM gcc-base as gcc-build
// build ccache here
FROM gcc-base as gcc
COPY built ccache from gcc-build
FROM base as clang
COPY built ccache gcc-build
That sounds reasonable. I'd like to do this as a follow-up though, so that the ccache PR in the rippled repo can take advantage of the newer version, which should fix the issue with the coverage build objects not being cachable, and possibly the Ubuntu Jammy ones too. I'll then send you the build-from-source PR in a day or two. I'll also look into whether some of the other dependencies, like mold, should get the same treatment. |
| --build-arg GCC_VERSION=${GCC_VERSION} \ | ||
| --build-arg GCOVR_VERSION=${GCOVR_VERSION} \ | ||
| --build-arg CCACHE_VERSION=${CCACHE_VERSION} \ |
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.
Looks like refactoring for sort is not done fully here - let's either sort everywhere in a similar manner, or not do refactoring here at all
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.
As mentioned in the PR description, I've ordered this by:
- Distro
- Compiler
- Other flags, alphabetically.
Sorting everything alphabetically doesn't capture the importance of the distro and compiler enough, since those two pieces are the most representative parts of the image.
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.
Ah, got it, thanks.
I highly recommend adding an empty line between sections to make it more clear and show intent better
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.
Will do in the follow-up PR - stay tuned!
The change modifies the Debian, RHEL, and Ubuntu images to build from source. As suggested in #90 (review), I first explored using the GCC image as a base to build ccache, and then copy it into the final GCC and Clang images. However, this resulted in two issues: 1. The Clang images would need to build the GCC base image first, which would result in additional work. 2. The Clang images would need to be provided with a GCC version to use. I considered building a wholly separate image, e.g. a `tools-rippled-ccache`, to use as base image in which GCC would be installed and then ccache built from source, upon which it could be copied into the distro images. However, this has its own drawbacks: 1. The CI pipeline currently runs all builds in parallel, so it is not set up to handle dependencies between images. This would then result in failures until the ccache image is built and pushed. 2. Future updates to the ccache image would lead to unexpected issues. Namely, if you were to change the ccache version, the old image would still used due to issue 1. above, so you'd need to first merge the change, and then rerun the pipelines afterwards so the distro images actually use the new ccache image. (Note that we already have this issue with the GCC base image in the `docker/gcc` directory, used by Debian, so I prefer to not add more such cases.) So, to keep things simple at the expense of duplication (of which there already is a bit; unfortunately Docker doesn't support importing a bunch of instructions from another Dockerfile), I build ccache from source in both the GCC and Clang final images. Note that the gold linker in RHEL 8 is so old that it doesn't support a compiler flag used by ccache, so I've configured it to use the default linker instead.
Ccache has had many improvements over the years, including one that no longer considers the
-fprofile-update=atomiccompiler flag to be unsupported. This change therefore installs the most recent version directly from their GitHub repo instead of using the version bundled by the OS package manager.However, note that RHEL 8 and Debian Bullseye do not support the latest version due to the GLIBC version of the distro being too old. While an older ccache version could be installed, these are only available for direct download for x86_64. Since we don't use the problematic flag on these two distros (and, since we release using Debian Bullseye where we disable ccache anyway), this is fine.
Also note that as the flags in the README and CI were an unorganized mess, I've ordered them as follows, which should make things more readable: