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/Hydra
Matrix/MSC
Matrix/Media
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status/Blocked
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:
a71e584a2598632d511bFwiw 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 URLconst 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_urlis 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_REGISTRYcould 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_REGISTRYon my version and this repo, and testing a version that usesvars.BUILTIN_REGISTRYinstead, 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 SHAShort 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=maxHave you tried enabling this again?
@ -0,0 +57,4 @@LOG_LEVEL: ${{ inputs.logLevel || 'info' }}# Forgejo/Gitea specificRENOVATE_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_AUTHORwith an example value if you want to update it 🙂@ -0,0 +60,4 @@RENOVATE_GIT_AUTHOR: 'Renovate Bot <renovate@noreply.${{ github.server_url }}>'# PR settingsRENOVATE_PR_HOURLY_LIMIT: '2'Pretty sure you already set this in the config file
98632d511b667f5f3472667f5f347269d8c82a7f69d8c82a7f087beca7fc087beca7fca20fe8fc8ea20fe8fc8e741a26a7c5741a26a7c539e5e2b11939e5e2b1198c1b59c6848c1b59c68474b0760982ci: tidy CI pipeline for readability and performanceto ci: Tidy CI pipeline for readability and performance74b0760982a08c6aacc5a08c6aacc5b0ebdb59edb0ebdb59ed50250f51fc@ -0,0 +225,4 @@# Stage 2: Final scratch imageFROM scratchARG GIT_COMMIT_HASHthese 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 linkedif [ "${{ matrix.platform }}" = "linux/amd64" ]; thenif ldd target/${{ matrix.rust_target }}/release/conduwuit 2>/dev/null | grep -q "=>" ; thenLDD 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-configneeds 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-configwe're cross-compiling.@ -0,0 +123,4 @@cxx: g++linker: gccdpkg_arch: ""gcc_package: gccWhat'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=1Shouldn'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 / PublishChecks /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 firstWAIT_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.
50250f51fcb9b33e3db7@ -0,0 +216,4 @@# Copy binarycp target/${{ matrix.rust_target }}/release/conduwuit docker-context/# Note: We rely on Rust producing mostly-static binaries with --releaseI'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
lldto pull out dynamic libraries, but you've also pointed out that we can't just uselldto pull out dynamic libraries on cross-compiled binaries.I'm currently testing switching from
gcctoclangso thatlldcan 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
b9b33e3db7b481ff31c0ci: 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