Skip to content
This repository was archived by the owner on Oct 13, 2023. It is now read-only.

Conversation

thaJeztah
Copy link
Member

backport of:

fixes docker/for-win#4349
built on top of moby#39698

This fixes a regression introduced in 6d87f19 (moby#38599),
causing COPY --from to fail if the target directory does not exist:

FROM mcr.microsoft.com/windows/servercore:ltsc2019 as s1
RUN echo "Hello World" > /hello

FROM mcr.microsoft.com/windows/servercore:ltsc2019
COPY --from=s1 /hello /hello/another/world

Would produce an error:

Step 4/4 : COPY --from=s1 /hello /hello/another/world
failed to copy files: mkdir \\?: The filename, directory name, or volume label syntax is incorrect.

The cause for this was that Go's os.MkdirAll() does not support/detect volume GUID paths
(\\?\Volume{dae8d3ac-b9a1-11e9-88eb-e8554b2ba1db}\hello\another}), and as a result
attempted to create the volume as a directory (\\?), causing it to fail.

This patch replaces os.MkdirAll() with our own system.MkdirAll() function, which
is capable of detecting GUID volumes.

- How to verify it

To verify (on Linux; don't know the equivalent on Windows, LOL):

make TEST_FILTER=TestBuildMultiStageCopy test-integration

...
INFO: Testing against a local daemon
=== RUN   TestBuildMultiStageCopy
=== RUN   TestBuildMultiStageCopy/copy_to_root
=== RUN   TestBuildMultiStageCopy/copy_to_newdir
=== RUN   TestBuildMultiStageCopy/copy_to_newdir_nested
=== RUN   TestBuildMultiStageCopy/copy_to_existingdir
=== RUN   TestBuildMultiStageCopy/copy_to_newsubdir
--- PASS: TestBuildMultiStageCopy (11.32s)
    --- PASS: TestBuildMultiStageCopy/copy_to_root (5.76s)
    --- PASS: TestBuildMultiStageCopy/copy_to_newdir (1.33s)
    --- PASS: TestBuildMultiStageCopy/copy_to_newdir_nested (1.41s)
    --- PASS: TestBuildMultiStageCopy/copy_to_existingdir (1.34s)
    --- PASS: TestBuildMultiStageCopy/copy_to_newsubdir (1.48s)
PASS
...

- Description for the changelog

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

@thaJeztah thaJeztah added this to the 19.03.2 milestone Aug 8, 2019
@thaJeztah thaJeztah requested a review from tonistiigi as a code owner August 8, 2019 14:49
@thaJeztah
Copy link
Member Author

@ddebroy @StefanScherer @kolyshkin ptal

@thaJeztah
Copy link
Member Author

Ah, failing, because #313 isn't merged yet

This function changed to the correct working directory before starting the tests
(which is the same as on Linux), however the `ProcessStartInfo` process does
not inherit this working directory, which caused Windows tests to be running
with a different working directory as Linux (causing files used in tests to not
be found).

From the documentation; https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.workingdirectory?view=netframework-4.8

> When `UseShellExecute` is `true`, the fully qualified name of the directory that contains
> the process to be started. When the `UseShellExecute` property is `false`, the working
> directory for the process to be started. The default is an empty string (`""`).

This patch sets the `ProcessStartInfo.WorkingDirectory` to the correct working
directory before starting the process.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 6ae46ae)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This fixes a regression introduced in 6d87f19,
causing `COPY --from` to fail if the target directory does not exist:

```
FROM mcr.microsoft.com/windows/servercore:ltsc2019 as s1
RUN echo "Hello World" > /hello

FROM mcr.microsoft.com/windows/servercore:ltsc2019
COPY --from=s1 /hello /hello/another/world
```

Would produce an error:

```
Step 4/4 : COPY --from=s1 /hello /hello/another/world
failed to copy files: mkdir \\?: The filename, directory name, or volume label syntax is incorrect.
```

The cause for this was that Go's `os.MkdirAll()` does not support/detect volume GUID paths
(`\\?\Volume{dae8d3ac-b9a1-11e9-88eb-e8554b2ba1db}\hello\another}`), and as a result
attempted to create the volume as a directory (`\\?`), causing it to fail.

This patch replaces `os.MkdirAll()` with our own `system.MkdirAll()` function, which
is capable of detecting GUID volumes.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 5858a99)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the 19.03_backport_fix_copy_on_windows branch from fe070fa to ff0a0e3 Compare August 8, 2019 15:05
@thaJeztah
Copy link
Member Author

#313 was merged; rebased this PR (should go green now)

Copy link

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewhsu andrewhsu merged commit c7139be into docker-archive:19.03 Aug 8, 2019
Copy link

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah deleted the 19.03_backport_fix_copy_on_windows branch August 8, 2019 17:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants