Upgrader: copy entire application directory#1228
Conversation
| directory.Delete(); | ||
| } | ||
|
|
||
| public virtual void CopyDirectory(string source, string destination) |
There was a problem hiding this comment.
naming nit: CopyDirectoryRecursive ?
jrbriggs
added
affects: engineering
jamill
force-pushed
the
upgrader_tasks/copy_directory
branch
from
7ed3548 to
28a6da1
Compare
Teach PhysicalFileSystem the ability to copy an entire directory to a destination path.
When upgrading VFS for Git via the built-in upgrade logic, the upgrade verb will copy the upgrader application to a temporary directory, and run the upgrade app from the temporary directory. The main reason for this is so that the upgrade application can update the original application without open file handles getting in the way (and preventing the deletion of the application that is being updated). The upgrade verb currently has a hard coded list of dependencies (files) required for the upgrader application to run. This can be fragile as we need to keep this list in sync with the actual dependencies, across multiple platforms. This can easily break if the dependencies change, and we update the list of files included with the installer, but not with the upgrader application. A specific occurrence of this happened in microsoft#1144. Instead, just copy over the whole VFS for Git application directory when creating a temporary copy of the upgrader application to run. We might copy over a few additional files, but this should not cause any noticeable downsides.
jamill
force-pushed
the
upgrader_tasks/copy_directory
branch
from
28a6da1 to
55bb915
Compare
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
I see your "Tests" TODO. I'm assuming you just want a unit test on the recursive copy, right?
Is there any reason the CopyDirectoryRecursive() method shouldn't be an extension method on a FileSystem object? That would allow calling it on a mock object during the test.
I'm assuming our functional tests already cover what we can in the upgrade scenario.
I have been thinking about how to test this. My initial idea was to do a unit test to verify that a source directory (with some structure) was copied to a destination directory as expected, but then I would be testing the
That might help with how to test this. I don't think there is any reason why it couldn't be an extension method. We don't have any other extension methods for this class (yet), so this would be the first. It seems similar in function to existing methods such as
I think we are a bit thin here - we have discussed improving this, and I added a GitHub issue to track this work (in case we don't have an existing issue). |
|
I am going to proceed with this change as is. I included the test topic as a deferred / follow up item in the top level issue tracking the macOS upgrader (#1129) |
4 participants
When upgrading VFS for Git via the built-in upgrade logic, the upgrade verb will copy the upgrader application to a temporary directory, and run the upgrade app from the temporary directory. The main reason for this is so that the upgrade application can update the original application without open file handles getting in the way (and preventing the deletion of the application that is being updated).
The upgrade verb currently has a hard coded list of dependencies (files) required for the upgrader application to run. This can be fragile as we need to keep this list in sync with the actual dependencies, across multiple platforms. This can easily break if the dependencies change, and we update the list of files included with the installer, but not with the upgrader application. A specific occurrence of this happened in #1144.
Instead, just copy over the whole VFS for Git application directory when creating a temporary copy of the upgrader application to run. We might copy over a few additional files, but this should not cause any noticeable downsides.
TODO:
[ ] Testscopied to backlog in Upgrader for macOS MVP #1129