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.

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
Merged

pkg/archive: Canonicalize stored paths #10865

merged 1 commit into from Feb 21, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb 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

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
Copy link
Contributor Author

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
Copy link
Contributor

ping @unclejack for archive stuffs :)

@unclejack
Copy link
Contributor

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, "/") {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

windows is really wonderland

@LK4D4
Copy link
Contributor

LK4D4 commented Feb 20, 2015

LGTM

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 20, 2015

Got 2 LGTMs, what now?

@crosbymichael
Copy link
Contributor

Is this supposed to fail on windows right now?

@ahmetb
Copy link
Contributor Author

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
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Feb 21, 2015
pkg/archive: Canonicalize stored paths
@crosbymichael crosbymichael merged commit 1d382f9 into moby:master Feb 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants