search

microsoft / scalar

Scalar: A set of tools and extensions for Git to allow very large monorepos to run on Git without a virtualization layer
MIT License
1.39k stars 63 forks source link

refactor POSIX platforms and remove partial classes and unused methods #448

Closed chrisd8088 closed 4 years ago

chrisd8088 commented 4 years ago

We refactor the common methods and classes in the Linux and Mac platform implementations so they share a single common POSIX implementation instead, as much as is reasonably possible.

One hurdle to this effort was how the various "data root" retrieval methods such as GetCommonAppDataRootForScalar() in each platform's main class, which were override methods of abstract ones in ScalarPlatform.cs, called into corresponding static *Implementation() methods in their respective {Linux,Mac}Platform.Shared.cs partial classes, which then returned values from static readonly variables over in the main class using a helper method defined in POSIXPlatform.cs, their immediate superclass. Trying to pull the common code into the POSIXPlatform class always resulted in one form or another of compile-time error, usually related to static methods lacking an instantiated object.

However, we can bypass this complexity by noticing that with the removal of the GVFS.Hooks project in commit f12effc371d7b1457ab58c6d44959632404e5998 in PR #23, there is no longer any need to split certain static methods into separate partial classes so they can be shared with that project.

So in addition to refactoring the POSIX platform classes, we also undo all the split partial classes (the *.Shared.cs files) throughout the codebase. This in turn makes it easier to find a few commonalities between the Windows and POSIX platforms and further refactor out those into ScalarPlatform in commit 9a3ea3786871ecbfb567f09b81aef44ffb5f2526 in this PR.

Finally, these simplifications permit the identification of several unused methods, specifically ScalarPlatform.IsProcessActive() and ScalarPlatform.IsConsoleOutputRedirectedToFile(), so we can remove these and along with some Windows native values and system call wrappers of which they were the exclusive users.

This PR should be reviewable as a single set of changes, but will likely be easier to understand in commit-by-commit fashion.

~One open question is whether GetUpgrade{LogDirectoryParent,HighestAvailableVersion}Directory() on POSIX should also just throw NotImplementedException() like GetUpgradeProtectedDataDirectory() does now (the Mac version was changed to do that in 9f635a9b94ce47db9bedc7241166ffe185b98883 in PR #425, which notes that we want to "redirect all upgrade logic away from the upgrader scaffolding".~

~Another question is whether we actually need any of what this PR moves into POSIXProductUpgraderPlatformStrategy.cs and which is currently implemented identically in the Mac and Linux versions.~

Details:

We refactor and create a common abstract POSIXFileBasedLock class shared by both the Linux and Mac versions, which now only differ in their key internal constants and type sizes.

We move the definition of the GetTemplateHooksDirectory() method and the NativeMethods.ResolveSymlink() method it uses into POSIXPlatform, eliminating duplication between the Mac and Linux versions. To do this we need to add a MaxPathLength value to the POSIXPlatform class and its subclasses, as the native PATH_MAX constant differs between Linux and macOS.

The Linux platform does not need to implement the GetUpgradeProtectedDataDirectory() method since the Mac platform already returns NotImplementedException() for this method. Thus we consolidate these methods into a common one in the POSIXPlatform.

The LinuxProductUpgraderPlatformStrategy class was a simple clone of the Mac version, so we consolidate these into a common POSIXProductUpgraderPlatformStrategy class. Moreover, since the POSIXPlatform sets its UnderConstructionFlags.UsesCustomUpgrader flag to false, none of the LinuxProductUpgraderPlatformStrategy class's methods are actually called when running the scalar upgrade command, and so we can simply change them all to throw a NotImplementedException().

The GVFS/GVFS.Common/Paths.cs file was split into two and Paths.Shared.cs was created in commit 2db0c030eb257beebf8e17f1c2ce72ffb166f533, in order to share methods such as Paths.GetGVFSEnlistmentRoot() with the GVFS.Hooks project. The original Paths.cs file was later renamed and moved to GVFS/GVFS.Platform.Windows/WindowsFileSystem.Shared.cs in commit 785ccb4a67281d0eb10f19cb53e6431b62b88555; that file is now Scalar.Common/Platforms/Windows/WindowsFileSystem.Shared.cs and its sole TryGetNormalizedPathImplementation() method is also implemented in the parallel POSIXFileSystem.Shared.cs file. Since the shared classes are no longer needed, we rename Paths.Shared.cs back to just Paths.cs, and move the substance of the TryGetNormalizedPathImplementation() methods into the corresponding main per-platform classes for Windows and POSIX. This also allows us to convert these classes back from being partial ones split over two files each.

We drop the GetNamedPipeNameImplementation() method from POSIXPlatform as the corresponding method in the MacPlatform.Shared.cs file was removed in commit 2a75485d30728283be2cd5d5d22303548ca19bf1 in PR #251 and so the POSIX version is also unused.

The ProductUpgraderInfo.IsLocalUpgradeAvailable() method's only caller was in the GVFS.Hooks project, so we can just drop that method and one private one as well from the now-consolidated ProductUpgraderInfo.cs file.

As the folder name "Scalar" is used several times in the WindowsPlatform, we refactor slightly to make that a common constant string in a new ScalarConstants.WindowsPlatform class, akin to what we do for the POSIX platforms.

As the Scalar.Service does not run the ProductUpgradeTimer on macOS, and there is no Scalar.Service on Linux, and because the UpgradeVerb does not record any highest-available version data except on Windows, there is no necessity to implement the GetUpgradeHighestAvailableVersionDirectory() method on POSIX. We can therefore just return a NotImplementedException(), and also simplify the GetUpgradeLogDirectoryParentDirectory() method, which is used on POSIX to provide the path to the log file generated by the scalar upgrade command.

The last callers of the IsConsoleOutputRedirectedToFile() methods in the ScalarPlatform classes were removed in commit 4183579d858e8fc633804834afc2be1f981e48b8 in PR #439. Therefore we remove this method, which (was never properly implemented on POSIX anyway per microsoft/VFSForGit#1355), along with some native Win32 system call wrappers and values used exclusively by the Windows implementation.

The last caller of the IsProcessActive() methods in the ScalarPlatform classes was removed in commit cedeeaa3ba68e86d4aa0ec3e5d453b8c033f7f6b in PR #29. Therefore we remove this method as well as some native Win32 system call wrappers and values used exclusively by the Windows implementation.

chrisd8088 commented 4 years ago

Thanks for taking a look, @mjcheetham! I'll fix up those comments (possibly in a force-push so I can also fix a commit's description where I see a typo also). One thing I wanted to call out from my wall of text in the PR description is what you and @derrickstolee think about these two possible further simplifications/changes:

One open question is whether GetUpgrade{LogDirectoryParent,HighestAvailableVersion}Directory() on POSIX should also just throw NotImplementedException() like GetUpgradeProtectedDataDirectory() does now (the Mac version was changed to do that in 9f635a9b94ce47db9bedc7241166ffe185b98883 in PR #425, which notes that we want to "redirect all upgrade logic away from the upgrader scaffolding".

Another question is whether we actually need any of what this PR moves into POSIXProductUpgraderPlatformStrategy.cs and which is currently implemented identically in the Mac and Linux versions.

derrickstolee commented 4 years ago

Thanks for taking a look, @mjcheetham! I'll fix up those comments (possibly in a force-push so I can also fix a commit's description where I see a typo also). One thing I wanted to call out from my wall of text in the PR description is what you and @derrickstolee think about these two possible further simplifications/changes:

A force-push is a great idea to keep the history clean.

One open question is whether GetUpgrade{LogDirectoryParent,HighestAvailableVersion}Directory() on POSIX should also just throw NotImplementedException() like GetUpgradeProtectedDataDirectory() does now (the Mac version was changed to do that in 9f635a9 in PR #425, which notes that we want to "redirect all upgrade logic away from the upgrader scaffolding".

Yes, we do want to remove the upgrader entirely soon. macOS already uses brew instead, and Linux should be updated to call apt-get when we have that system working.

Another question is whether we actually need any of what this PR moves into POSIXProductUpgraderPlatformStrategy.cs and which is currently implemented identically in the Mac and Linux versions.

So, we probably just want to delete this code and throw InvalidOperationException() if we try to create a strategy on a non-Windows platform.

A quick question, but it might increase your scope: can we just delete all of the upgrader code now? This would require changing scalar upgrade to say "scalar upgrade is not available on your platform" for non-macOS platforms right now, but it might make this change a bit simpler in some ways. Thoughts, @chrisd8088 and @mjcheetham?

chrisd8088 commented 4 years ago

Thanks for the feedback ... I think what I'll do is experiment a little to see what's required to get scalar upgrade to report a friendly non-implemented error on Linux/macOS, and otherwise tidy up any leftover implementation bits on those platforms, and then if it's not too large I can add it to this PR and otherwise break it off into a fresh one.

chrisd8088 commented 4 years ago

So the good news here is that scalar upgrade on Linux already reports ERROR: `scalar upgrade` is not supported on this operating system.

Also, by tracing the usage of the TryPrepare*Directory() methods in the {Mac,Linux}ProductUpgraderPlatformStrategy classes (which are consolidated into one POSIX class in 8d36c9d5eab0e6c929dd7f5f77ee081995732a70 in this PR), it seems clear that none of them are actually used because the places where they are called are all either in classes that are meaningful only on Windows like GitHubUpgrader and NuGetUpgrader, or in code paths invoked from methods which execute only on Windows like UpgradeVerb.TryRunInstaller(), or are behind checks on the UnderConstruction.UsesCustomUpgrader flag, which is false for the POSIXPlatform.

So I updated this PR to just make all these methods throw a NotImplementedException() on POSIX.

I also found that the POSIX version of GetUpgradeLogDirectoryParentDirectory() introduced in this PR is needed and can't just throw an exception, because the scalar upgrade process on all platforms begins by setting up a local log file into which it will write things like, e.g., errors from trying to run brew commands on macOS. So that method in POSIXPlatform is definitely still required.

Lastly, I was able to find that the third method of that type, GetUpgradeHighestAvailableVersionDirectory(), will also never be utilized on macOS (or Linux), so I could make that just return an exception and also tidy the remaining GetUpgradeLogDirectoryParentDirectory() method a bit.

chrisd8088 commented 4 years ago

Hey @derrickstolee and @mjcheetham, would you mind giving this another once-over?

I think I've analyzed the upgrade-related methods that can be turned into no-ops (see https://github.com/microsoft/scalar/pull/448#issuecomment-706508260 for more details) and found a few unused methods in the *.Shared.cs files which can be pruned out as well. The overall diff now removes 11 files and about 650 lines, and I think that's about as far as I can reasonably go with this PR.