[Mac, Linux] refactor Mac platform into POSIX base by kivikakk · Pull Request #926 · microsoft/VFSForGit

kivikakk · 2019-03-14T05:11:32Z

Here's a refactor of the Mac platform into a POSIX base. (On my end, I did the full refactor of Mac and Linux platforms, then rebased away the Linux changes.) This PR does so without introducing the Mono dependency; just moving code around.

/cc @jrbiggs, #810
/cc @chrisd8088 fyi!

GVFS/GVFS.Platform.POSIX/POSIXFileBasedLock.cs

jrbriggs

Just one small thing.

@wilbaker can you take a look too?

GVFS/GVFS.Hooks/HooksPlatform/GVFSHooksPlatform.Mac.cs

GVFS/GVFS.Platform.POSIX/POSIXFileSystem.Shared.cs

GVFS/GVFS.Platform.Mac/MacPlatform.cs

GVFS/GVFS.Platform.POSIX/NetCoreMethods.cs

GVFS/GVFS.Platform.POSIX/POSIXDiskLayoutUpgradeData.cs

GVFS/GVFS.Hooks/HooksPlatform/GVFSHooksPlatform.Mac.cs

chrisd8088 · 2019-03-21T22:40:30Z

GVFS/GVFS.Platform.POSIX/POSIXFileSystem.cs

-            public const int ReadOnly = 0x0000;
-            public const int WriteOnly = 0x0001;
-
-            public const int Create = 0x0200;


We have the same problem here as in the file-lock code; this is 0x0040 on Linux, and the various chmod-related calls should use an uint. There may be other Linux vs. BSD changes here as well, so this probably needs to be refactored into a POSIX base with Linux and Mac extensions.

There may be other Linux vs. BSD changes here as well, so this probably needs to be refactored into a POSIX base with Linux and Mac extensions.

@kivikakk, @chrisd8088, are you planning to make this change in this PR, or in a follow up PR?

@kivikakk is away for a bit and as this is their work, I'm not looking to make any major changes to it right now. My own feeling at the moment is summarized by in my review comment:

To that end, the #810 PR (still a WIP) has a number of these modifications in it now, so using that as a basis for refactoring (maybe refactoring after that code has landed?) might be useful.

But I'll defer to @kivikakk to decide if they want to try to get the POSIX changes in first. For my part, I'm focused on looking at blockers to running gvfs mount on Linux, which is somewhat orthogonal work to this refactoring.

@chrisd8088 ok thanks!

Regarding the status of this PR, I think it's close being ready to merge, there are just a few open comments left from the first round of review.

/cc: @jrbriggs @kivikakk

I'd like to perform these changes later when we add the Linux platform in -- at that point we should refactor so that e.g. there's only one definition of any given native method for a given platform, whereas right now we have open and O_CREAT defined twice (in FileSystem and FileBasedLock).

It's pretty hard to get a refactor right when there's only one consumer of the code, and right now that's the case (Mac platform code is the single consumer of the POSIX platform base code). We'll maybe find more cases like this where Linux varies vs. what we've pre-emptively abstracted, but we should handle them as we implement the Linux platform in the tree, and not try to get this PR 100% correct before (imo). More important is that we get the initial refactor down asap so we don't get increasingly large deltas as time presses on.

Sounds like a reasonable plan!

chrisd8088 · 2019-03-22T06:55:25Z

GVFS/GVFS.Hooks/HooksPlatform/GVFSHooksPlatform.Mac.cs

@@ -1,42 +1,42 @@
-using GVFS.Platform.Mac;
+using GVFS.Platform.POSIX;


One additional thought here is that we should presumably also rename this file to GVFSHooksPlatform.POSIX.cs and adjust the various *.csproj files to use that instead.

There'll likely need to be one each for Mac and Linux since GetInstallerExtension will differ for both. It's a bit awkward; any thoughts? Could dump everything except GetInstallerExtension into GVFSHooksPlatform.POSIX, make the class partial, and then have a .Mac and .Linux variant with only GetInstallerExtension?

