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

ci: build docker images for linux/arm64 #496

Merged
merged 1 commit into from
Oct 26, 2023
Merged

ci: build docker images for linux/arm64 #496

merged 1 commit into from
Oct 26, 2023

Conversation

nikaro
Copy link
Contributor

@nikaro nikaro commented Oct 26, 2023

  • Add linux/arm64 in the build platforms
  • Use a matrix for parallel build of Docker images
  • Update parent images to rust:1.73-alpine3.18 as build and alpine:3.18

Copy link
Collaborator

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This technically looks good to me, however I would like to know first what motivates this change.

In particular, there's 2 aspects I'm uncertain about:

  1. How costly are the steps that are now duplicated because of the matrix. What do we gain by using a matrix?
  2. Does Taplo guarantee MSRV? It doesn't look like it but I might be wrong. If it does, then maybe 1.61 was chosen for a reason.

@ia0 ia0 requested a review from panekj October 26, 2023 17:57
@panekj
Copy link
Collaborator

panekj commented Oct 26, 2023

For point 1, using matrix makes jobs run in parallel instead on a single runner one after another.

@ia0
Copy link
Collaborator

ia0 commented Oct 26, 2023

For point 1, using matrix makes jobs run in parallel instead on a single runner one after another.

Yes but that's not the question. The question is whether the 3 docker actions that will now be duplicated are costly or not. (And implicitly if we can afford that cost.)

@panekj
Copy link
Collaborator

panekj commented Oct 26, 2023

For point 1, using matrix makes jobs run in parallel instead on a single runner one after another.

Yes but that's not the question. The question is whether the 3 docker actions that will now be duplicated are costly or not. (And implicitly if we can afford that cost.)

I'm not sure what cost are you speaking of

@nikaro
Copy link
Contributor Author

nikaro commented Oct 26, 2023

Thanks for the PR! This technically looks good to me, however I would like to know first what motivates this change.

This is mainly to be able to use Taplo through Docker on ARM machines, like the Apple Silicon MacBooks or Raspberry Pi.
I want to make a pre-commit hook that uses the docker image to lint and format TOML files, but currently it wont work on my M1 machine.

In particular, there's 2 aspects I'm uncertain about:

  1. How costly are the steps that are now duplicated because of the matrix. What do we gain by using a matrix?

Most of the time for this job is consumed by the docker/build-push-action@v3 step, the others steps only take a few seconds each. So it won't change much regarding CI minutes consumption.

  1. Does Taplo guarantee MSRV? It doesn't look like it but I might be wrong. If it does, then maybe 1.61 was chosen for a reason.

I don't speak Rust 😬 so i don't understand what you are talking about. But i upgraded the FROM rust:* image because i couldn't build the container with the current version:

> docker build -t taplo -f docker/cli-alpine.Dockerfile .
[+] Building 267.6s (11/12)                                                                                                                                                                 docker:orbstack
 => [internal] load build definition from cli-alpine.Dockerfile                                                                                                                                        0.0s
 => => transferring dockerfile: 297B                                                                                                                                                                   0.0s
 => [internal] load .dockerignore                                                                                                                                                                      0.0s
 => => transferring context: 47B                                                                                                                                                                       0.0s
 => [internal] load metadata for docker.io/library/alpine:3.16                                                                                                                                         1.5s
 => [internal] load metadata for docker.io/library/rust:1.61-alpine3.16                                                                                                                                1.0s
 => [build 1/5] FROM docker.io/library/rust:1.61-alpine3.16@sha256:3bcdeab61ea4e01db688277c5565b4bfb845293520d3b5eee7e9acf02ad6902b                                                                    0.0s
 => CACHED [stage-1 1/2] FROM docker.io/library/alpine:3.16@sha256:a8cbb8c69ee71561f4b69c066bad07f7e510caaa523da26fbfc606b10bd7934b                                                                    0.0s
 => [internal] load build context                                                                                                                                                                      0.0s
 => => transferring context: 91.75kB                                                                                                                                                                   0.0s
 => CACHED [build 2/5] WORKDIR /build                                                                                                                                                                  0.0s
 => CACHED [build 3/5] RUN apk add --no-cache musl-dev                                                                                                                                                 0.0s
 => [build 4/5] COPY . .                                                                                                                                                                               0.1s
 => ERROR [build 5/5] RUN cargo build -p taplo-cli --release                                                                                                                                         266.0s
