Skip to content

Conversation

@cpuguy83
Copy link
Collaborator

@cpuguy83 cpuguy83 commented Jan 7, 2025

Allows windowscross to produce a single multiplatform image using the specified base images.

Supersedes #480
Closes #480

@cpuguy83 cpuguy83 force-pushed the multi-base branch 6 times, most recently from 65d3fec to 87ac6a8 Compare January 9, 2025 00:22
@cpuguy83 cpuguy83 force-pushed the multi-base branch 2 times, most recently from 81e57da to cabf8c6 Compare January 9, 2025 00:50
@cpuguy83 cpuguy83 marked this pull request as ready for review January 9, 2025 16:00
@cpuguy83 cpuguy83 requested a review from a team as a code owner January 9, 2025 16:00
// which means we end up overwriting map keys when there are multiple windows
// platform images being output but with different OSVerions.
// platforms.FormatAll takes OSVersion into account.
func dcBuild(ctx context.Context, bc *dockerui.Client, fn dockerui.BuildFunc) (*resultBuilder, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really a good idea to fork this code? I generally think that's not a good idea, as we won't benefit from upstream changes if there are any.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to fork it to get the functionality we need.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can these changes be submitted upstream? Then we wouldn't need to fork it.

I'm sure there's a way to preserve the old behavior while allowing the new.

Copy link
Contributor

@pmengelbert pmengelbert Jan 13, 2025

Choose a reason for hiding this comment

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

Here is how it could be done:
pmengelbert/buildkit@7552611

Whether or not that's acceptible upstream, I think we should at least commit to a plan to undo the fork if there's no practical way around forking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moby/buildkit#5614 upstreams this work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a link to moby/buildkit#5614 in the comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upstream is merged.
Looks like you have an issue already to unfork this. Seems like should be good?

@cpuguy83 cpuguy83 requested a review from pmengelbert January 13, 2025 17:27
Copy link
Contributor

@MorrisLaw MorrisLaw left a comment

Choose a reason for hiding this comment

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

First pass lgtm! Don't have a whole lot of context though. Left some nits and clarification questions.

This is pretty much to support building for multiple windows versions
from windowscross but can also be useful (in the future) for allowing
users to specify something other than a registry reference to provide an
image.

The actual support for multiple bases, where it can be supported, will
come in a follow-up commit.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This is useful for when you want to have one image for different
versions of Windows, given that for Windows containers the container OS
must match the host OS.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 merged commit 1528f1a into project-dalec:main Jan 21, 2025
19 checks passed
@cpuguy83 cpuguy83 deleted the multi-base branch January 21, 2025 19:44
@cpuguy83 cpuguy83 added this to the v0.12.0 milestone Jan 29, 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.

3 participants