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.

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

pkg/archive: Canonicalize stored paths #10865

Merged
merged 1 commit into from Feb 21, 2015

Conversation

Projects
None yet
8 participants
@ahmetb
Contributor

ahmetb commented Feb 18, 2015

Currently pkg/archive stores nested windows files with
backslashes (e.g. dir\, dir\file.txt) and this causes
tar not being correctly extracted on Linux daemon.

This change assures we canonicalize all paths to unix
paths and add them to tar with that name independent of platform.
For example windows paths like dir\foo.txt gets stored as dir/foo.txt
and dir\ entry gets stored as dir/ in the tar.

With this change we sustain unix-style paths in Dockerfiles
and .dockerignore files. The windows daemon probably needs to
modify the logic to untar unix paths correctly on windows filesystems.

Fixes the following test cases for Windows CI:

  • TestBuildAddFileWithWhitespace
  • TestBuildCopyFileWithWhitespace
  • TestBuildAddDirContentToRoot
  • TestBuildAddDirContentToExistingDir
  • TestBuildCopyDirContentToRoot
  • TestBuildCopyDirContentToExistDir
  • TestBuildDockerignore
  • TestBuildEnvUsage
  • TestBuildEnvUsage2
  • TestBuildCopyWildcard

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @unclejack @tiborvass @icecrime @jfrazelle @swernli @jhowardmsft @jeffmendoza

pkg/archive: Canonicalize stored paths
Currently pkg/archive stores nested windows files with
backslashes (e.g. `dir\`, `dir\file.txt`) and this causes
tar not being correctly extracted on Linux daemon.

This change assures we canonicalize all paths to unix
paths and add them to tar with that name independent of platform.

Fixes the following test cases for Windows CI:
- TestBuildAddFileWithWhitespace
- TestBuildCopyFileWithWhitespace
- TestBuildAddDirContentToRoot
- TestBuildAddDirContentToExistingDir
- TestBuildCopyDirContentToRoot
- TestBuildCopyDirContentToExistDir
- TestBuildDockerignore
- TestBuildEnvUsage
- TestBuildEnvUsage2

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 18, 2015

https://jenkins.dockerproject.com/job/Windows-PRs/57/console

=== RUN TestBuildAddFileWithWhitespace
[PASSED]: build - add file with whitespace
--- PASS: TestBuildAddFileWithWhitespace (4.17s)
=== RUN TestBuildCopyFileWithWhitespace
[PASSED]: build - copy file with whitespace
--- PASS: TestBuildCopyFileWithWhitespace (4.19s)
[...]
=== RUN TestBuildAddDirContentToRoot
[PASSED]: build - add directory contents to root
--- PASS: TestBuildAddDirContentToRoot (1.75s)
=== RUN TestBuildAddDirContentToExistingDir
[PASSED]: build - add directory contents to existing dir
--- PASS: TestBuildAddDirContentToExistingDir (2.87s)
[...]
=== RUN TestBuildCopyDirContentToRoot
[PASSED]: build - copy directory contents to root
--- PASS: TestBuildCopyDirContentToRoot (1.61s)
=== RUN TestBuildCopyDirContentToExistDir
[PASSED]: build - copy directory contents to existing dir
--- PASS: TestBuildCopyDirContentToExistDir (2.98s)
[...]
=== RUN TestBuildDockerignore
[PASSED]: build - test .dockerignore
--- PASS: TestBuildDockerignore (3.22s)
[...]
=== RUN TestBuildEnvUsage
[PASSED]: build - environment variables usage
--- PASS: TestBuildEnvUsage (3.04s)
=== RUN TestBuildEnvUsage2
[PASSED]: build - environment variables usage2
--- PASS: TestBuildEnvUsage2 (5.78s)

👍 happy CI is happy.

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 19, 2015

ping @unclejack for archive stuffs :)

@unclejack

This comment has been minimized.

Contributor

unclejack commented Feb 20, 2015

LGTM

// into forward slashes. since windows does not allow '/' or '\'
// in file names, it is mostly safe to replace however we must
// check just in case
if strings.Contains(p, "/") {

This comment has been minimized.

@LK4D4

LK4D4 Feb 20, 2015

Contributor

it is really impossible to have / in path even escaped?

This comment has been minimized.

@ahmetb

ahmetb Feb 20, 2015

Contributor

@LK4D4 you can't. http://stackoverflow.com/a/10708449/54929
but as a precautionary measure, I check for "/" and if it's there we just fail rather than blindly replacing it.

This comment has been minimized.

@LK4D4

LK4D4 Feb 20, 2015

Contributor

windows is really wonderland

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Feb 20, 2015

LGTM

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 20, 2015

Got 2 LGTMs, what now?

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Feb 21, 2015

Is this supposed to fail on windows right now?

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 21, 2015

@crosbymichael I'm not sure what you mean but if we build build context using windows CLI right now, nested paths are stored with backslashes, and therefore on linux daemon side, all those nested files are extracted as filenames with backslashes rather than going inside these directories. this fixes that.

edit: per our irc convo, yes, windows CI is expected to fail for everything, we're counting down from 100 failures to a green CI at this point.

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Feb 21, 2015

LGTM

crosbymichael added a commit that referenced this pull request Feb 21, 2015

Merge pull request #10865 from ahmetalpbalkan/win-cli/tar-path
pkg/archive: Canonicalize stored paths

@crosbymichael crosbymichael merged commit 1d382f9 into moby:master Feb 21, 2015

1 of 2 checks passed

windows Jenkins build Windows-PRs 57 has failed
Details
janky Jenkins build Docker-PRs 1268 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment