local: fix platform-split=true option

[tonistiigi]

Currently the option only worked when platform-split=false was set for multi-platform build but not for setting single platform build to use platform-split=true.

docker/buildx#3215

[tonistiigi]

@profnandaa Any idea what happened to the windows tests here?

[profnandaa]

@profnandaa Any idea what happened to the windows tests here?

Taking a look rn.

[profnandaa]

We missed that path that calls outputFS.Walk since it wasn't being hit on master.

(dlv)
> github.com/moby/buildkit/exporter/local.(*localExporterInstance).Export.func2.1() C:/dev/core-containers/buildkit/exporter/local/export.go:128 (PC: 0x224b379)
   123:                         }
   124:                         if cleanup != nil {
   125:                                 defer cleanup()
   126:                         }
   127:
=> 128:                         if !e.opts.PlatformSplit {
   129:                                 // check for duplicate paths
   130:                                 err = outputFS.Walk(ctx, "", func(p string, entry os.DirEntry, err error) error {
   131:                                         if entry.IsDir() {
   132:                                                 return nil
   133:                                         }
   
(dlv) p e.opts.PlatformSplit
true

Cross-checking with the rest of the exporters then send a patch.

Ref #4994

[profnandaa]

PFA the patch to your branch, ran on my fork here

0001-fix-wcow-privileges-for-local-exporter-fs-walk.patch

[tonistiigi]

@profnandaa So previously this code path did not run for windows?

[profnandaa]

@profnandaa So previously this code path did not run for windows?

Yes, did not run previously.

[crazy-max]

@profnandaa This test failure looks related to this commit 30b572b

https://github.com/moby/buildkit/actions/runs/15430742302/job/43437060882?pr=6007#step:8:307

=== FAIL: client TestIntegration/slice=3-4/TestExportLocalForcePlatformSplit/worker=containerd (0.89s)
    client_test.go:6905: 
        	Error Trace:	D:/a/buildkit/buildkit/client/client_test.go:6905
        	            				D:/a/buildkit/buildkit/util/testutil/integration/run.go:103
        	            				D:/a/buildkit/buildkit/util/testutil/integration/run.go:251
        	Error:      	"[]" should have 1 item(s), but has 0
        	Test:       	TestIntegration/slice=3-4/TestExportLocalForcePlatformSplit/worker=containerd
        	Messages:   	expected one files in the output directory

[profnandaa]

@crazy-max -- I doubt... I've tried to dig through to find where the failure is happening but can't find it since it fails just silently. Same happens even if I revert my change.

Ran the debugger till session/filesync/diffcopy_windows.go:19, but can't find anything.

buildctl build --frontend dockerfile.v0 --local context=. --local dockerfile=. --output type=local,dest=./output,platform-split=true
--
#10 exporting to client directory
#10 copying files windows(10.0.22631)/amd64
#10 copying files windows(10.0.22631)/amd64 35.6s done
#10 DONE 43.6s
--
// but then wehn you check the ./output directory, it's empty

// it writes the output correctly when platform-split=false

Any ideas?

[crazy-max]

#10 copying files windows(10.0.22631)/amd64
#10 copying files windows(10.0.22631)/amd64 35.6s done

Hum wonder if it could be the folder name with os version 🤔

[profnandaa]

Hum wonder if it could be the folder name with os version 🤔

That was my first suspicion but windows(10.0.22631)_amd64 is still a valid Windows directory. Adding explicit --opt platform=windows/amd64 still yields same results, so not really the directory...

[tonistiigi]

@profnandaa Do the platform directories work correctly for wcow on multi-platform builds that don't require platform-split?

[profnandaa]

@profnandaa Do the platform directories work correctly for wcow on multi-platform builds that don't require platform-split?

Since we don't have multi-platform build support yet, I'm unable to test this.

I was just about to write to you that since this is actually not needed at the moment coz of the single-platform support, I suggest we skip the test on Windows for now; as I continue with the investigation.

[profnandaa]

PFA the patch to your branch, ran on my fork here

0001-fix-wcow-privileges-for-local-exporter-fs-walk.patch

@tonistiigi -- this is besides our current issue. I just noticed that you cherry-picked a slightly older fix from my branch then (if you see the patch). I have done a refactor here to match, will be nice if you can pick that - profnandaa@780b19b

[profnandaa]

@profnandaa Do the platform directories work correctly for wcow on multi-platform builds that don't require platform-split?

Since we don't have multi-platform build support yet, I'm unable to test this.

I was just about to write to you that since this is actually not needed at the moment coz of the single-platform support, I suggest we skip the test on Windows for now; as I continue with the investigation.

@crazy-max @tonistiigi -- I think I've finally found where the bug is at last, in fsutil after the call to fsutil.Sent() at session/filesync/diffcopy_windows.go:19:

> github.com/tonistiigi/fsutil.(*subDirFS).Walk() C:/dev/core-containers/buildkit/vendor/github.com/tonistiigi/fsutil/fs.go:125 (PC: 0x1501600)
   120: func (fs *subDirFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc) error {
   121:         first, rest, _ := strings.Cut(target, string(filepath.Separator))
   122:
   123:         for _, d := range fs.dirs {
   124:                 if first != "" && first != d.Stat.Path {
=> 125:                         continue
   126:                 }
   127:
   128:                 fi := &StatInfo{d.Stat.Clone()}
   129:                 if !fi.IsDir() {
   130:                         return errors.WithStack(&os.PathError{Path: d.Stat.Path, Err: syscall.ENOTDIR, Op: "walk subdir"})
(dlv) p first
"/"
(dlv) p rest
""
(dlv) p target
"/"
(dlv) p d.Stat.Path
"windows_amd64"

filepath.Separator here is \\, while target=/.

Exploring how best to fix...

[profnandaa]

Looks like we'll have to fix from the fsutil side.

https://github.com/tonistiigi/fsutil/blob/3f76f81301443083e015c78ea5526da2823bf419/send.go#L151

err := s.fs.Walk(ctx, "/", func(path string, entry os.DirEntry, err error) error {

Already target = "/" and we are using the platform-specific separator later on at fsutil/fs.go:121. Should we just use / instead of filepath.separator or switch the target to the platform-specific /? Tested both approaches, work okay.

diff --git a/vendor/github.com/tonistiigi/fsutil/send.go b/vendor/github.com/tonistiigi/fsutil/send.go
index e4a315638..6584ccef7 100644
--- a/vendor/github.com/tonistiigi/fsutil/send.go
+++ b/vendor/github.com/tonistiigi/fsutil/send.go
@@ -148,7 +148,7 @@ func (s *sender) sendFile(h *sendHandle) error {

 func (s *sender) walk(ctx context.Context) error {
        var i uint32 = 0
-       err := s.fs.Walk(ctx, "/", func(path string, entry os.DirEntry, err error) error {
+       err := s.fs.Walk(ctx, string(filepath.Separator), func(path string, entry os.DirEntry, err error) error {
                if err != nil {
                        return err
                }

[profnandaa]

how do we deal with the windows case where the build number is added to the os name? Unless you explicitly specify --opt platform=..., eg.

#10 exporting to client directory
#10 copying files windows(10.0.22631)/amd64
#10 copying files windows(10.0.22631)/amd64 6.01MB 11.4s
#10 copying files windows(10.0.22631)/amd64 150.83MB 16.5s
#10 copying files windows(10.0.22631)/amd64 239.47MB 23.7s
#10 copying files windows(10.0.22631)/amd64 239.47MB 23.9s done
#10 DONE 24.2s

should we normalize that from the code?

[tonistiigi]

are these () invalid names for windows?

[profnandaa]

they are not invalid, I think this change was brought in some time back here - containerd/platforms#6

[profnandaa]

what do you think about this? profnandaa@3fc6393

[crazy-max]

Rebased to include #6017

[profnandaa]

Rebased to include #6017

@crazy-max -- the failure now is coz the directory is named differently on windows with a (version) suffix, not suffix but something like windows(10.0.22631)_amd64, I'd proposed this - #6007 (comment)

[crazy-max]

Rebased to include #6017

@crazy-max -- the failure now is coz the directory is named differently on windows with a (version) suffix, not suffix but something like windows(10.0.22631)_amd64,

I have updated the test to use platforms.DefaultSpec() instead of runtime consts (see last commit).

@tonistiigi Feel free to squash with your commit.

I'd proposed this - #6007 (comment)

I agree that it would be better to strip the os version but I think we should just change the key here:

buildkit/exporter/local/export.go

Line 184 in 25c7c80

eg.Go(export(ctx, p.ID, r, inp.Attestations[p.ID]))

Instead of p.ID we could use platforms.Format(p.Platform).

We can look at this as follow-up.

[profnandaa]

PFA the patch to your branch, ran on my fork here
0001-fix-wcow-privileges-for-local-exporter-fs-walk.patch

@tonistiigi -- this is besides our current issue. I just noticed that you cherry-picked a slightly older fix from my branch then (if you see the patch). I have done a refactor here to match, will be nice if you can pick that - profnandaa@780b19b

@crazy-max One last thing, can cherry pick this refactor instead? Tonis has picked a slightly older version from my branch, I'd thought he'll use my patch file.

[crazy-max]

One last thing, can cherry pick this refactor instead?

Done, thanks!

[crazy-max]

@ArthurFlag Needs docs follow-up in https://docs.docker.com/build/exporters/local-tar/ with some examples as well similar to what we have in BuildKit README but with Buildx: https://github.com/moby/buildkit?tab=readme-ov-file#local-directory