-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Use zigbuild to unify the build process of binary files and docker #1093
base: main
Are you sure you want to change the base?
Conversation
I haven't tested it but it looks great, thanks @vickunwu ! If it's not too much of an ask, would be willing to add SLSA provenance to the builds? @33KK since you created the current |
d8c9e20
to
342bbc0
Compare
@mdecimus While I was working on the SLSA provenance, I just found that when specifying a single $TARGETPLATFORM in docker bake file, it will preserve only one arch under the same tag in Container Registry. Clearly this PR needs some refinements, I am converting this into a draft. Considering about the workaround, do you think it's ok that we just copy the artifacts from previous build jobs to the runtime container when building the image in a seperate job? If so, should we add attestations for the artifacts, or just generate provenances for the compressed release files? |
f05376b
to
552fa81
Compare
Never mind. I find a way to build multi-arch image across different runner. |
Great, thanks! Please let me know once it is ready and I'll test it out. Just two comments and one question:
Thanks again. |
ea64780
to
5421dad
Compare
Changes ready for review now.
As for arm64 runner, right now it seems unnecessary to delegate the corresponding tasks with it. I believe we can add another attribute(e.g. host_os) to the linux build matrix once foundationdb releases a stable version of arm64 debian pkg.
Sorry, I don't have much clue about it. |
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.
Well, this isn't using zigbuild to unify anything, but docker-bake
, which I have zero clue about. And replaces cross
with cargo-zigbuild
, which is a good idea if it works properly. Don't really like the amount of complexity and separate tools and build systems here, but I guess there's not that much you can do about it.
It's crazy how unnecessarily complex and fragile are CI and build systems, and all the container related tools are. (warning, rant)
Yaml became the programming language for all the CI systems, which has zero static analysis, and really sucks to write since it's not an actual programming language but a JSON clone bodged to function as a """programming language""". It pretty much requires pushing your changes to a repo a hundred times and waiting for the CI to fail a hundred to see if it works or not. It pretty much requires you to use shell scripts or some other scripting language to get anything done, which also means you have to deal with two different similar variable syntaxes ${VAR}
for shells and ${{expr}} for github actions
.
Docker only ever "fixed" the dependency hell from the POV of whoever has to deploy the built containers, it's still a disaster in terms of actually building them, it's still a mix of package managers, random file downloads, awkward copying of files from other images or using base images. Plus the build tools around it is a disaster.
Also everything relies on POSIX compatible shells which, let's be real, are not really portable, extremely fragile, error prone and painful to get anything done with, or just simply terrible.
All of this just doesn't integrate well together and is extremely fragile and very hard to debug. Also the amount of parties to trust sure doesn't give me much hope that any of this is at all resistant to supply chain attacks. The "attest" thing barely gives any reassurance considering that all it ensures is that the image was built on github actions (if it actually even does that and you can't just fake it with some amount of effort and using a ssh server action) but does nothing about all the external tools and dependencies.
SCCACHE_GHA_ENABLED: true | ||
RUSTC_WRAPPER: sccache |
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.
Probably shouldn't be enabled on releases
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 you please be more specific about the reason?
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.
More or less a "just in case" thing, I imagine there's a chance it could introduce miscompilations, release builds aren't terribly frequent and don't take that much longer anyway
- name: Inject cache into docker | ||
uses: reproducible-containers/buildkit-cache-dance@v3.1.2 | ||
with: | ||
cache-map: | | ||
{ | ||
"var-cache-apt": "/var/cache/apt", | ||
"var-lib-apt": "/var/lib/apt", | ||
"usr-local-cargo-registry": "/usr/local/cargo/registry", | ||
"usr-local-cargo-git": "/usr/local/cargo/git" | ||
} | ||
skip-extraction: ${{ steps.cargo-cache.outputs.cache-hit }} && ${{ steps.apt-cache.outputs.cache-hit }} |
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.
Not sure how exactly reproducibility goes together with injecting cache only ever accessible from the CI
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.
Haha is that a judgement from the action name?
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.
Yeah, not really about the PR I guess, it's just kinda funny to me how that github org hosts an action that very much seems to do the opposite of reproducibility
type=ref,event=tag | ||
type=ref,event=branch,prefix=branch- | ||
type=edge,branch=main | ||
type=semver,pattern=v{{major}}.{{minor}} |
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.
Not sure how exactly this works, but it should end up creating tags like latest
and vMAJOR.MINOR.PATCH
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.
you can think the 2nd and 3rd rule are for manual build.
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 mean to me it seems like it creates "edge" instead of "latest" but idk, docker build tools are really confusing
RUN ZIG_VERSION=$(curl --retry 5 -sL "https://api.github.com/repos/ziglang/zig/releases/latest" | jq -r '.tag_name') && \ | ||
[ ! -z "$ZIG_VERSION" ] && \ | ||
curl --retry 5 -Ls "https://ziglang.org/download/${ZIG_VERSION}/zig-linux-$(uname -m)-${ZIG_VERSION}.tar.xz" | tar -J -x -C /usr/local && \ | ||
ln -s "/usr/local/zig-linux-$(uname -m)-${ZIG_VERSION}/zig" /usr/local/bin/zig | ||
# Install cargo-binstall | ||
RUN curl --retry 5 -L --proto '=https' --tlsv1.2 -sSf https://raw.githubusercontent.com/cargo-bins/cargo-binstall/main/install-from-binstall-release.sh | bash | ||
# Install FoundationDB | ||
# TODO According to https://github.com/apple/foundationdb/issues/11448#issuecomment-2417766293 | ||
# Once FoundationDB v7.3.53 gets released, we should be able to build the aarch64-unknown-linux-gnu target. | ||
# The last command is for future build use, so if you are building on a native arm64 device, please use docker qemu. | ||
RUN curl --retry 5 -Lso /usr/lib/libfdb_c.so "$(curl --retry 5 -Ls 'https://api.github.com/repos/apple/foundationdb/releases' | jq --arg arch "$(uname -m)" -r '.[] | select(.prerelease == false) | .assets[] | select(.name | test("libfdb_c." + $arch + ".so")) | .browser_download_url' | head -n1)" | ||
# Install cargo-chef & sccache & cargo-zigbuild | ||
RUN cargo binstall --no-confirm cargo-chef sccache cargo-zigbuild |
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.
Don't particularly like all the curl
ing action, but I guess that's just a Docker moment. The alternative is to use cargo-zigbuild
image as the base and copy the cargo-chef
binary from it's image, and sccache
from apt 💀
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.
cargo zigbuild image imports unnecessary macos sdk and all supported rust targets. most importantly, it may not track the latest stable rust 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.
That makes sense, I guess there's not much you can do here
--mount=type=secret,id=ACTIONS_CACHE_URL,env=ACTIONS_CACHE_URL \ | ||
--mount=type=secret,id=ACTIONS_RUNTIME_TOKEN,env=ACTIONS_RUNTIME_TOKEN \ |
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 idea what is this about, don't see anything that [I'd expect to] use it.
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.
This is the required env vars for sccache to use Github actio 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.
Makes sense
# ***************** | ||
# Binary stage | ||
# ***************** | ||
FROM scratch AS binaries | ||
COPY --from=builder /app/artifact / |
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 not just COPY from builder
in the images below?
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.
Because when the devs want to get the binaries without building a whole image, the docker build stops here.
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.
Is this how the docker-bake thing ends up generating binary artifacts? I guess that makes sense
Couldn't agree more.
I think there's still some upside about it. Personall I use it to build stuff because it sort of prevents the cache "pollution". |
This PR does the following things:
-alpine
)