pkg/archive fixes, and port most unit tests to Windows#37982
Merged
yongtang merged 1 commit intomoby:masterfrom Nov 26, 2018
Merged
pkg/archive fixes, and port most unit tests to Windows#37982yongtang merged 1 commit intomoby:masterfrom
yongtang merged 1 commit intomoby:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #37982 +/- ##
=========================================
Coverage ? 36.1%
=========================================
Files ? 610
Lines ? 45250
Branches ? 0
=========================================
Hits ? 16339
Misses ? 26672
Partials ? 2239 |
thaJeztah
reviewed
Oct 8, 2018
kolyshkin
reviewed
Oct 8, 2018
kolyshkin
reviewed
Oct 8, 2018
kolyshkin
reviewed
Oct 8, 2018
kolyshkin
reviewed
Oct 8, 2018
kolyshkin
reviewed
Oct 8, 2018
kolyshkin
reviewed
Oct 8, 2018
kolyshkin
requested changes
Oct 8, 2018
Contributor
kolyshkin
left a comment
There was a problem hiding this comment.
Overall LGTM; a few minor nitpicks here and there need to be addressed though
lowenna
pushed a commit
to microsoft/docker
that referenced
this pull request
Oct 8, 2018
Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby#9874, and the fix at moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code.
Member
Author
|
@kolyshkin Pushed update. See #37982 (comment) about ordering, and #37982 (comment) though. Other comments addressed. |
Member
|
Ping @kolyshkin to see if it is ready to go. |
Contributor
|
@jhowardmsft can you please take another look at aba8b13#r223440313? You currently have 5 nested ifs, what I propose would bring it down to 3 without sacrificing anything and improving readability. |
kolyshkin
requested changes
Oct 22, 2018
Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby#9874, and the fix at moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code.
Member
Author
|
@kolyshkin (Finally, sorry!) fixed and addressed all your comments. |
Contributor
|
@jhowardmsft thanks! LGTM once CI is green |
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Nov 27, 2018
Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby/moby#9874, and the fix at moby/moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby/moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code. Upstream-commit: 56b732058e4d55aa5559996cce03fe11e6ff3baa Component: engine
adi-dhulipala
pushed a commit
to adi-dhulipala/docker
that referenced
this pull request
Apr 11, 2019
Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby#9874, and the fix at moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code.
dmcgowan
pushed a commit
to moby/go-archive
that referenced
this pull request
Apr 3, 2025
Signed-off-by: John Howard <jhoward@microsoft.com> If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to moby/moby#9874, and the fix at moby/moby#11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See moby/moby#37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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
If fixes an error in sameFsTime which was using
==to compare two times. The correct way is to use go's built-in timea.Equals(timeb).In changes_windows, it uses sameFsTime to compare mTim of a
system.StatTto allow TestChangesDirsMutated to operate correctly now.Note there is slight difference between the Linux and Windows implementations of detecting changes. Due to #9874, and the fix at #11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See #37982 for more information. The result in
TestChangesDirsMutated,dir3is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second.With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows.
It provides an implementation of
copyDirin tests for Windows. To make a copy similar to Linux'scp -awhile preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code.@thaJeztah @johnstep PTAL.