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.

Sign up for GitHub

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

Jump to bottom

Switch BUILDKIT_SYNTAX from to tianon/buildkit #17632

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Switch BUILDKIT_SYNTAX from to tianon/buildkit #17632

wants to merge 1 commit into from

Conversation

tianon
Copy link
Member

@tianon tianon commented Sep 26, 2024

Support for this was implemented in my images back in tianon/dockerfiles@8c85466 / tianon/dockerfiles#733 as a PoC (and support for it was kept in tianon/dockerfiles@2b78191 / tianon/dockerfiles#744 when the explicit 0.13 builds were added), and I've actually been using it pretty heavily since without issue, both locally and for all of my tianon/xxx personal image builds (including building of tianon/buildkit itself, for maximum meta 😂 see https://oci.dag.dev/?blob=tianon/buildkit@sha256:d09d4afa9bb834d38fed1d30d2108e844b282050ea01f82f077d092424993ac3&mt=application%2Fvnd.in-toto%2Bjson&size=26900#:~:text=build%2Darg%3ABUILDKIT_SYNTAX).

The primary benefit is that we get the exact same architecture support matrix as our BuildKit images (and the same set of applied patches, if they happen to impact the frontend code as well).

$ docker buildx build --pull --no-cache --build-arg BUILDKIT_SYNTAX=tianon/buildkit:0.13 - <<<$'FROM bash\nRUN echo see, it works!'
#0 building with "default" instance using docker driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 71B done
#1 DONE 0.0s

#2 [internal] load .dockerignore
#2 transferring context: 2B done
#2 DONE 0.0s

#3 resolve image config for docker.io/tianon/buildkit:0.13
#3 DONE 0.2s

#4 docker-image://docker.io/tianon/buildkit:0.13@sha256:95db7c7b47f142af3fe0cae7de217996dadde8c44ef9e331ddd564857ca277ab
#4 resolve docker.io/tianon/buildkit:0.13@sha256:95db7c7b47f142af3fe0cae7de217996dadde8c44ef9e331ddd564857ca277ab 0.0s done
#4 CACHED

#5 [internal] load metadata for docker.io/library/bash:latest
#5 DONE 0.2s

#6 [1/2] FROM docker.io/library/bash:latest@sha256:ce062497c248eb1cf4d32927f8c1780cce158d3ed0658c586a5be7308d583cbb
#6 CACHED

#7 [2/2] RUN echo see, it works!
see, it works!
#7 DONE 0.3s

#8 exporting to image
#8 exporting layers 0.0s done
#8 writing image sha256:ceb5fb7b6904a68eeedd25c253e54e9ae1160c404c7e9fbb26c142d8a8143ef8 done
#8 DONE 0.0s

@tianon
Switch BUILDKIT_SYNTAX from to tianon/buildkit
8bbdde0 
Support for this was implemented in my images back in tianon/dockerfiles@8c85466 as a PoC (and support for it was kept in tianon/dockerfiles@2b78191 when the explicit 0.13 builds were added), and I've actually been using it pretty heavily since without issue, both locally *and* for all of my `tianon/xxx` personal image builds (including building of `tianon/buildkit` itself, for maximum meta 😂).

The primary benefit is that we get the exact same architecture support matrix as our BuildKit images (and the same set of applied patches, if they happen to impact the frontend code as well).

    $ docker buildx build --pull --no-cache --build-arg BUILDKIT_SYNTAX=tianon/buildkit:0.13 - <<<$'FROM bash\nRUN echo see, it works!'
    #0 building with "default" instance using docker driver

    #1 [internal] load build definition from Dockerfile
    #1 transferring dockerfile: 71B done
    #1 DONE 0.0s

    #2 [internal] load .dockerignore
    #2 transferring context: 2B done
    #2 DONE 0.0s

    docker-library#3 resolve image config for docker.io/tianon/buildkit:0.13
    docker-library#3 DONE 0.2s

    docker-library#4 docker-image://docker.io/tianon/buildkit:0.13@sha256:95db7c7b47f142af3fe0cae7de217996dadde8c44ef9e331ddd564857ca277ab
    docker-library#4 resolve docker.io/tianon/buildkit:0.13@sha256:95db7c7b47f142af3fe0cae7de217996dadde8c44ef9e331ddd564857ca277ab 0.0s done
    docker-library#4 CACHED

    docker-library#5 [internal] load metadata for docker.io/library/bash:latest
    docker-library#5 DONE 0.2s

    docker-library#6 [1/2] FROM docker.io/library/bash:latest@sha256:ce062497c248eb1cf4d32927f8c1780cce158d3ed0658c586a5be7308d583cbb
    docker-library#6 CACHED

    docker-library#7 [2/2] RUN echo see, it works!
    see, it works!
    docker-library#7 DONE 0.3s

    docker-library#8 exporting to image
    docker-library#8 exporting layers 0.0s done
    docker-library#8 writing image sha256:ceb5fb7b6904a68eeedd25c253e54e9ae1160c404c7e9fbb26c142d8a8143ef8 done
    docker-library#8 DONE 0.0s
@tianon tianon requested a review from a team as a code owner September 26, 2024 23:31
@github-actions GitHub Actions
Copy link

Diff for 8bbdde0: 
diff --git a/.external-pins/docker/dockerfile___1/bashbrew.json b/.external-pins/docker/dockerfile___1/bashbrew.json
deleted file mode 100644
index 353e7e1..0000000
diff --git a/.external-pins/docker/dockerfile___1/manifest-sha256_dc9e236567481e0aca4c1f52351af213b9a176622f10e3f4a86e5cc48919fa01-config.json b/.external-pins/docker/dockerfile___1/manifest-sha256_dc9e236567481e0aca4c1f52351af213b9a176622f10e3f4a86e5cc48919fa01-config.json
deleted file mode 100644
index cf9a292..0000000
diff --git a/.external-pins/docker/dockerfile___1/manifest-sha256_dc9e236567481e0aca4c1f52351af213b9a176622f10e3f4a86e5cc48919fa01.json b/.external-pins/docker/dockerfile___1/manifest-sha256_dc9e236567481e0aca4c1f52351af213b9a176622f10e3f4a86e5cc48919fa01.json
deleted file mode 100644
index d1087d3..0000000

whalelines
whalelines reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@whalelines whalelines left a comment

Choose a reason for hiding this comment

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

Are there any plans to move to more recent versions of BuildKit?

.bin/bashbrew-buildkit-env-setup.sh
@@ -47,7 +47,7 @@ _bashbrew_buildkit_env_setup() {
local vars='{}'

local dockerfileTag
dockerfileTag="$(grep <<<"$externalPins" -m1 '^docker/dockerfile:')"
dockerfileTag="$(grep <<<"$externalPins" -m1 '^tianon/buildkit:')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it called tianon/buildkit and not tianon/dockerfile?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the genesis of tianon/dockerfiles#733 -- it's a single image that includes both buildkitd and the dockerfile frontend, and uses context clues to decide which one to run automatically. The justification is that they're built from the same exact source tree, and the "dockerfiles" tags/releases have thus far been a strict subset of the buildkit tags/releases (which makes sense), so it's much easier to apply patches consistently if we build both from the same patched source tree, and at that point it's even easier to both build and use if they're the same image (it's really easy IMO to remember that tianon/buildkit is the buildkit image, no matter which context I'm needing a buildkit-related image in).

Copy link
Member Author

Choose a reason for hiding this comment

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

To perhaps illustrate what I mean about the tags better:

$ git ls-remote --tags https://github.com/moby/buildkit.git | grep 0865fcc9b78559e856e81dc52b3613701e7be28d
0865fcc9b78559e856e81dc52b3613701e7be28d	refs/tags/dockerfile/1.10.0
0865fcc9b78559e856e81dc52b3613701e7be28d	refs/tags/dockerfile/1.10.0-labs
0865fcc9b78559e856e81dc52b3613701e7be28d	refs/tags/v0.16.0

$ git tag -l | grep -E '^v0[.](1[3-6])[.]' | xargs -r git rev-parse | sort -u | xargs -rn1 git log -1 --oneline --decorate --format='format:%S %d'
0865fcc9b78559e856e81dc52b3613701e7be28d  (tag: v0.16.0, tag: dockerfile/1.10.0-labs, tag: dockerfile/1.10.0)
1981eb123dc979fc71d097adeb5bbb84110aa9f4  (tag: v0.13.0-beta3)
2ae42e0c0c793d7d66b7a23424af6fd6c2f9c8f3  (tag: v0.13.1)
2afc050d57d17983f3f662d5424c2725a35c60f4  (tag: v0.13.0, tag: dockerfile/1.7.0-labs, tag: dockerfile/1.7.0)
2e18d709fefdcc2db20853ee241c75b058189d39  (tag: v0.13.2, tag: dockerfile/1.7.1-labs, tag: dockerfile/1.7.1, origin/v0.13, v0.13)
331b5d5a4b0d5980e091eaf50f2bedd253cc8cc9  (tag: v0.13.0-rc1)
4d9a4e5df9e11596a3261c1952cde3c6346be762  (tag: v0.14.0, tag: dockerfile/1.8.0-labs, tag: dockerfile/1.8.0)
596ef8f01e11e15889576a88ffa4c7f92fa44518  (tag: v0.13.0-rc2, tag: dockerfile/1.7.0-rc1-labs, tag: dockerfile/1.7.0-rc1)
7b23ff0766d3c639a9a0ff9ff0af630089a12eb8  (tag: v0.13.0-rc3)
8ade9b239b580ff2af97b527e2484df60d6a2b6b  (tag: v0.15.0-rc1, tag: dockerfile/1.9.0-rc1-labs, tag: dockerfile/1.9.0-rc1)
979542e90f2cb38077c808e0867d8d2c16ed10b8  (tag: v0.15.1)
9e14164a1099d3e41b58fc879cbdd6f2b2edb04e  (tag: v0.15.2, origin/v0.15)
aebcc1f0eabcbaeef4be8e948641f653140fe2bf  (tag: v0.14.0-rc2, tag: dockerfile/1.8.0-rc2-labs, tag: dockerfile/1.8.0-rc2)
b9fcd7688a5d7d4461f0c1706216b6d4b3e584e6  (tag: v0.13.0-beta1)
c958c686e9ebf6b7d754881b92ba23b7859c5e59  (tag: v0.16.0-rc1, tag: dockerfile/1.10.0-rc1-labs, tag: dockerfile/1.10.0-rc1)
c9d08dde729d5ff0c7f6faf38b5bfd9ce8a753d5  (tag: v0.16.0-rc2)
e740d4b90ab98e1a10f3aae58d421bf135cff3cf  (tag: v0.14.0-rc1, tag: dockerfile/1.8.0-rc1-labs, tag: dockerfile/1.8.0-rc1)
e83d79a51fb49aeb921d8a2348ae14a58701c98c  (tag: v0.15.0-rc2, tag: v0.15.0, tag: dockerfile/1.9.0-rc2-labs, tag: dockerfile/1.9.0-rc2, tag: dockerfile/1.9.0-labs, tag: dockerfile/1.9.0)
eb864a84592468ee9b434326cb7efd66f58555af  (tag: v0.14.1, tag: dockerfile/1.8.1-labs, tag: dockerfile/1.8.1, origin/v0.14)
ef61ad133d7fd20632eb964dfc78292360f2f09d  (tag: v0.13.0-beta2)

@tianon
Copy link
Member Author

tianon commented Sep 27, 2024

Are there any plans to move to more recent versions of BuildKit?

Yes, we've definitely talked about it and want to do so -- honestly I think we're even more skittish about it than usual thanks to our recent permissions snafu (moby/buildkit#5066). It's really hard to feel confident in our efforts to validate newer versions because it's hard to know what to look for when validating. As we've discussed a few times, the "next" permissions snafu isn't going to look exactly like that, so while we could write a validation for that specific issue to increase our confidence, it's honestly not very likely we'll see that issue again, and the next thing is going to be something else.

That being said, I have been using tianon/buildkit:latest as both the buildkitd and the frontend for all my personal image builds (and when I need to build something locally), and haven't seen any obvious issues, so my confidence level is currently pretty high and I think we'd be reasonable to update. I also think that's a good justification for this PR, in that it makes it easier for us to apply patches to both halves (daemon and frontend) when we run into inevitable issues that are a priority for us but not necessarily a priority for upstream.

whalelines
whalelines approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@whalelines whalelines left a comment

Choose a reason for hiding this comment

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

LGTM

Wait until Monday to merge?

@tianon
Copy link
Member Author

tianon commented Sep 27, 2024

Yeah, I'd like to make sure @yosifkit doesn't see any issues with going ahead before we merge, so at least waiting until he gets back (and our current queue calms down) seems prudent 👍

@tianon
Copy link
Member Author

tianon commented Sep 28, 2024

"from to" -> can you tell this commit message used to have more detail in it? 🤦

(I'll fix this on ~Monday)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whalelines whalelines whalelines approved these changes

@yosifkit yosifkit Awaiting requested review from yosifkit

At least 0 approving reviews are required to merge this pull request.

Assignees

@yosifkit yosifkit

Labels
external/docker/dockerfile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tianon @whalelines @yosifkit