Upgrader - platform specific directory creation by jamill · Pull Request #1225 · microsoft/VFSForGit

jamill · 2019-05-30T19:37:38Z

The upgrade logic between the different platforms has some high level differences that need to be accounted for:

Creation of directories with the correct ACLs
- On Mac, the directories will be created with the expected ACLs, and we will not need to re-acl the directories.

There are other ways to handle the platform differences, but including:

performing the re-ACLing of directories on non-windows platforms. We could re-use more of the top level logic, but then we would be doing extra work with ACLs that is not needed.
Do no re-ACL on non-windows platforms. I did not want to cause confusion with the methods not doing what they advertise (especially around ACLs), in case someone misses this subtlety in the future. This would be hard from a maintainability standpoint...

Whether the Upgrader is responsible for remounting or not
- On Windows, the upgrader itself is responsible for re-mounting after upgrade, while on Mac, the GVFS.Service will auto remount when the service starts up.

There is still some further work I need to do here to get this into shape, but I wanted to put this up for initial feedback on the general direction.

derrickstolee

Looks promising to start. I'd unify the cross-platform pattern to not have Windows be the default, but instead be at the same level as Mac. This means having an abstract base class with an implementation for each platform.

It would be nice to do some research into the Linux platform a bit here so we can figure out what can be combined into a POSIX platform class and extend that with Mac and Linux stuff. For example: creating directories with given permissions will be the same in each.

Windows -> Base
POSIX -> Base
Mac -> POSIX
Linux -> POSIX

GVFS/GVFS.Common/ProductUpgraderPlatformStrategy.cs

GVFS/GVFS.Common/ProductUpgraderPlatformStrategyFactory.cs

GVFS/GVFS.Common/WindowsProductUpgraderPlatformStrategy.cs

GVFS/GVFS.Upgrader/MacUpgradeOrchestrator.cs

GVFS/GVFS.Upgrader/Program.cs

GVFS/GVFS.Upgrader/UpgradeOrchestrator.cs

GVFS/GVFS.Upgrader/UpgradeOrchestratorFactory.cs

kewillford · 2019-05-30T21:08:50Z

GVFS/GVFS.Common/MacProductUpgraderPlatformStrategy.cs

+
+namespace GVFS.Common
+{
+    public class MacProductUpgraderPlatformStrategy : ProductUpgraderPlatformStrategy


Still looking but I'm not sure what PlatformStrategy means.

I also don't think we should have platform specific code going into GVFS.Common. Looking at this and it seems these should be a part of IPlatformFileSystem with a TryPrepareDirectory(string path, out string error). We might even be able to remove the 2 permission method from that interface since they appear only to be for Windows.

bool TryCreateDirectoryWithAdminAndUserModifyPermissions(string directoryPath, out string error); bool TryCreateOrUpdateDirectoryToAdminModifyPermissions(ITracer tracer, string directoryPath, out string error);

it seems these should be a part of IPlatformFileSystem with a TryPrepareDirectory(string path, out string error)

I like this idea - but the problem I ran into is that that action depends on the directory (which the upgrader has the most context on).

If there is an appropriate abstraction here, I am open to adding these directly to the IPlatformFileSystem class, but I haven't figured out a non-upgrader specific abstraction here... I will continue to think about this.

kewillford · 2019-05-30T21:13:28Z

GVFS/GVFS.Upgrader/GVFS.Upgrader.csproj

    <When Condition="'$(OS)' == 'Windows_NT'">
      <PropertyGroup>
        <TargetFrameworks>net461;netcoreapp2.1</TargetFrameworks>
+	<DefineConstants>$(DefineConstants);WINDOWS_BUILD</DefineConstants>


Looks like this project is already referencing the Windows or the Mac Platform projects so why can't we have the platform specific code live in those projects instead of adding another way to create platform specific code?

That is an option - but then we are putting upgrader specific code in the platform project...

I will look at moving this into platform specific projects

After thinking about this a bit more - I moved the platform specific code out of the Common project, but I am still am not sure about the GVFS.Upgrader project. This project is almost a separate application, and I didn't want to bring high level "other application" specific logic into the Platform classes.

I am open to thoughts on this, but I wasn't going to convert this just yet.

jamill · 2019-05-31T03:20:08Z

Thanks @derrickstolee for the review!

This means having an abstract base class with an implementation for each platform.

I think this makes sense as a goal. The problem I ran into is that we have a bunch of existing tests that exercise the current concrete implementation. If we make the UpgradeOrchestrator class abstract, then these tests will need reaction work... Either a "Testable" UpgradeOrchestrator class to test the common logic (which is most of the class), moving the tests to follow the new concrete implementations, etc... I like the suggestion - so let me investigate it a bit further.

It would be nice to do some research into the Linux platform a bit here so we can figure out what can be combined into a POSIX platform class

Good idea! I think a lot (or at least some) of the logic that is in the Mac specific class can actually go in the POSIX layer instead (as you noted).

jamill · 2019-05-31T13:19:43Z

GVFS/GVFS.Common/GVFSPlatform.cs

            ITracer tracer,
            string lockPath);