Could dump everything except GetInstallerExtension into GVFSHooksPlatform.POSIX, make the class partial, and then have a .Mac and .Linux variant with only GetInstallerExtension?

That sounds good to me. I am okay with leaving the current code as-is and making that change in a follow up PR if you prefer.

@kivikakk I just realized nothing is calling GetInstallerExtension anymore. Could you just remove it from this class (and the .Windows.cs version of this class)?

wilbaker · 2019-03-22T21:09:43Z

/azp run GitHub VFSForGit Mac Functional Tests

kewillford · 2019-03-22T21:11:16Z

/azp run GitHub VFSForGit Mac Functional Tests

kewillford · 2019-03-22T21:53:18Z

/azp run GitHub VFSForGit Mac Functional Tests

wilbaker · 2019-03-22T22:03:09Z

@kivikakk @chrisd8088, @kewillford and I have been unable to kick off the Mac functional tests and we suspect it may be due to the merge conflict in GVFS/GVFS.Platform.Mac/MacPlatform.Shared.cs.

Could you resolve this conflict and we'll try kicking off the tests again?

GVFS/GVFS.Platform.Mac/MacPlatform.cs

For ease of comparsion with the original Mac-only source files, we retain the line endings on the refactored POSIX common files to exactly match the original ones.

chrisd8088 · 2019-03-23T05:33:35Z

OK @wilbaker, I've rebased on the latest master and resolved the conflicts and changes which came with that, and all the CI tests pass except the one which seems "stuck".

kewillford · 2019-03-25T14:56:27Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-03-25T14:56:59Z

No pipelines are associated with this pull request.

@chrisd8088

Some constants differ per @chrisd8088's comment here: microsoft#926 (comment) When we add the Linux project, we should factor *out* of POSIXFileBasedLock what's Mac-specific.

kivikakk · 2019-04-02T09:02:26Z

All test passing. Let me know if this looks okay; cc @wilbaker @jrbriggs.

jrbriggs · 2019-04-02T12:48:46Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-04-02T12:48:53Z

No pipelines are associated with this pull request.

kivikakk · 2019-04-02T22:07:02Z

Looks like Mac Functional Tests timed out after running for more than 3 hours (but passed as far as it got?).

jrbriggs

Approved with suggestions.

GVFS/GVFS.Platform.Mac/MacFileBasedLock.cs

jrbriggs · 2019-04-03T15:13:34Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-04-03T15:13:40Z

No pipelines are associated with this pull request.

wilbaker

Approved with suggestions

GVFS/GVFS.Platform.Mac/MacFileBasedLock.cs

GVFS/GVFS.Platform.Mac/MacPlatform.cs

kivikakk · 2019-04-07T23:44:02Z

Suggestions taken up 👍

kivikakk · 2019-04-08T00:54:00Z

Curious if the following will work for me:

kivikakk · 2019-04-08T00:54:12Z

/azp run GitHub VFSForGit Mac Functional Tests

kewillford · 2019-04-08T15:24:20Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-04-08T15:24:25Z

No pipelines are associated with this pull request.

wilbaker

Just one more minor file naming comment on the latest changes.

GVFS/GVFS.Hooks/HooksPlatform/GVFSHooksPlatform.Mac.cs

jrbriggs · 2019-04-09T13:05:28Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-04-09T13:05:33Z

No pipelines are associated with this pull request.

kivikakk · 2019-04-10T05:02:46Z

Thanks for your patience with this! ❤️

kivikakk mentioned this pull request Mar 14, 2019

[WIP] initial build of GVFS provider for Linux #810

Closed

chrisd8088 suggested changes Mar 15, 2019

View reviewed changes

GVFS/GVFS.Platform.POSIX/POSIXFileBasedLock.cs Outdated Show resolved Hide resolved

jrbriggs suggested changes Mar 15, 2019

View reviewed changes

GVFS/GVFS.Hooks/HooksPlatform/GVFSHooksPlatform.Mac.cs Outdated Show resolved Hide resolved

wilbaker suggested changes Mar 15, 2019

View reviewed changes

kewillford reviewed Mar 15, 2019

View reviewed changes

