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

Add stub implementations to make buildkitd buildable for Darwin #5276

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

slonopotamus
Copy link
Contributor

@slonopotamus slonopotamus commented Aug 29, 2024

These changes are extracted from #4059. Other half was already merged in #5271.

localmounter_darwin.go is fully identical to localmounter_freebsd.go except that it uses bind mount type instead of FreeBSD-specific nullfs.

Not sure whether you want to enable CI job for Darwin buildkitd, I'm absolutely OK if that will be omitted from current PR.

Dockerfile Outdated
@@ -175,6 +175,7 @@ COPY --link --from=buildctl /usr/bin/buildctl /
COPY --link --from=buildkitd /usr/bin/buildkitd /

FROM scratch AS binaries-darwin
COPY --link --from=buildkitd /usr/bin/buildkitd /
Copy link
Member

Choose a reason for hiding this comment

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

We do not want to ship stub buildkitd binary for macos users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will remove from here. Do you want me to add another job that would check that it compiles?

Copy link
Member

Choose a reason for hiding this comment

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

You can add it to this matrix:

platform:
- windows/amd64
- freebsd/amd64

      matrix:
        platform:
          - windows/amd64
          - freebsd/amd64
          - darwin/amd64

Copy link
Contributor Author

@slonopotamus slonopotamus Sep 2, 2024

Choose a reason for hiding this comment

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

If I read things properly, that job goes through the same Dockerfile target (binaries-darwin), so it won't build buildkitd.

Copy link
Member

Choose a reason for hiding this comment

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

Hum right it will build binaries-for-test target:

targets: binaries-for-test

buildkit/docker-bake.hcl

Lines 102 to 106 in ea6f91e

target "binaries-for-test" {
inherits = ["_common"]
target = "binaries-for-test"
output = [bindir("build")]
}

buildkit/Dockerfile

Lines 367 to 371 in ea6f91e

FROM scratch AS binaries-for-test
COPY --link --from=gotestsum /out /
COPY --link --from=registry /out /
COPY --link --from=containerd /out /
COPY --link --from=binaries / /

We would just want buildctl and buildkitd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Soo... Do you want me to add COPY --link --from=buildkitd /usr/bin/buildkitd / to binaries-for-test? It will do redundant work for other platforms, but I don't see any other good way.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can create a dedicated job in test-os workflow to build buildkitd target for darwin/amd64 platform. I can push these changes to your branch if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can push these changes to your branch if you want.

That would be cool!

steps:
-
name: Prepare
run: |
platform=${{ matrix.platform }}
echo "PLATFORM_PAIR=${platform//\//-}" >> $GITHUB_ENV
if [ "${platform}" = "darwin/amd64" ]; then
echo "BUILD_TARGET=buildkitd" >> $GITHUB_ENV
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crazy-max It looks like you stopped building buildctl this way. I have no experience with docker/bake-action, is it possible to ask it to build multiple targets?

Copy link
Member

@crazy-max crazy-max Sep 2, 2024

Choose a reason for hiding this comment

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

I changed the workflow to create a dedicated job instead to just build buildkitd. buildctl is already built here:

e.g., https://github.com/moby/buildkit/actions/runs/10665890596/job/29560219583

Copy link
Contributor Author

@slonopotamus slonopotamus Sep 2, 2024

Choose a reason for hiding this comment

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

Looks good, thanks for help

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

So what's the benefit of this? I would get if some packages are useful for darwin and we want to make sure that they are buildable. But why build a broken buildkitd. Why make executor "buildable" when there really is no implementation. Why make source/git "buildable" while with this PR it would just fail on runtime as soon as it would get called?

We have no intention of actually supporting macos as target via some non-container execution mechanism https://github.com/moby/buildkit/blob/master/PROJECT.md#project-scope .

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Sep 11, 2024

WRT source/git: I believe it is a very marginal functionality that can be added at a later point when anyone actually needs it.

WRT executor: I do not agree that there is no implementation. Everything that buildkit needs is to call containerd (and current PR enables containerd worker). There are multiple containerd runtimes that work on Darwin, for example https://github.com/ukontainer/runu/

I do not expect any significant Darwin-specific changes to buildkit besides current PR.

)

func runWithStandardUmask(_ context.Context, _ *exec.Cmd) error {
return errors.New("runWithStandardUmask not supported on " + runtime.GOOS)
Copy link
Member

Choose a reason for hiding this comment

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

make this use the freebsd implementation

@tonistiigi tonistiigi merged commit 734a6cc into moby:master Sep 16, 2024
91 checks passed
@slonopotamus slonopotamus deleted the darwin branch September 17, 2024 04:20
@slonopotamus
Copy link
Contributor Author

Looks like CI failed on master branch after this PR was merged. I believe it is not related to my changes, and is caused by hashicorp/vagrant#13501 and the fact that FreeBSD job doesn't lock specific versions of tools (vargant+virtualbox).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants