Builder: fix "COPY --from" to non-existing directory on Windows#39695
Builder: fix "COPY --from" to non-existing directory on Windows#39695kolyshkin merged 2 commits intomoby:masterfrom
Conversation
|
ping @ddebroy @StefanScherer PTAL |
|
ah, dang; guess I need to flip a slash |
53b3e9c to
95a63be
Compare
|
Fixed (hopefully) |
|
still looks good on Windows |
|
Still doesn't work on Windows 😞 |
95a63be to
e7ae1b7
Compare
|
Getting close |
integration/build/build_test.go
Outdated
There was a problem hiding this comment.
Maybe inline it as in other tests?
There was a problem hiding this comment.
That didn't work; looks like on Windows the CWD is different, and it sees C:\gopath\src\github.com\docker\docker\ as current directory, not C:\gopath\src\github.com\docker\docker\integration\build\.
I can work around that, but looking why
CI is running the integration tests from here;
Lines 338 to 354 in 4fb5e9e
Which looks quite similar to the Linux equivalent;
moby/hack/make/.integration-test-helpers
Lines 44 to 48 in 4fb5e9e
I wonder if System.Diagnostics.ProcessStartInfo inherits the working directory, or if it needs to be set explicitly 🤔
There was a problem hiding this comment.
Looks like that may be it: it has a ProcessStartInfo.WorkingDirectory property
From the documentation; https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.workingdirectory?view=netframework-4.8
When
UseShellExecuteistrue, the fully qualified name of the directory that contains
the process to be started. When theUseShellExecuteproperty isfalse, the working
directory for the process to be started. The default is an empty string ("").
There was a problem hiding this comment.
Opened #39698 to fix that
I'll rebase this PR on top of that one to get the fix in this branch
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>
e7ae1b7 to
b50f192
Compare
b50f192 to
3ffe794
Compare
|
hmf test is failing |
|
|
3298c55 to
f38b340
Compare
|
Whoop! Fixed the test; now works both on Windows and Linux
|
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>
501a718 to
5858a99
Compare
ddebroy
left a comment
There was a problem hiding this comment.
LGTM with one clarification.
what's the clarification? did yo forget to post that? |
|
Looks like Docker Hub made a whoopsie |
|
#39698 is merged, can you please rebase? |
|
oops about the note above. I meant to say for Windows |
fixes docker/for-win#4349
built on top of #39698
This fixes a regression introduced in 6d87f19 (#38599),
causing
COPY --fromto fail if the target directory does not exist:Would produce an error:
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 resultattempted to create the volume as a directory (
\\?), causing it to fail.This patch replaces
os.MkdirAll()with our ownsystem.MkdirAll()function, whichis capable of detecting GUID volumes.
- How to verify it
To verify (on Linux; don't know the equivalent on Windows, LOL):
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)