GVFS/GVFS.Hooks/HooksPlatform/GVFSHooksPlatform.Mac.cs Show resolved Hide resolved

chrisd8088 reviewed Mar 21, 2019

View reviewed changes

chrisd8088 reviewed Mar 22, 2019

View reviewed changes

chrisd8088 suggested changes Mar 22, 2019

View reviewed changes

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

chrisd8088 force-pushed the refactor-mac-posix branch 3 times, most recently from 0450a84 to c02a192 Compare March 23, 2019 04:05

refactor Mac platform into POSIX base

262772e

For ease of comparsion with the original Mac-only source files, we retain the line endings on the refactored POSIX common files to exactly match the original ones.

chrisd8088 force-pushed the refactor-mac-posix branch from c02a192 to 262772e Compare March 23, 2019 04:33

chrisd8088 mentioned this pull request Mar 30, 2019

[Linux] Build GVFS provider on Linux #986

Closed

kivikakk added 4 commits April 2, 2019 14:40

reintroduce MacPlatform.InstallerExtension

0d1ba4c

using GVFS.Platform.POSIX

b96d740

revert to MacDiskLayoutUpgradeData

e67c4ef

ensure POSIXFileBasedLock isn't reused blindly

8978fa8

Some constants differ per @chrisd8088's comment here: microsoft#926 (comment) When we add the Linux project, we should factor *out* of POSIXFileBasedLock what's Mac-specific.

jrbriggs approved these changes Apr 3, 2019

View reviewed changes

GVFS/GVFS.Platform.Mac/MacFileBasedLock.cs Outdated Show resolved Hide resolved

GVFS/GVFS.Platform.Mac/MacFileBasedLock.cs Outdated Show resolved Hide resolved

remove POSIXFileBasedLock for now

b9abfec

wilbaker added platform: linux platform: macOS pr: refactoring Keeping the codebase healthy labels Apr 3, 2019

wilbaker approved these changes Apr 3, 2019

View reviewed changes

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

GVFS/GVFS.Platform.Mac/MacPlatform.cs Outdated Show resolved Hide resolved

kivikakk added 4 commits April 8, 2019 09:31

revert whitespace changes

a1a101c

remove GetInstallerExtension

e602b59

remove Mac InstallerExtension

0c712ce

remove Windows InstallerExtension

bd94666

wilbaker reviewed Apr 8, 2019

View reviewed changes

GVFS/GVFS.Hooks/HooksPlatform/GVFSHooksPlatform.Mac.cs Show resolved Hide resolved

rename

d2919f5

jrbriggs merged commit c9e404d into microsoft:master Apr 9, 2019

chrisd8088 deleted the refactor-mac-posix branch April 9, 2019 21:58

		@@ -1,42 +1,42 @@
		using GVFS.Platform.Mac;
		using GVFS.Platform.POSIX;

Conversation

kivikakk commented Mar 14, 2019

Uh oh!

Uh oh!

jrbriggs left a comment

Choose a reason for hiding this comment

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!

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!

wilbaker commented Mar 22, 2019

Uh oh!

kewillford commented Mar 22, 2019

Uh oh!

kewillford commented Mar 22, 2019

Uh oh!

wilbaker commented Mar 22, 2019

Uh oh!

Uh oh!

chrisd8088 commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kewillford commented Mar 25, 2019

Uh oh!

azure-pipelines bot commented Mar 25, 2019

Uh oh!

kivikakk commented Apr 2, 2019

Uh oh!

jrbriggs commented Apr 2, 2019

Uh oh!

azure-pipelines bot commented Apr 2, 2019

Uh oh!

kivikakk commented Apr 2, 2019

Uh oh!

jrbriggs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jrbriggs commented Apr 3, 2019

Uh oh!

azure-pipelines bot commented Apr 3, 2019

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kivikakk commented Apr 7, 2019

Uh oh!

kivikakk commented Apr 8, 2019

Uh oh!

kivikakk commented Apr 8, 2019

Uh oh!

kewillford commented Apr 8, 2019

Uh oh!

azure-pipelines bot commented Apr 8, 2019

chrisd8088 commented Mar 23, 2019 •

edited

Loading