WIP: ci: Tidy CI pipeline for readability and performance #924
No reviewers
Labels
No labels
Bug
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/MSC
Matrix/Media
Meta
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
To-Merge
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!924
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tom/fixes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
While looking at complement, I've prepared some tidy-ups that shave precious time off the CI checks, use caching more, and update prefligit to the new version of prek.
Pipeline Modernisation:
Workflow Restructuring:
Performance Improvements:
Dependency Management:
a71e584a25
98632d511b
Fwiw sccache only needs the secrets to download files from GitHub without rate limiting, I'm not sure why that action forces them. If you can get the binary in a different way that might be better
Renaming things is redundant by the way, and breaks the branch protection rule (
Checks / *
)@ -0,0 +43,4 @@
// Extract hostname from server URL
const serverUrl = '${{ github.server_url }}'
const registry = serverUrl.replace(/^https?:\/\//, '')
This is wrong - would extract the registry as github.com rather than ghcr.io on github for example. Perhaps there's an env variable for this? Anyway
There isn't,
github.server_url
is the only one I could find in the docs... we could specify an env var for the preferred Docker repository though, which would make this easier - my problem was that it wasn't at all portable, so when I tried to run CI on my fork, it immediately collapsed when trying to auth with forgejo.ellis.linkPerhaps
env.BUILTIN_REGISTRY
could be removed from the top and we just declare it on the repo, then I'll adjust this to skip trying to upload an image if that's missing?Yeah that works
It would be vars.
I've set
BUILTIN_REGISTRY
on my version and this repo, and testing a version that usesvars.BUILTIN_REGISTRY
instead, if that's ok?(Edit: Ah ha, you'd already replied!)
@ -0,0 +120,4 @@
env:
DOCKER_METADATA_ANNOTATIONS_LEVELS: manifest,index
- name: Get commit SHA
Short commit sha specified explicitly here for a reason
I can't actually review this with the renames because it breaks the diff.
@ -0,0 +194,4 @@
labels: ${{ steps.meta.outputs.labels }}
annotations: ${{ steps.meta.outputs.annotations }}
cache-from: type=gha
# cache-to: type=gha,mode=max
Have you tried enabling this again?
@ -0,0 +57,4 @@
LOG_LEVEL: ${{ inputs.logLevel || 'info' }}
# Forgejo/Gitea specific
RENOVATE_GIT_AUTHOR: 'Renovate Bot <renovate@noreply.${{ github.server_url }}>'
Should be configurable so I can get this to actually match up with the account
Sure! Would you prefer it configurable as one line, or individual vars for name and email?
Probably easier as just one line
Cool, I've added
RENOVATE_AUTHOR
with an example value if you want to update it 🙂@ -0,0 +60,4 @@
RENOVATE_GIT_AUTHOR: 'Renovate Bot <renovate@noreply.${{ github.server_url }}>'
# PR settings
RENOVATE_PR_HOURLY_LIMIT: '2'
Pretty sure you already set this in the config file
98632d511b
667f5f3472
667f5f3472
69d8c82a7f
69d8c82a7f
087beca7fc
087beca7fc
a20fe8fc8e
a20fe8fc8e
741a26a7c5
741a26a7c5
39e5e2b119
39e5e2b119
8c1b59c684
8c1b59c684
74b0760982
ci: tidy CI pipeline for readability and performanceto ci: Tidy CI pipeline for readability and performance74b0760982
a08c6aacc5
a08c6aacc5
b0ebdb59ed
b0ebdb59ed
50250f51fc
@ -0,0 +225,4 @@
# Stage 2: Final scratch image
FROM scratch
ARG GIT_COMMIT_HASH
these are all for build time
They were being passed in as build args from the workflow, but I'll convert them so we're directly substituting into envs in the workflow.
@ -0,0 +204,4 @@
# For native builds, extract libraries if dynamically linked
if [ "${{ matrix.platform }}" = "linux/amd64" ]; then
if ldd target/${{ matrix.rust_target }}/release/conduwuit 2>/dev/null | grep -q "=>" ; then
LDD doesn't work cor cross compile
(you will end with none of the needed libs on cross-compiles)
It was only being done for
linux/amd64
(which we can do it for) but I'll remove it to make all of the builds identical - when we don't extract them, we just end with a larger static build.@ -0,0 +185,4 @@
--release \
--locked \
--features "standard,jemalloc_prof,perf_measurements,tokio_console" \
--target ${{ matrix.rust_target }}
Missing SBOM, missing ability to build for a specific target CPU (eg haswell), not doing cross-language LTO afaict
I've added SBOM and updated the x86_64 arch from "base" to "haswell" and enabled LTO 🙂
We still need to build for base because not all CPUs are post haswell
Ok, I'd appreciate if you could be a bit clearer on how you'd prefer this to look then - I couldn't see any Docker tags for haswell, so how would you prefer it be delivered?
The ability to pass in a target CPU that gets set as the needed compiler flags -
ARG TARGET_CPU
- and then to build that as a separate job in the matrix and publish the image with a suffix. This wasn't used previously because of the extra build time. Automatic selection of the correct binary from a single image could be achieved via https://forgejo.ellis.link/continuwuation/hwcaps-image but I never got around to it and I'm not sure about the thing being GPL3
@ -0,0 +168,4 @@
gcc-package: ${{ matrix.gcc_package }}
gxx-package: ${{ matrix.gxx_package }}
liburing-package: ${{ matrix.liburing_package }}
is-cross-compile: ${{ matrix.is_cross_compile }}
Cross compiling should be effectively identical to normal builds
That would be the dream, but there are differences - in particular,
pkg-config
needs to be told it's a cross-compile, but we also need to download arch-specific liburing too.See how the Dockerfile handles it. For example, it has two sets of deps - host and target - and always installs both sets.
I believe Docker emulates the architecture outside the container, so inside the container we just install for emulated architecture?
In this case, to use tools like sccache, we're cross-compiling without using Docker (just adding the binary to Docker afterwards) so we need to configure cross-compilation and tell
pkg-config
we're cross-compiling.@ -0,0 +123,4 @@
cxx: g++
linker: gcc
dpkg_arch: ""
gcc_package: gcc
What's going on here? this really shoudn't need to be specified for every platform in the matrix.
The inputs save defining any OS-specific config inside the action, so literally all the compile config is defined in the workflow, then the action just configures the Rust environment to use what it's been told.
.forgejo/actions/rust-with-cache/action.yml
- where all the logic is - is already tied to bash and debian very specifically."OS" was intended to mean "architecture" but the runner commands are only written to use Debian-type because the runner is
ubuntu-latest
- the actual cross-compile config provided in the matrix strategy can target Darwin, FreeBSD, or even Windows if we have the necessary libraries.@ -1,235 +0,0 @@
ARG RUST_VERSION=1
Shouldn't be deleting the dockerfile anyway.
It seemed a risk to leave the Dockerfile laying around if we never actually use it for builds, as it'll quickly fall out of maintenance... I can leave it in to be retired on a future date, whatever works for you 🤷♂️
I'll still be using (a slightly modified version) of it for my prod deploys.
But the main reason was I was still experimenting with getting it and #912 to work
I've restored the Dockerfile, I'll look at whether anything needs updating later.
@ -0,0 +1,409 @@
name: Checks / Build / Publish
Checks /
means it blocks merges - should probably be separate (you can add depends across workflows IIRC)I'm not sure what you mean here - the current checks and builds run on every branch/PR, so I've organised them into one workflow to stop 6 jobs trying to run at once, to generate better cache, and complete faster. The same jobs will run, it'll just run more efficiently (and fast-fail the other jobs when linting/clippy fails).
Everything under
Checks /
has to finish before you can hit merge - so you have to wait for the images to publish now, when you didn't before. Not as much of an issue because it's faster now though@ -0,0 +626,4 @@
echo "::group::🎯 Cross-compilation target setup"
# Wait for basic Rust setup to complete first
WAIT_START=$(date +%s)
for al bash scripts, given there are so many, it would probably be better if they were in files where they're syntax highlighted and not indented
I'm not sure we can use GHA variables the same way inside Bash scripts in separate files... I could convert it to JavaScript if you'd rather, or just split into more actions so it's easier to audit in chunks?
GHA vars can still be passed in via
env:
inputs - it's up to you tbh. It just gets kind of hard to read & edit code embedded into yaml lolIt is a big chonk, but it was partly why I tried to shift most of the common "make Rust work please" dependencies into a big action that extrapolates a lot of the mess out of the other workflows.
I'd be up for splitting it into more actions, just think it might be a rough experience to have all of the shell scripts separate in folders. I can see what that'd look like though?
Nah it's OK if you don't think it's worth it.
50250f51fc
b9b33e3db7
@ -0,0 +216,4 @@
# Copy binary
cp target/${{ matrix.rust_target }}/release/conduwuit docker-context/
# Note: We rely on Rust producing mostly-static binaries with --release
I'd prefer words as well as screenshots to communicate, but I think what you're saying here is that the current build method (that does everything inside an emulated architecture inside Docker) can use
lld
to pull out dynamic libraries, but you've also pointed out that we can't just uselld
to pull out dynamic libraries on cross-compiled binaries.I'm currently testing switching from
gcc
toclang
so thatlld
can support the cross-compilation environment and potentially replicate how this was working in Docker... if that doesn't work then 🤷♂️This is done in the docker image using lddtree/goblin, which does not need to execute the files and so can work on cross compiles.
Oh and you're gonna need to switch to clang anyway for the cross-language LTO to work
b9b33e3db7
b481ff31c0
ci: Tidy CI pipeline for readability and performanceto WIP: ci: Tidy CI pipeline for readability and performanceMarking this WIP till it produces working images
Pull request closed