refactor POSIX platforms and remove partial classes and unused methods #448
Conversation
|
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:
|
9f6d2ef to
e850dbb
Compare
A force-push is a great idea to keep the history clean.
Yes, we do want to remove the upgrader entirely soon. macOS already uses
So, we probably just want to delete this code and A quick question, but it might increase your scope: can we just delete all of the upgrader code now? This would require changing |
We can refactor and create a common abstract POSIXFileBasedLock class which can then by shared by both the Linux and Mac versions, which now only differ in their key internal constants and type sizes. From microsoft/VFSForGit@8978fa8 and microsoft/VFSForGit@7b70850. Co-authored-by: Ashe Connor <ashe@kivikakk.ee>
e850dbb to
a8f8bca
Compare
|
Thanks for the feedback ... I think what I'll do is experiment a little to see what's required to get |
a8f8bca to
d9feb3c
Compare
|
So the good news here is that Also, by tracing the usage of the So I updated this PR to just make all these methods throw a I also found that the POSIX version of Lastly, I was able to find that the third method of that type, |
We can 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 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 can consolidate these methods into a common one in the POSIXPlatform and remove one to-do item for Linux at the same time.
The LinuxProductUpgraderPlatformStrategy class was a simple clone of the Mac version, so we can consolidate these into a common POSIXProductUpgraderPlatformStrategy class. Moreover, since the POSIXPlatform sets its UnderConstructionFlags.UsesCustomUpgrader flag to false, none of this class's methods are actually called when running the "scalar upgrade" command, 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 2db0c03, 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 785ccb4. 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. Given that there is no longer a corresponding Scalar version of the GVFS.Hooks project, there is no need to split out certain (static) methods from the others in any of these classes. (The GVFS.Hooks project was removed in commit f12effc in PR microsoft#23.) Thus 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.
The corresponding method in the MacPlatform.Shared.cs file was removed in commit 2a75485 in PR microsoft#251, so we can also drop GetNamedPipeNameImplementation() from the POSIX platform as well.
The POSIXPlatform.cs and POSIXPlatform.Shared.cs classes exist as a pair of partial classes in order to share the static methods in the latter with the GVFS.Hooks project; however, that project was removed in commit f12effc in PR microsoft#23, so we can merge the implementation of several methods back into the main POSIXPlatform class and remove the shared one.
The {Linux,Mac}Platform.cs and {Linux,Mac}Platform.Shared.cs
classes exist as pairs of partial classes in order to share
static methods in the Shared classes with the GVFS.Hooks project;
however, that project was removed in commit
f12effc in PR microsoft#23, so we can
merge the implementation of several methods back into the common
POSIXPlatform class and remove the shared ones.
We also align all the per-platform EnvironmentVariables arrays
to be private.
The WindowsPlatform.cs and WindowsPlatform.Shared.cs classes exist as a pair of partial classes in order to share the static methods in the latter with the GVFS.Hooks project; however, that project was removed in commit f12effc in PR microsoft#23, so we can merge the implementation of several methods back into the main WindowsPlatform class and remove the shared one. We also slightly reorder the GetUpgradeProtectedDataDirectory() method so it aligns with those in ScalarPlatform and POSIXPlatform, which may help with future refactoring or simplification.
As the folder name "Scalar" is used several times in the WindowsPlatform, we can refactor slightly to make that a common constant string in a new ScalarConstants.WindowsPlatform class, akin to what we do for the POSIX platforms.
Having consolidated the various per-platform variants of ScalarPlatform in previous commits, we can further simplify by pulling the common implementations of certain methods into ScalarPlatform itself, specifically those which just append a path component to the application data root path and secure data root path.
The GVFS/GVFS.Common/GVFSEnlistment.cs file was split into two and GVFS/GVFS.Common/GVFSEnlistment.Shared.cs was created in commit ed04790, in order to share the IsUnattended() method with the GVFS.Hooks project. Given that there is no longer a corresponding Scalar version of the GVFS.Hooks project, there is no need to split out certain (static) methods from the others in any classes. (The GVFS.Hooks project was removed in commit f12effc in PR microsoft#23.) Thus we merge the IsUnattended() method into ScalarEnlistment.cs and remove the ScalarEnlistment.Shared.cs file.
The GVFS/GVFS.Common/ProductUpgraderInfo.cs file was split into two and GVFS/GVFS.Common/ProductUpgraderInfo.Shared.cs was created in commit d5d9e7d, in order to share the IsLocalUpgradeAvailable() method and some constant path strings with the GVFS.Hooks project. Given that there is no longer a corresponding Scalar version of the GVFS.Hooks project, there is no need to split out certain (static) methods from the others in any classes. (The GVFS.Hooks project was removed in commit f12effc in PR microsoft#23.) Further, the IsLocalUpgradeAvailable() method's only caller was in the GVFS.Hooks project, so we can just drop that method and one private one as well. We do merge another private method and the path constants into ProductUpgraderInfo.cs and then remove the ProductUpgraderInfo.Shared.cs file.
The GVFS/GVFS.Common/NativeMethods.cs file was split into two and GVFS/GVFS.Common/NativeMethods.Shared.cs was created in commit 785ccb4, in order to share the ThrowLastWin32Exception() and GetFinalPathName() methods and some native Win32 enums and system call wrappers with the GVFS.Hooks project. Given that there is no longer a corresponding Scalar version of the GVFS.Hooks project, there is no need to split out certain (static) methods from the others in any classes. (The GVFS.Hooks project was removed in commit f12effc in PR microsoft#23.) Thus we merge the two methods, file attribute and access enums, and system call wrappers into NativeMethods.cs and remove the NativeMethods.Shared.cs file.
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.
cfb413a to
5abf595
Compare
The last callers of the IsConsoleOutputRedirectedToFile() methods in the ScalarPlatform classes were removed in commit 4183579 in PR microsoft#439. Therefore we can remove this method (which was never properly implemented on POSIX anyway per microsoft/VFSForGit#1355) as well as the native Windows system call wrappers and enums which were used only by the WindowPlatform implementation of this method.
b137396 to
2b4e3f3
Compare
The last caller of the IsProcessActive() methods in the ScalarPlatform classes was removed in commit cedeeaa in PR microsoft#29. Therefore we can remove this method as well as the native Windows system call wrappers and enums which were used only by the WindowPlatform implementation of this method.
2b4e3f3 to
726e277
Compare
|
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 #448 (comment) for more details) and found a few unused methods in the |
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 wereoverridemethods ofabstractones inScalarPlatform.cs, called into correspondingstatic*Implementation()methods in their respective{Linux,Mac}Platform.Shared.cspartialclasses, which then returned values fromstatic readonlyvariables over in the main class using a helper method defined inPOSIXPlatform.cs, their immediate superclass. Trying to pull the common code into thePOSIXPlatformclass always resulted in one form or another of compile-time error, usually related tostaticmethods lacking an instantiated object.However, we can bypass this complexity by noticing that with the removal of the
GVFS.Hooksproject in commit f12effc in PR #23, there is no longer any need to split certainstaticmethods into separatepartialclasses so they can be shared with that project.So in addition to refactoring the POSIX platform classes, we also undo all the split
partialclasses (the*.Shared.csfiles) 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 intoScalarPlatformin commit 9a3ea37 in this PR.Finally, these simplifications permit the identification of several unused methods, specifically
ScalarPlatform.IsProcessActive()andScalarPlatform.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 whetherGetUpgrade{LogDirectoryParent,HighestAvailableVersion}Directory()on POSIX should also just throwNotImplementedException()likeGetUpgradeProtectedDataDirectory()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".Another question is whether we actually need any of what this PR moves intoPOSIXProductUpgraderPlatformStrategy.csand which is currently implemented identically in the Mac and Linux versions.Details:
We refactor and create a common abstract
POSIXFileBasedLockclass 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 theNativeMethods.ResolveSymlink()method it uses intoPOSIXPlatform, eliminating duplication between the Mac and Linux versions. To do this we need to add aMaxPathLengthvalue to thePOSIXPlatformclass and its subclasses, as the nativePATH_MAXconstant differs between Linux and macOS.The Linux platform does not need to implement the
GetUpgradeProtectedDataDirectory()method since the Mac platform already returnsNotImplementedException()for this method. Thus we consolidate these methods into a common one in thePOSIXPlatform.The
LinuxProductUpgraderPlatformStrategyclass was a simple clone of the Mac version, so we consolidate these into a commonPOSIXProductUpgraderPlatformStrategyclass. Moreover, since thePOSIXPlatformsets itsUnderConstructionFlags.UsesCustomUpgraderflag tofalse, none of theLinuxProductUpgraderPlatformStrategyclass's methods are actually called when running thescalar upgradecommand, and so we can simply change them all to throw aNotImplementedException().The
GVFS/GVFS.Common/Paths.csfile was split into two andPaths.Shared.cswas created in commit 2db0c03, in order to share methods such asPaths.GetGVFSEnlistmentRoot()with theGVFS.Hooksproject. The originalPaths.csfile was later renamed and moved toGVFS/GVFS.Platform.Windows/WindowsFileSystem.Shared.csin commit 785ccb4; that file is nowScalar.Common/Platforms/Windows/WindowsFileSystem.Shared.csand its soleTryGetNormalizedPathImplementation()method is also implemented in the parallelPOSIXFileSystem.Shared.csfile. Since the shared classes are no longer needed, we renamePaths.Shared.csback to justPaths.cs, and move the substance of theTryGetNormalizedPathImplementation()methods into the corresponding main per-platform classes for Windows and POSIX. This also allows us to convert these classes back from beingpartialones split over two files each.We drop the
GetNamedPipeNameImplementation()method fromPOSIXPlatformas the corresponding method in theMacPlatform.Shared.csfile was removed in commit 2a75485 in PR #251 and so the POSIX version is also unused.The
ProductUpgraderInfo.IsLocalUpgradeAvailable()method's only caller was in theGVFS.Hooksproject, so we can just drop that method and one private one as well from the now-consolidatedProductUpgraderInfo.csfile.As the folder name
"Scalar"is used several times in theWindowsPlatform, we refactor slightly to make that acommon constant string in a new
ScalarConstants.WindowsPlatformclass, akin to what we do for the POSIX platforms.As the
Scalar.Servicedoes not run theProductUpgradeTimeron macOS, and there is noScalar.Serviceon Linux, and because theUpgradeVerbdoes not record any highest-available version data except on Windows, there is no necessity to implement theGetUpgradeHighestAvailableVersionDirectory()method on POSIX. We can therefore just return aNotImplementedException(), and also simplify theGetUpgradeLogDirectoryParentDirectory()method, which is used on POSIX to provide the path to the log file generated by thescalar upgradecommand.The last callers of the
IsConsoleOutputRedirectedToFile()methods in theScalarPlatformclasses were removed in commit 4183579 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 theScalarPlatformclasses was removed in commit cedeeaa 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.