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

Use BuildKit syntax in Dockerfile #2831

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

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Nov 25, 2024

Copy link
Collaborator

@sabine sabine left a comment

Choose a reason for hiding this comment

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

https://deploy.ci.ocaml.org/job/2024-11-25/151612-ocluster-build-0edd49 complains:

Error response from daemon: dockerfile parse error line 21: Unknown flag: link

@MisterDA
Copy link
Contributor Author

Yes, BuildKit needs to be enabled in ocurrent-deployer for ocaml.org. Regardless, are you interested in these changes?

@sabine
Copy link
Collaborator

sabine commented Dec 10, 2024

With the outcome of a faster build, they seem useful in the sense of taking less resources of the build cluster.

@cuihtlauac
Copy link
Collaborator

Thanks, @MisterDA. We're also considering replacing the opam-repo pin with a docker image pin. See the discussion #2760. Am I correct to assume using buildkit is an independent matter? We should be able to have both, in separate stages.

In this PR, it seems three things are done together

  1. Using buildkit features
  2. Moving the opam repo pin
  3. Improve the syntax, clarity and style

As much as possible, I'd like to have those addressed in separate PRs.

  1. -> Should be addressed first, let's merge that and have it out of sight.
  2. -> The pin needs to be changed in several places at the same time (yes, this is clunky). It's better to have a PR that only does that
  3. -> BuildKit seems very cool. I'm excited to take advantage of it.

P.S.

Sorry for sounding bureaucratic. With @mtelvers we recently had to make an urgent fix. A subtle bug was introduced when moving to OCaml 5. It didn't break until 5.2.1 was released.

Our builds (Docker + local + GitHub) are brittle. Let's be extra cautious when tweaking them. We all love debugging. But the holiday season is close. We'd better make sure build issues don't gatecrash into the parties.

@MisterDA MisterDA force-pushed the buildkit branch 2 times, most recently from 293fce5 to 1eb1c9f Compare December 12, 2024 09:15
@cuihtlauac cuihtlauac mentioned this pull request Dec 12, 2024
@cuihtlauac
Copy link
Collaborator

cuihtlauac commented Dec 13, 2024

@MisterDA, @mtelvers, @shonfeder: I have computed MTBC (mean time between commits) for ocamlorg and opam-repo

ocamlorg: ~14h14m
opam-repo: ~1h45m

In ocamlorg, commits are pushed one by one (we've turned off push to main and all merges are stashed). Can we make the same assumption for opam-repo?

Those numbers should be compared with the change frequencies of the ocamlorg pin and the docker image

ocamlorg build pin: ~ 2 per year
docker image: 1 per week

That's why I believe we shouldn’t fetch from opam-repo early. It should take place right before init-cache

@shonfeder
Copy link
Contributor

Hi! :) I'm afraid I'll need a bit of clarification.

  1. I don't really get the intended significance of the mean time between commits. Could you flesh this out a bit for me?

  2. I'm not sure what "all merges are stashed" means.

  3. In general, given that you've asked that this to be factored into separate PRs (a great call, IMO!) I'm not exactly sure the relevance of this point. Is it related to the discussion at Recreate PR #2861: Dockerfile cleanup #2866 (review) ?

Thanks in advance for the clarification!

@cuihtlauac
Copy link
Collaborator

@MisterDA: I've merged the cleanup PR. Can you rebase this one?

@MisterDA MisterDA force-pushed the buildkit branch 2 times, most recently from f813aa4 to f99e4b7 Compare December 18, 2024 17:13
- use mount caches to avoid redownloading apk packages;
- use COPY --link for more efficient copy and lighter images;
- use Buildkit built-in support for Git clones.
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