LCOW: lazycontext: Use correct lstat, fix archive check #37356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@johnstep PTAL |
|
@cpuguy83 This addresses your feedback on the original PR: #37316 (comment) |
tonistiigi
reviewed
Jun 27, 2018
pkg/archive/archive_windows.go
Outdated
| return "", fmt.Errorf("Windows path contains forward slash: %s", p) | ||
| } | ||
| return strings.Replace(p, string(os.PathSeparator), "/", -1), nil | ||
| func CanonicalTarNameForPath(p string) string { |
There was a problem hiding this comment.
That seems same as filepath.ToSlash()
There was a problem hiding this comment.
Ha, yes, indeed. Not sure how long it's been like that. Will update.
There was a problem hiding this comment.
I thought just removing this function completely but that can be a follow-up.
Codecov Report
@@ Coverage Diff @@
## master #37356 +/- ##
==========================================
+ Coverage 34.83% 34.85% +0.01%
==========================================
Files 610 610
Lines 44986 44981 -5
==========================================
+ Hits 15672 15677 +5
+ Misses 27201 27196 -5
+ Partials 2113 2108 -5 |
cpuguy83
requested changes
Jun 27, 2018
| }{ | ||
| {"foo", "foo", false}, | ||
| {"foo/bar", "___", true}, // unix-styled windows path must fail |
There was a problem hiding this comment.
@cpuguy83 Is is a desired behavior for these paths to fail? I would think not.
There was a problem hiding this comment.
Not fail but stay the same at least.
There was a problem hiding this comment.
Sure, added back in and updated to check they stay the same.
Signed-off-by: John Howard <jhoward@microsoft.com>
|
LGTM |
|
@thaJeztah Is this a known failure in experimental and Janky - both failed the same, but restarted: |
|
Janky https://jenkins.dockerproject.org/job/Docker-PRs/49887/console failing on a flaky test; tracked through #36501 Experimental https://jenkins.dockerproject.org/job/Docker-PRs-experimental/41137 also failing on a flaky test (tracked through #37246) |
|
None of those failures are related in any way, so I'll go ahead and merge |
thaJeztah
added
the
area/lcow
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: John Howard jhoward@microsoft.com
Fixes #36793. Replacement for #37316
First - thanks @yusuf-gunaydin for debugging and coming up with a fix. Yours was so very close :)
Here's the output using the same files as described in #36793 with the updated fix:
Note on the last build step:
bce0113cef4correctly.During debugging this (happened to be building the Microsoft/opengcs repo as that exercises LCOW code quite well in lieu of format CI yet), as mentioned in the issue, I noticed that as the
Lstatcall is now being made, and can succeed, there's an incorrect assertion in generating the hash (prepareHashfunction) when it calls into the archiver to Canonicalise the path, which causes build failures to occur on a step such asCOPY --from=runc /usr/bin/runc /usr/bin. Have removed the incorrect assertion, updated the function to no longer return anerror, and updated unit tests/call-sites not to handle the error there.