pkglib/build: make dockerRunner public #4173

christoph-zededa · 2025-09-12T12:24:22Z

- What I did

there is already a public method "WithBuildDocker", so it makes sense that the parameter definition is public as well so that a user of this method can actually use it

also allow dry-run with specific docker-runner

- How I did it

- How to verify it

no new feature was implemented, everything should work as before

- Description for the changelog

make dockerRunner public
untangle dockerRunner and DryRun

- A picture of a cute animal (not mandatory but encouraged)

(ChatGPT was very lazy today ...)

deitch

There are some comment changes (the find-and-replace got a little too aggressive), and one func that probably should be public as well, given the changes. Those are for the first commit.

For the second commit, the disentangle, I didn't quite get what the change is. Instead of a different docker runner, it calls pkglib.WithDryRun() and it calls pkglib.WithBuildDocker(pkglib.NewDockerDryRunner())? I don't get it

src/cmd/linuxkit/pkglib/dockerimpl.go

there is already a public method "WithBuildDocker", so it makes sense that the parameter definition is public as well so that a user of this method can actually use it Signed-off-by: Christoph Ostarek <[email protected]>

deitch · 2025-09-15T10:52:11Z

Thanks for cleaning those up. I still don't quite get the second commit, though.

christoph-zededa · 2025-09-15T10:54:49Z

Thanks for cleaning those up. I still don't quite get the second commit, though.

Perhaps if you have a look at https://github.com/lf-edge/eve/pull/5227/files#diff-d4026cc3de67a3f4fd7524ddfa23d059e96437dd33764d85de6c2f2dbeef2e42R100

If the second commit wouldn't be there, then https://github.com/lf-edge/eve/pull/5227/files#diff-d4026cc3de67a3f4fd7524ddfa23d059e96437dd33764d85de6c2f2dbeef2e42R100 would just overwrite the WithBuildDocker

deitch · 2025-09-15T11:02:33Z

The current way is:

you pass WithBuildDocker() if you want a specific docker runner, else it uses default (which it does in most cases)
you pass WithDryRun() to indicate dry run

You are trying to merge those together, so instead of WithDryRun(), you can just do WithBuildDocker(NewDryRunner())? But don't you still need WithDryRun() anyways, because of other parts, like here and here? So what does it buy us?

BTW, the first commit is good to go, and solves your immediate problem. If you want to move the second commit to its own PR slo it can be discussed, and not hold you up on your requirements, go for it.

christoph-zededa · 2025-09-15T11:04:03Z

If you want to move the second commit to its own PR slo it can be discussed, and not hold you up on your requirements, go for it.

I also need that commit.

deitch · 2025-09-15T11:05:10Z

I also need that commit.

It was a nice idea, though 😁

deitch · 2025-09-15T11:05:16Z

I also need that commit.

What for?

christoph-zededa · 2025-09-15T11:08:08Z

I also need that commit.

What for?

I need Build to return here: https://github.com/linuxkit/linuxkit/blob/master/src/cmd/linuxkit/pkglib/build.go#L506 otherwise it does some other things and returns an error.

deitch · 2025-09-15T11:11:56Z

But it does already, with WithDryRun(), which already exists. What does that have to do with the second commit?

christoph-zededa · 2025-09-15T11:15:37Z

WithDryRun()

The problem is that WithDryRun() also overwrites my WithBuildDocker (on master)

deitch · 2025-09-15T11:27:51Z

also overwrites my

Is that what the second commit is trying to do? In normal usage (no dry run), it respects your WithBuildDocker() and, if none provided, uses the default. If dry run and none provided, it uses the default dry run, but if you used WithBuildDocker(), it ignores it?

christoph-zededa · 2025-09-15T11:29:29Z

also overwrites my

Is that what the second commit is trying to do? In normal usage (no dry run), it respects your WithBuildDocker() and, if none provided, uses the default. If dry run and none provided, it uses the default dry run, but if you used WithBuildDocker(), it ignores it?

Yes, it gets overwritten here: https://github.com/linuxkit/linuxkit/blob/master/src/cmd/linuxkit/pkglib/build.go#L322

deitch · 2025-09-15T11:54:13Z

OK, now I get it. I will make a small suggestion, then we will be good to go.

deitch · 2025-09-15T11:54:42Z

src/cmd/linuxkit/pkg_build.go


 			if dryRun {
 				opts = append(opts, pkglib.WithDryRun())
+				opts = append(opts, pkglib.WithBuildDocker(pkglib.NewDockerDryRunner()))


Recommend getting rid of this, as this will override if someone both uses WithDryRun() and WithBuildDocker().

Wrong line?

This is in the cobra.Command ...

Okay, now with the other comment it makes sense to me

src/cmd/linuxkit/pkglib/build.go

this makes it possible for a user of this API to build their own DryRunner also make newDockerRunner public as well to be consistent Signed-off-by: Christoph Ostarek <[email protected]>

christoph-zededa mentioned this pull request Sep 12, 2025

get-deps: fix automatic hashes lf-edge/eve#5227

Merged

7 tasks

deitch requested changes Sep 14, 2025

View reviewed changes

src/cmd/linuxkit/pkglib/dockerimpl.go Show resolved Hide resolved

src/cmd/linuxkit/pkglib/dockerimpl.go Outdated Show resolved Hide resolved

src/cmd/linuxkit/pkglib/dockerimpl.go Outdated Show resolved Hide resolved

src/cmd/linuxkit/pkglib/dockerimpl.go Outdated Show resolved Hide resolved

christoph-zededa force-pushed the with_build_docker_public branch 2 times, most recently from a090dbb to b9a9795 Compare September 15, 2025 09:48

pkglib/build: make dockerRunner public

12a835b

there is already a public method "WithBuildDocker", so it makes sense that the parameter definition is public as well so that a user of this method can actually use it Signed-off-by: Christoph Ostarek <[email protected]>

christoph-zededa force-pushed the with_build_docker_public branch from b9a9795 to 0c7e48e Compare September 15, 2025 09:52

deitch reviewed Sep 15, 2025

View reviewed changes

src/cmd/linuxkit/pkglib/build.go Show resolved Hide resolved

pkglib: untangle WithBuildDocker and DryRun

c670e2d

this makes it possible for a user of this API to build their own DryRunner also make newDockerRunner public as well to be consistent Signed-off-by: Christoph Ostarek <[email protected]>

christoph-zededa force-pushed the with_build_docker_public branch from 0c7e48e to c670e2d Compare September 15, 2025 11:59

deitch approved these changes Sep 15, 2025

View reviewed changes

deitch merged commit 43200ea into linuxkit:master Sep 15, 2025
25 checks passed

pkglib/build: make dockerRunner public #4173

pkglib/build: make dockerRunner public #4173

Uh oh!

Conversation

christoph-zededa commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deitch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deitch commented Sep 15, 2025

Uh oh!

christoph-zededa commented Sep 15, 2025

Uh oh!

deitch commented Sep 15, 2025

Uh oh!

christoph-zededa commented Sep 15, 2025

Uh oh!

deitch commented Sep 15, 2025

Uh oh!

deitch commented Sep 15, 2025

Uh oh!

christoph-zededa commented Sep 15, 2025

Uh oh!

deitch commented Sep 15, 2025

Uh oh!

christoph-zededa commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deitch commented Sep 15, 2025

Uh oh!

christoph-zededa commented Sep 15, 2025

Uh oh!

deitch commented Sep 15, 2025

Uh oh!

deitch Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

christoph-zededa Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

christoph-zededa Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

christoph-zededa commented Sep 12, 2025 •

edited

Loading

christoph-zededa commented Sep 15, 2025 •

edited

Loading