Skip to content

Commit faab717

Browse files
author
John Howard
committed
Windows: Reduce CLI time, move some to unit tests
Signed-off-by: John Howard <jhoward@microsoft.com>
1 parent 8d4ccd1 commit faab717

File tree

9 files changed

+248
-260
lines changed

9 files changed

+248
-260
lines changed

‎builder/dockerfile/dispatchers.go‎

Lines changed: 5 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ package dockerfile
99

1010
import (
1111
"fmt"
12-
"os"
13-
"path/filepath"
1412
"regexp"
1513
"runtime"
1614
"sort"
@@ -20,7 +18,6 @@ import (
2018
"github.com/docker/docker/api"
2119
"github.com/docker/docker/builder"
2220
"github.com/docker/docker/pkg/signal"
23-
"github.com/docker/docker/pkg/system"
2421
runconfigopts "github.com/docker/docker/runconfig/opts"
2522
"github.com/docker/engine-api/types/container"
2623
"github.com/docker/engine-api/types/strslice"
@@ -257,45 +254,17 @@ func workdir(b *Builder, args []string, attributes map[string]bool, original str
257254
return errExactlyOneArgument("WORKDIR")
258255
}
259256

260-
if err := b.flags.Parse(); err != nil {
257+
err := b.flags.Parse()
258+
if err != nil {
261259
return err
262260
}
263261

264262
// This is from the Dockerfile and will not necessarily be in platform
265263
// specific semantics, hence ensure it is converted.
266-
workdir := filepath.FromSlash(args[0])
267-
current := filepath.FromSlash(b.runConfig.WorkingDir)
268-
if runtime.GOOS == "windows" {
269-
// Windows is a little more complicated than Linux. This code ensures
270-
// we end up with a workdir which is consistent in terms of platform
271-
// semantics. This means C:\somefolder, specifically in the format:
272-
// UPPERCASEDriveLetter-Colon-Backslash-FolderName. We are already
273-
// guaranteed that `current`, if set, is consistent. This allows us to
274-
// cope correctly with any of the following in a Dockerfile:
275-
// WORKDIR a --> C:\a
276-
// WORKDIR c:\\foo --> C:\foo
277-
// WORKDIR \\foo --> C:\foo
278-
// WORKDIR /foo --> C:\foo
279-
// WORKDIR c:\\foo \ WORKDIR bar --> C:\foo --> C:\foo\bar
280-
// WORKDIR C:/foo \ WORKDIR bar --> C:\foo --> C:\foo\bar
281-
// WORKDIR C:/foo \ WORKDIR \\bar --> C:\foo --> C:\bar
282-
// WORKDIR /foo \ WORKDIR c:/bar --> C:\foo --> C:\bar
283-
if len(current) == 0 || system.IsAbs(workdir) {
284-
if (workdir[0] == os.PathSeparator) ||
285-
(len(workdir) > 1 && string(workdir[1]) != ":") ||
286-
(len(workdir) == 1) {
287-
workdir = filepath.Join(`C:\`, workdir)
288-
}
289-
} else {
290-
workdir = filepath.Join(current, workdir)
291-
}
292-
workdir = strings.ToUpper(string(workdir[0])) + workdir[1:] // Upper-case drive letter
293-
} else {
294-
if !filepath.IsAbs(workdir) {
295-
workdir = filepath.Join(string(os.PathSeparator), current, workdir)
296-
}
264+
b.runConfig.WorkingDir, err = normaliseWorkdir(b.runConfig.WorkingDir, args[0])
265+
if err != nil {
266+
return err
297267
}
298-
b.runConfig.WorkingDir = workdir
299268

300269
return b.commit("", b.runConfig.Cmd, fmt.Sprintf("WORKDIR %v", workdir))
301270
}

‎builder/dockerfile/dispatchers_unix.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// +build !windows
2+
3+
package dockerfile
4+
5+
import (
6+
"fmt"
7+
"os"
8+
"path/filepath"
9+
)
10+
11+
// normaliseWorkdir normalises a user requested working directory in a
12+
// platform sematically consistent way.
13+
func normaliseWorkdir(current string, requested string) (string, error) {
14+
if requested == "" {
15+
return "", fmt.Errorf("cannot normalise nothing")
16+
}
17+
current = filepath.FromSlash(current)
18+
requested = filepath.FromSlash(requested)
19+
if !filepath.IsAbs(requested) {
20+
return filepath.Join(string(os.PathSeparator), current, requested), nil
21+
}
22+
return requested, nil
23+
}

‎builder/dockerfile/dispatchers_windows.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package dockerfile
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"path/filepath"
7+
"strings"
8+
9+
"github.com/docker/docker/pkg/system"
10+
)
11+
12+
// normaliseWorkdir normalises a user requested working directory in a
13+
// platform sematically consistent way.
14+
func normaliseWorkdir(current string, requested string) (string, error) {
15+
if requested == "" {
16+
return "", fmt.Errorf("cannot normalise nothing")
17+
}
18+
19+
current = filepath.FromSlash(current)
20+
requested = filepath.FromSlash(requested)
21+
22+
// Target semantics is C:\somefolder, specifically in the format:
23+
// UPPERCASEDriveLetter-Colon-Backslash-FolderName. We are already
24+
// guaranteed that `current`, if set, is consistent. This allows us to
25+
// cope correctly with any of the following in a Dockerfile:
26+
// WORKDIR a --> C:\a
27+
// WORKDIR c:\\foo --> C:\foo
28+
// WORKDIR \\foo --> C:\foo
29+
// WORKDIR /foo --> C:\foo
30+
// WORKDIR c:\\foo \ WORKDIR bar --> C:\foo --> C:\foo\bar
31+
// WORKDIR C:/foo \ WORKDIR bar --> C:\foo --> C:\foo\bar
32+
// WORKDIR C:/foo \ WORKDIR \\bar --> C:\foo --> C:\bar
33+
// WORKDIR /foo \ WORKDIR c:/bar --> C:\foo --> C:\bar
34+
if len(current) == 0 || system.IsAbs(requested) {
35+
if (requested[0] == os.PathSeparator) ||
36+
(len(requested) > 1 && string(requested[1]) != ":") ||
37+
(len(requested) == 1) {
38+
requested = filepath.Join(`C:\`, requested)
39+
}
40+
} else {
41+
requested = filepath.Join(current, requested)
42+
}
43+
// Upper-case drive letter
44+
return (strings.ToUpper(string(requested[0])) + requested[1:]), nil
45+
}

‎builder/dockerfile/dispatchers_windows_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// +build windows
2+
3+
package dockerfile
4+
5+
import "testing"
6+
7+
func TestNormaliseWorkdir(t *testing.T) {
8+
tests := []struct{ current, requested, expected, etext string }{
9+
{``, ``, ``, `cannot normalise nothing`},
10+
{``, `a`, `C:\a`, ``},
11+
{``, `c:\foo`, `C:\foo`, ``},
12+
{``, `\foo`, `C:\foo`, ``},
13+
{``, `/foo`, `C:\foo`, ``},
14+
{``, `C:/foo`, `C:\foo`, ``},
15+
{`C:\foo`, `bar`, `C:\foo\bar`, ``},
16+
{`C:\foo`, `/bar`, `C:\bar`, ``},
17+
{`C:\foo`, `\bar`, `C:\bar`, ``},
18+
}
19+
for _, i := range tests {
20+
r, e := normaliseWorkdir(i.current, i.requested)
21+
22+
if i.etext != "" && e == nil {
23+
t.Fatalf("TestNormaliseWorkingDir Expected error %s", i.etext)
24+
}
25+
26+
if i.etext != "" && e.Error() != i.etext {
27+
t.Fatalf("TestNormaliseWorkingDir Expected error %s, got %s", i.etext, e.Error())
28+
}
29+
30+
if r != i.expected {
31+
t.Fatalf("TestNormaliseWorkingDir Expected %s for %s %s", i.expected, i.current, i.requested)
32+
}
33+
}
34+
}

‎builder/dockerfile/internals.go‎

Lines changed: 2 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -200,60 +200,8 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
200200

201201
// Twiddle the destination when its a relative path - meaning, make it
202202
// relative to the WORKINGDIR
203-
204-
endsInSlash := strings.HasSuffix(dest, string(os.PathSeparator))
205-
206-
if runtime.GOOS == "windows" {
207-
// On Windows, this is more complicated. We are guaranteed that the
208-
// WorkingDir is already platform consistent meaning in the format
209-
// UPPERCASEDriveLetter-Colon-Backslash-Foldername. However, Windows
210-
// for now also has the limitation that ADD/COPY can only be done to
211-
// the C: (system) drive, not any drives that might be present as a
212-
// result of bind mounts.
213-
//
214-
// So... if the path specified is Linux-style absolute (/foo or \\foo),
215-
// we assume it is the system drive. If it is a Windows-style absolute
216-
// (DRIVE:\\foo), error if DRIVE is not C. And finally, ensure we
217-
// strip any configured working directories drive letter so that it
218-
// can be subsequently legitimately converted to a Windows volume-style
219-
// pathname.
220-
221-
// Not a typo - filepath.IsAbs, not system.IsAbs on this next check as
222-
// we only want to validate where the DriveColon part has been supplied.
223-
if filepath.IsAbs(dest) {
224-
if strings.ToUpper(string(dest[0])) != "C" {
225-
return fmt.Errorf("Windows does not support %s with a destinations not on the system drive (C:)", cmdName)
226-
}
227-
dest = dest[2:] // Strip the drive letter
228-
}
229-
230-
// Cannot handle relative where WorkingDir is not the system drive.
231-
if len(b.runConfig.WorkingDir) > 0 {
232-
if !system.IsAbs(b.runConfig.WorkingDir[2:]) {
233-
return fmt.Errorf("Current WorkingDir %s is not platform consistent", b.runConfig.WorkingDir)
234-
}
235-
if !system.IsAbs(dest) {
236-
if string(b.runConfig.WorkingDir[0]) != "C" {
237-
return fmt.Errorf("Windows does not support %s with relative paths when WORKDIR is not the system drive", cmdName)
238-
}
239-
240-
dest = filepath.Join(string(os.PathSeparator), b.runConfig.WorkingDir[2:], dest)
241-
242-
// Make sure we preserve any trailing slash
243-
if endsInSlash {
244-
dest += string(os.PathSeparator)
245-
}
246-
}
247-
}
248-
} else {
249-
if !system.IsAbs(dest) {
250-
dest = filepath.Join(string(os.PathSeparator), filepath.FromSlash(b.runConfig.WorkingDir), dest)
251-
252-
// Make sure we preserve any trailing slash
253-
if endsInSlash {
254-
dest += string(os.PathSeparator)
255-
}
256-
}
203+
if dest, err = normaliseDest(cmdName, b.runConfig.WorkingDir, dest); err != nil {
204+
return err
257205
}
258206

259207
for _, info := range infos {

‎builder/dockerfile/internals_unix.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// +build !windows
2+
3+
package dockerfile
4+
5+
import (
6+
"os"
7+
"path/filepath"
8+
"strings"
9+
10+
"github.com/docker/docker/pkg/system"
11+
)
12+
13+
// normaliseDest normalises the destination of a COPY/ADD command in a
14+
// platform semantically consistent way.
15+
func normaliseDest(cmdName, workingDir, requested string) (string, error) {
16+
dest := filepath.FromSlash(requested)
17+
endsInSlash := strings.HasSuffix(requested, string(os.PathSeparator))
18+
if !system.IsAbs(requested) {
19+
dest = filepath.Join(string(os.PathSeparator), filepath.FromSlash(workingDir), dest)
20+
// Make sure we preserve any trailing slash
21+
if endsInSlash {
22+
dest += string(os.PathSeparator)
23+
}
24+
}
25+
return dest, nil
26+
}

‎builder/dockerfile/internals_windows.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
package dockerfile
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"path/filepath"
7+
"strings"
8+
9+
"github.com/docker/docker/pkg/system"
10+
)
11+
12+
// normaliseDest normalises the destination of a COPY/ADD command in a
13+
// platform semantically consistent way.
14+
func normaliseDest(cmdName, workingDir, requested string) (string, error) {
15+
dest := filepath.FromSlash(requested)
16+
endsInSlash := strings.HasSuffix(dest, string(os.PathSeparator))
17+
18+
// We are guaranteed that the working directory is already consistent,
19+
// However, Windows also has, for now, the limitation that ADD/COPY can
20+
// only be done to the system drive, not any drives that might be present
21+
// as a result of a bind mount.
22+
//
23+
// So... if the path requested is Linux-style absolute (/foo or \\foo),
24+
// we assume it is the system drive. If it is a Windows-style absolute
25+
// (DRIVE:\\foo), error if DRIVE is not C. And finally, ensure we
26+
// strip any configured working directories drive letter so that it
27+
// can be subsequently legitimately converted to a Windows volume-style
28+
// pathname.
29+
30+
// Not a typo - filepath.IsAbs, not system.IsAbs on this next check as
31+
// we only want to validate where the DriveColon part has been supplied.
32+
if filepath.IsAbs(dest) {
33+
if strings.ToUpper(string(dest[0])) != "C" {
34+
return "", fmt.Errorf("Windows does not support %s with a destinations not on the system drive (C:)", cmdName)
35+
}
36+
dest = dest[2:] // Strip the drive letter
37+
}
38+
39+
// Cannot handle relative where WorkingDir is not the system drive.
40+
if len(workingDir) > 0 {
41+
if ((len(workingDir) > 1) && !system.IsAbs(workingDir[2:])) || (len(workingDir) == 1) {
42+
return "", fmt.Errorf("Current WorkingDir %s is not platform consistent", workingDir)
43+
}
44+
if !system.IsAbs(dest) {
45+
if string(workingDir[0]) != "C" {
46+
return "", fmt.Errorf("Windows does not support %s with relative paths when WORKDIR is not the system drive", cmdName)
47+
}
48+
dest = filepath.Join(string(os.PathSeparator), workingDir[2:], dest)
49+
// Make sure we preserve any trailing slash
50+
if endsInSlash {
51+
dest += string(os.PathSeparator)
52+
}
53+
}
54+
}
55+
return dest, nil
56+
}

‎builder/dockerfile/internals_windows_test.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// +build windows
2+
3+
package dockerfile
4+
5+
import "testing"
6+
7+
func TestNormaliseDest(t *testing.T) {
8+
tests := []struct{ current, requested, expected, etext string }{
9+
{``, `D:\`, ``, `Windows does not support TEST with a destinations not on the system drive (C:)`},
10+
{``, `e:/`, ``, `Windows does not support TEST with a destinations not on the system drive (C:)`},
11+
{`invalid`, `./c1`, ``, `Current WorkingDir invalid is not platform consistent`},
12+
{`C:`, ``, ``, `Current WorkingDir C: is not platform consistent`},
13+
{`C`, ``, ``, `Current WorkingDir C is not platform consistent`},
14+
{`D:\`, `.`, ``, "Windows does not support TEST with relative paths when WORKDIR is not the system drive"},
15+
{``, `D`, `D`, ``},
16+
{``, `./a1`, `.\a1`, ``},
17+
{``, `.\b1`, `.\b1`, ``},
18+
{``, `/`, `\`, ``},
19+
{``, `\`, `\`, ``},
20+
{``, `c:/`, `\`, ``},
21+
{``, `c:\`, `\`, ``},
22+
{``, `.`, `.`, ``},
23+
{`C:\wdd`, `./a1`, `\wdd\a1`, ``},
24+
{`C:\wde`, `.\b1`, `\wde\b1`, ``},
25+
{`C:\wdf`, `/`, `\`, ``},
26+
{`C:\wdg`, `\`, `\`, ``},
27+
{`C:\wdh`, `c:/`, `\`, ``},
28+
{`C:\wdi`, `c:\`, `\`, ``},
29+
{`C:\wdj`, `.`, `\wdj`, ``},
30+
{`C:\wdk`, `foo/bar`, `\wdk\foo\bar`, ``},
31+
{`C:\wdl`, `foo\bar`, `\wdl\foo\bar`, ``},
32+
{`C:\wdm`, `foo/bar/`, `\wdm\foo\bar\`, ``},
33+
{`C:\wdn`, `foo\bar/`, `\wdn\foo\bar\`, ``},
34+
}
35+
for _, i := range tests {
36+
got, err := normaliseDest("TEST", i.current, i.requested)
37+
if err != nil && i.etext == "" {
38+
t.Fatalf("TestNormaliseDest Got unexpected error %q for %s %s. ", err.Error(), i.current, i.requested)
39+
}
40+
if i.etext != "" && ((err == nil) || (err != nil && err.Error() != i.etext)) {
41+
if err == nil {
42+
t.Fatalf("TestNormaliseDest Expected an error for %s %s but didn't get one", i.current, i.requested)
43+
} else {
44+
t.Fatalf("TestNormaliseDest Wrong error text for %s %s - %s", i.current, i.requested, err.Error())
45+
}
46+
}
47+
if i.etext == "" && got != i.expected {
48+
t.Fatalf("TestNormaliseDest Expected %q for %q and %q. Got %q", i.expected, i.current, i.requested, got)
49+
}
50+
}
51+
}

0 commit comments

Comments
 (0)