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

[build] add new dockerfiles for building from source #244

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

master-hax
Copy link

@master-hax master-hax commented Sep 18, 2024

description

this PR adds 2 new dockerfiles that allow users & developers to build containers from source in reference to previous discussion, restoring functionality that was lost in #59. one issue i see people facing is that doing a docker build against a specific branch or commit unexpectedly ignores that and uses the latest release.

the original dockerfiles were:

they were all basically the same alpine build but used emulation to support different architectures. however we can now use the built in multi-arch support from docker. this still uses emulation but it is simpler and no longer needs specific handling in the dockerfile.

i condensed all three of those down to Dockerfile.alpine with small improvements & added a brand new ubuntu based Dockerfile.ubuntu.

i hope that we can use these to replace the current no-op dockerfile in a future PR. i have an example of releasing multi-arch builds with cross-compilation here.

usage examples:

local docker build:

docker build . -f ./Dockerfile.alpine
docker build . -f ./Dockerfile.ubuntu
docker build . -f ./Dockerfile.alpine --build-arg ALPINE_VERSION=3.19
docker build . -f ./Dockerfile.ubuntu --build-arg UBUNTU_RELEASE_VERSION=jammy

local buildx build (enables cross-compilation and multi-arch containers):

docker buildx build --platform linux/amd64,linux/arm64 . -f ./Dockerfile.alpine
docker buildx build --platform linux/amd64 . -f ./Dockerfile.ubuntu
docker buildx build --platform linux/arm64 . -f ./Dockerfile.alpine --build-arg ALPINE_VERSION=3.19
docker buildx build --platform linux/amd64,linux/arm64 . -f ./Dockerfile.ubuntu --build-arg UBUNTU_RELEASE_VERSION=jammy

compose file from remote source:

services:
  redlib:
    build:
      context: https://github.com/redlib-org/redlib.git
      dockerfile: Dockerfile.alpine
services:
  redlib:
    build:
      context: https://github.com/redlib-org/redlib.git#v0.31.2
      dockerfile: Dockerfile.ubuntu
      args:
        UBUNTU_RELEASE_VERSION: "jammy"

compose file from local source (best for development):

services:
  redlib:
    build:
      context: ~/git/redlib
      dockerfile: Dockerfile.ubuntu
services:
  redlib:
    build:
      context: "$PWD"
      dockerfile: Dockerfile.alpine
      args:
        ALPINE_VERSION: "3.20"

@master-hax
Copy link
Author

cc @sigaloid @chowder

@master-hax master-hax changed the title [build] add new dockerfiles [build] add new dockerfiles for building from source Sep 18, 2024
Copy link
Contributor

@pimlie pimlie left a comment

Choose a reason for hiding this comment

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

Nice trick to cache the cargo dependencies! Left some remarks on the Ubuntu file that mostly also apply to the Alpine file 👍

Dockerfile.ubuntu Outdated Show resolved Hide resolved
Dockerfile.ubuntu Outdated Show resolved Hide resolved
Dockerfile.ubuntu Outdated Show resolved Hide resolved
Dockerfile.ubuntu Outdated Show resolved Hide resolved
Dockerfile.ubuntu Outdated Show resolved Hide resolved
Dockerfile.ubuntu Show resolved Hide resolved
Dockerfile.ubuntu Outdated Show resolved Hide resolved
@Tokarak
Copy link
Contributor

Tokarak commented Sep 20, 2024

By The Way, I recommend using LukeMathWalker/cargo-chef to cache the compilation of dependencies. I created this pr (mattfbacon/typst-bot#23) for another rust project, which you can use along with the official documentation.

Sorry, I didn't read this pr fully — maybe cargo chef should be in a new pr.

@pimlie
Copy link
Contributor

pimlie commented Sep 22, 2024

@Tokarak Reading the Benefits of cargo-chef, the main benefit for using cargo chef seems to be when you are using workspaces. As redlib doesn't use those (and there is only one cargo.toml file), just using @master-hax's implementation here should be fine.

@master-hax Note that docker layer caching also requires extra configuration in the workflows, see https://docs.docker.com/build/ci/github-actions/cache/

@Tokarak
Copy link
Contributor

Tokarak commented Sep 22, 2024

@pimlie cargo chef cook builds all the dependencies as a seperate layer, same as in the hack used in this pr. The benefit over this hack is that it generates less layers, is more obvious with what it does, and is resistant to breaking changes in Redlib and Rust because it is maintained.

@pimlie
Copy link
Contributor

pimlie commented Sep 22, 2024

@Tokarak It also means you have to fully trust the maintainers of cargo chef and their release process. Have you met them in person?
Same applies ofc to other software used during building, but recent years have shown multiple attacks on various ecosystems so less dependencies is always better. Im happy to leave it to others to decide whether cargo chef is a useful dependency or not :)

@master-hax
Copy link
Author

master-hax commented Sep 22, 2024

@Tokarak says:

Get rid of this hack and use cargo-chef.

as i have mentioned, a similar optimization was merged a year ago. also as @pimlie pointed it, using cargo-chef adds a major (& unnecessary) dependency to the build system.

if we prefer to use cargo-chef, i think someone should put up a separate PR to add that functionality so we can keep the scope of this PR small.

@master-hax
Copy link
Author

i think all the comments have been resolved

Copy link
Contributor

@pimlie pimlie left a comment

Choose a reason for hiding this comment

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

LGTM after these 2 small changes

This might be known already, but please note that docker layer caching of github actions also requires additional configuration in the github action workflow. Without that addtional config the layers won't ever be cached, see https://docs.docker.com/build/ci/github-actions/cache/

Dockerfile.alpine Show resolved Hide resolved
Dockerfile.ubuntu Show resolved Hide resolved
@master-hax
Copy link
Author

@sigaloid do you think this looks good to merge?

@sigaloid
Copy link
Member

I think this looks good. I just switched to openssl via native-tls in a96bebb. What impacts will this have on your dockerfiles, since we no longer use 100% rust-native dependencies?

ARM builds are currently broken (#326) so if we could replace our builds with this, provided it still works, that would be fantastic.

PS: I have very little knowledge of cross-building especially for other architectures, and the native dependencies only make it worse. So please bear with me :)

@master-hax
Copy link
Author

What impacts will this have on your dockerfiles, since we no longer use 100% rust-native dependencies?

rebased the PR branch for testing

@master-hax
Copy link
Author

master-hax commented Nov 22, 2024

both dockerfiles seem to work fine on both of my x86_64 & aarch64 machines without explicitly installing libssl-dev - i can add it if needed

@master-hax
Copy link
Author

bump cc @sigaloid can we move forward with this pull request?

bdamokos added a commit to bdamokos/redlib that referenced this pull request Jan 6, 2025
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.

4 participants