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

Ensure Dockerfile WORKDIR C:\ becomes / in LLB #1621

Closed
wants to merge 1 commit into from
Closed

Ensure Dockerfile WORKDIR C:\ becomes / in LLB #1621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions frontend/dockerfile/dockerfile2llb/convert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
package dockerfile2llb

import (
"bytes"
"context"
"runtime"
"testing"

"github.com/moby/buildkit/client/llb"
"github.com/moby/buildkit/frontend/dockerfile/instructions"
"github.com/moby/buildkit/frontend/dockerfile/shell"
"github.com/moby/buildkit/frontend/dockerui"
"github.com/moby/buildkit/solver/pb"
"github.com/moby/buildkit/util/appcontext"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func toEnvMap(args []instructions.KeyValuePairOptional, env []string) map[string]string {
Expand Down Expand Up @@ -208,3 +213,99 @@ COPY --from=stage1 f2 /sub/
_, _, _, err = Dockerfile2LLB(appcontext.Context(), []byte(df), ConvertOpt{})
assert.EqualError(t, err, "circular dependency detected on stage: stage0")
}

func TestDockerfileWorkdirPathFormat(t *testing.T) {
// Ensure that paths seen in the Dockerfile, which may be in system-local format,
// are `/`-formatted in LLB, for in-container or context-relative use.

caps := pb.Caps.CapSet(pb.Caps.All())
convertOpts := ConvertOpt{
LLBCaps: &caps,
}

expectedState := llb.Scratch().Dir("/test").File(llb.Mkdir("/test", 0755, llb.WithParents(true)))
expectedDef, err := expectedState.Marshal(appcontext.Context(), llb.WithCaps(caps))
require.NoError(t, err)

df := `FROM scratch
WORKDIR /test
`
state, _, err := Dockerfile2LLB(appcontext.Context(), []byte(df), convertOpts)
require.NoError(t, err)

workdir, err := state.GetDir(appcontext.Context())
require.NoError(t, err)
assert.Equal(t, "/test", workdir)

def, err := state.Marshal(appcontext.Context(), llb.WithCaps(caps))
require.NoError(t, err)
require.Equal(t, len(expectedDef.Metadata), len(def.Metadata))
require.Equal(t, len(expectedDef.Def), len(def.Def))

for i := 0; i < len(expectedDef.Def); i++ {
res := bytes.Compare(expectedDef.Def[i], def.Def[i])
assert.Equalf(t, res, 0, "def %d expected %v, got %v", i, expectedDef.Def[i], def.Def[i])
}

if runtime.GOOS == "windows" {
df := `FROM scratch
WORKDIR \test
`
state, _, err := Dockerfile2LLB(appcontext.Context(), []byte(df), convertOpts)
assert.NoError(t, err)

workdir, err := state.GetDir(appcontext.Context())
assert.NoError(t, err)
assert.Equal(t, "/test", workdir)

def, err := state.Marshal(appcontext.Context(), llb.WithCaps(caps))
require.NoError(t, err)
require.Equal(t, len(expectedDef.Metadata), len(def.Metadata))
require.Equal(t, len(expectedDef.Def), len(def.Def))

for i := 0; i < len(expectedDef.Def); i++ {
res := bytes.Compare(expectedDef.Def[i], def.Def[i])
assert.Equalf(t, res, 0, "def %d expected %v, got %v", i, expectedDef.Def[i], def.Def[i])
}

df = `FROM scratch
WORKDIR C:/test
`
state, _, err = Dockerfile2LLB(appcontext.Context(), []byte(df), convertOpts)
assert.NoError(t, err)

workdir, err = state.GetDir(appcontext.Context())
assert.NoError(t, err)
assert.Equal(t, "/test", workdir)

def, err = state.Marshal(appcontext.Context(), llb.WithCaps(caps))
require.NoError(t, err)
require.Equal(t, len(expectedDef.Metadata), len(def.Metadata))
require.Equal(t, len(expectedDef.Def), len(def.Def))

for i := 0; i < len(expectedDef.Def); i++ {
res := bytes.Compare(expectedDef.Def[i], def.Def[i])
assert.Equalf(t, res, 0, "def %d expected %v, got %v", i, expectedDef.Def[i], def.Def[i])
}

df = `FROM scratch
WORKDIR C:\test
`
state, _, err = Dockerfile2LLB(appcontext.Context(), []byte(df), convertOpts)
assert.NoError(t, err)

workdir, err = state.GetDir(appcontext.Context())
assert.NoError(t, err)
assert.Equal(t, "/test", workdir)

def, err = state.Marshal(appcontext.Context(), llb.WithCaps(caps))
require.NoError(t, err)
require.Equal(t, len(expectedDef.Metadata), len(def.Metadata))
require.Equal(t, len(expectedDef.Def), len(def.Def))

for i := 0; i < len(expectedDef.Def); i++ {
res := bytes.Compare(expectedDef.Def[i], def.Def[i])
assert.Equalf(t, res, 0, "def %d expected %v, got %v", i, expectedDef.Def[i], def.Def[i])
}
}
}
10 changes: 9 additions & 1 deletion frontend/dockerfile/instructions/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package instructions

import (
"fmt"
"path/filepath"
"regexp"
"sort"
"strconv"
Expand All @@ -17,6 +18,7 @@ import (
"github.com/moby/buildkit/frontend/dockerfile/command"
"github.com/moby/buildkit/frontend/dockerfile/parser"
"github.com/moby/buildkit/util/suggest"
"github.com/moby/buildkit/util/system"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -409,8 +411,14 @@ func parseWorkdir(req parseRequest) (*WorkdirCommand, error) {
if err != nil {
return nil, err
}

path, err := system.CheckSystemDriveAndRemoveDriveLetter(req.args[0])
if err != nil {
return nil, err
}

return &WorkdirCommand{
Path: req.args[0],
Path: filepath.ToSlash(path),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of using filepath and making decisions based on the native architecture, it would be better to move this to dockerfile2llb and use the platform of the dispatchstate

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Line 546 in 3f3957d

platform *specs.Platform
to make the decision about converting paths. Would like that windows dockerfile that doesn't use RUN would work on Linux as well, like it works today if unix paths are used.

This looks somewhat similar to #1560 that you also might want to take.

Copy link
Collaborator Author

@TBBle TBBle Aug 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

To be clear, is the idea that WORKDIR C:\test should work when running a Linux-based build against the Windows targetPlatform? That seems reasonable, but will require a little bit more learning on my part.

I assume WORKDIR C:\test should fail if the targetPlatform is Linux... Or rather, that's possibly a valid filename, but it's relative, and it's the filesystem layer's problem to reject it.

I like this more, because one of the outstanding concerns I had with my tests was the runtime platform check. I can split the current test into two, and confirm that we get rational behaviour for both Linux and Windows targetPlatform, irrespective of the execution platform.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, is the idea that WORKDIR C:\test should work when running a Linux-based build against the Windows targetPlatform?

Yes. This works today (outside Moby integration). But I think today you just always need to use unix paths that should work even if you build for windows. Normally, I would just always recommend using unix paths in Dockerfiles but because old builder already supports windows paths BuildKit should as well. But the same Dockerfile should be usable for building natively on Windows and cross-building on Linux.

I assume WORKDIR C:\test should fail if the targetPlatform is Linux...

Yes, we shouldn't try to do any normalization magic for linux targets. If target is windows we can check if it is a windows style path and normalize to unix path if needed.

withNameAndCode: newWithNameAndCode(req),
}, nil
}
Expand Down