------
 > [build 5/5] RUN cargo build -p taplo-cli --release:
[...]
251.3    Compiling reqwest v0.11.12
252.7 error[E0658]: use of unstable library feature 'bool_to_option'
252.7    --> crates/taplo/src/formatter/mod.rs:608:18
252.7     |
252.7 608 |                 .then_some(*indent + 1)
252.7     |                  ^^^^^^^^^
252.7     |
252.7     = note: see issue #80967 <https://github.com/rust-lang/rust/issues/80967> for more information
252.7
253.8 For more information about this error, try `rustc --explain E0658`.
253.8 error: could not compile `taplo` due to previous error
253.8 warning: build failed, waiting for other jobs to finish...
------
cli-alpine.Dockerfile:9
--------------------
   7 |     COPY . .
   8 |
   9 | >>> RUN cargo build -p taplo-cli --release
  10 |
  11 |     FROM alpine:3.16
--------------------
ERROR: failed to solve: process "/bin/sh -c cargo build -p taplo-cli --release" did not complete successfully: exit code: 101

@panekj
Copy link
Collaborator

panekj commented Oct 26, 2023

This has been stabilized in 1.62

252.7 error[E0658]: use of unstable library feature 'bool_to_option'
252.7    --> crates/taplo/src/formatter/mod.rs:608:18
252.7     |
252.7 608 |                 .then_some(*indent + 1)
252.7     |                  ^^^^^^^^^
252.7     |
252.7     = note: see issue #80967 <https://github.com/rust-lang/rust/issues/80967> for more information

@ia0
Copy link
Collaborator

ia0 commented Oct 26, 2023

This is mainly to be able to use Taplo through Docker on ARM machines

I see, so that explains the added linux/arm64 line.

Most of the time for this job is consumed by the docker/build-push-action@v3 step, the others steps only take a few seconds each.

Ok perfect, so the factorization is fine.

I don't speak Rust

This is the Minimum Supported Rust Version. Some projects like to guarantee that their project builds for old versions of the compiler (as long as it's not older than the MSRV they want to support). I couldn't find any mention of the MSRV for Taplo (which kind of makes sense because it's much for of a binary than a library, and MSRV doesn't make too much sense for a binary).

So all good on my side to merge. That said, I couldn't find a successful run of this job, so I'm not sure this PR will solve your problem, but at least it increases the chances of solving your problem. Thanks again for the PR!

@nikaro
Copy link
Contributor Author

nikaro commented Oct 26, 2023

So all good on my side to merge. That said, I couldn't find a successful run of this job, so I'm not sure this PR will solve your problem, but at least it increases the chances of solving your problem. Thanks again for the PR!

Here is the last run for this job: https://github.com/tamasfe/taplo/actions/runs/5511385857/job/14920644021c
The log has expired but its outcome is successful so it should be fine. But we won't know it before the next release-taplo-cli-0* tag push.

Thanks for the quick feedback! 🙏

@ia0
Copy link
Collaborator

ia0 commented Oct 26, 2023

Here is the last run for this job: https://github.com/tamasfe/taplo/actions/runs/5511385857/job/14920644021c

Cool, thanks! I missed it. So it's 19min and was the last release so yeah it should probably work for the next as well, which should ideally be soon (see #473).

@ia0 ia0 mentioned this pull request Oct 26, 2023
@panekj panekj merged commit 42bce0c into tamasfe:master Oct 26, 2023
4 checks passed
@nikaro nikaro deleted the ci/docker-linux-arm64 branch October 26, 2023 20:41
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.

3 participants