+        public abstract ProductUpgraderPlatformStrategy CreateProductUpgraderPlatformInteractions(


I think the ProductUpgraderPlatformStrategy name is overly verbose and at the same time not clear. I don't think the Interactions suffix is any better... Still thinking for a better name.

derrickstolee

Just a few lingering questions.

GVFS/GVFS.Upgrader/WindowsUpgradeOrchestrator.cs

derrickstolee · 2019-06-06T17:18:47Z

GVFS/GVFS.Upgrader/UpgradeOptions.cs

+
+namespace GVFS.Upgrader
+{
+    [Verb("UpgradeOrchestrator", HelpText = "Upgrade VFS for Git.")]


This Verb annotation seems out of place.

This usage is different than how we generally structure this in our product, but it does match the intended usage:

https://github.com/commandlineparser/commandline#for-verbs

I think we could ask whether we need this to be a verb, as this is being used to process command line arguments for a program that doesn't actually have verbs. I will note this as a potential cleanup.

alameenshah

Testing notes - there seems to be an issue on Windows.

> gvfs upgrade
Checking for GVFS upgrades...Failed

ERROR: Could not load file or assembly 'System.Runtime.InteropServices.RuntimeInformation, Version=4.0.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.

GVFS/GVFS.Common/ProductUpgrader.cs

GVFS/GVFS.Common/ProductUpgraderPlatformStrategy.cs

GVFS/GVFS.Platform.Mac/MacProductUpgraderPlatformStrategy.cs

Instead of querying interop services at runtime (with a dll that is not included with the windows install), determine this at compile time using the under construction flags.

alameenshah

Thank you for fixing the issue.

Looking Good now.

jamill added the WIP label May 30, 2019

jamill requested review from alameenshah, kewillford, mjcheetham and wilbaker May 30, 2019 19:37

derrickstolee requested changes May 30, 2019

View reviewed changes

kewillford reviewed May 30, 2019

View reviewed changes

jamill force-pushed the upgrader_tasks/platform_strategy branch from 983ccc9 to 92118b0 Compare May 31, 2019 02:57

jamill force-pushed the upgrader_tasks/platform_strategy branch from 92118b0 to 0e68664 Compare May 31, 2019 03:22

jamill commented May 31, 2019

View reviewed changes

jamill mentioned this pull request May 31, 2019

Upgrader for macOS MVP #1129

Closed

15 tasks

jrbriggs added domain: upgrade platform: macOS platform: windows labels Jun 1, 2019

jamill force-pushed the upgrader_tasks/platform_strategy branch from 4ec26d1 to 19a2034 Compare June 4, 2019 17:56

jamill changed the title ~~WIP: Upgrader tasks/platform strategy~~ Upgrader tasks/platform strategy Jun 4, 2019

jamill removed the WIP label Jun 4, 2019

jamill changed the title ~~Upgrader tasks/platform strategy~~ Upgrader - platform specific directory creation Jun 4, 2019

jamill force-pushed the upgrader_tasks/platform_strategy branch 2 times, most recently from 130a5ea to 81d9f15 Compare June 6, 2019 03:03

derrickstolee approved these changes Jun 6, 2019

View reviewed changes

jamill force-pushed the upgrader_tasks/platform_strategy branch from 81d9f15 to 6b45433 Compare June 7, 2019 15:04

alameenshah suggested changes Jun 7, 2019

View reviewed changes

GVFS/GVFS.Common/ProductUpgrader.cs Show resolved Hide resolved

GVFS/GVFS.Common/ProductUpgraderPlatformStrategy.cs Show resolved Hide resolved

GVFS/GVFS.Platform.Mac/MacProductUpgraderPlatformStrategy.cs Show resolved Hide resolved

jamill force-pushed the upgrader_tasks/platform_strategy branch from 6b45433 to b32e997 Compare June 7, 2019 17:55

jamill added 4 commits June 7, 2019 14:10

NuGetUpgrader: fix issue with upgrader using interop services

fe04e7d

Instead of querying interop services at runtime (with a dll that is not included with the windows install), determine this at compile time using the under construction flags.

Upgrade: introduce platform strategy

6ca0311

UpgradeOrchestrator: handle platform differences

4a91904

UpgradeVerb: enable platform specific logic

bc21b84

jamill force-pushed the upgrader_tasks/platform_strategy branch from b32e997 to bc21b84 Compare June 7, 2019 18:10

alameenshah approved these changes Jun 7, 2019

View reviewed changes

jamill merged commit b367284 into microsoft:master Jun 7, 2019

jamill deleted the upgrader_tasks/platform_strategy branch June 7, 2019 23:24

jrbriggs added this to the M155 milestone Jun 21, 2019

Conversation

jamill commented May 30, 2019

Uh oh!

derrickstolee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented May 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrickstolee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alameenshah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alameenshah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants