Remove all vestiges of the mount process from C# code

Remove all vestiges of the mount process and related actions (mount, unmount) from the C# code and documentation. Mount references are left in the Mac notifications app for now so we can use that code as a reference to add upgrade notifications later, like we have on Windows. This is also preventing upgrade from working since it's trying to do unmount/remount during installation.
microsoft · Jan 28, 2020 · 12b4010 · 12b4010
2 parents 9d5b43b + 5d1f04d
commit 12b4010
Show file tree

Hide file tree

Showing 22 changed files with 207 additions and 462 deletions.
diff --git a/AuthoringTests.md b/AuthoringTests.md
@@ -92,23 +92,19 @@ Mac:
 
 ## How to Write a Functional Test
 
-Each piece of functionality that we add to Scalar should have corresponding functional tests that clone a repo, mount the filesystem, and use existing tools and filesystem
-APIs to interact with the virtual repo.
+Each piece of functionality that we add to Scalar should have corresponding functional tests that clone a repo and use existing tools and filesystem APIs to interact with the virtual repo.
 
 Since these are functional tests that can potentially modify the state of files on disk, you need to be careful to make sure each test can run in a clean 
 environment.  There are three base classes that you can derive from when writing your tests.  It's also important to put your new class into the same namespace
 as the base class, because NUnit treats namespaces like test suites, and we have logic that keys off that for deciding when to create enlistments.
 
 1. `TestsWithLongRunningEnlistment`
 
-    Before any test in this namespace is executed, we create a single enlistment and mount Scalar.  We then run all tests in this namespace that derive
-	from this base class.  Only put tests in here that are purely readonly and will leave the repo in a good state for future tests.
+    Before any test in this namespace is executed, we create a single enlistment.  We then run all tests in this namespace that derive from this base class.  Only put tests in here that are purely readonly and will leave the repo in a good state for future tests.
 
 2. `TestsWithEnlistmentPerFixture`
 
-    For any test fixture (a fixture is the same as a class in NUnit) that derives from this class, we create an enlistment and mount Scalar before running
-	any of the tests in the fixture, and then we unmount and delete the enlistment after all tests are done (but before any other fixture runs).  If you need
-	to write a sequence of tests that manipulate the same repo, this is the right base class.
+    For any test fixture (a fixture is the same as a class in NUnit) that derives from this class, we create an enlistment before running any of the tests in the fixture, and then we delete the enlistment after all tests are done (but before any other fixture runs).  If you need to write a sequence of tests that manipulate the same repo, this is the right base class.
 
 3. `TestsWithEnlistmentPerTestCase`
 

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -2,19 +2,18 @@
 
 Thank you for taking the time to contribute!
 
-## Guidelines
-
-* [Contributor License Agreement](#contributor-license-agreement)
-* [Code of Conduct](#code-of-conduct)
-* [Building Scalar on Windows](#building-scalar-on-windows)
-* [Building Scalar on Mac](#building-scalar-on-mac)
-* [Design Reviews](#design-reviews)
-* [Platform Specific Code](#platform-specific-code)
-* [Tracing and Logging](#tracing-and-logging)
-* [Error Handling](#error-handling)
-* [Background Threads](#background-threads)
-* [Coding Conventions](#coding-conventions)
-* [Testing](#testing)
+ - [Contributor License Agreement](#contributor-license-agreement)
+ - [Code of Conduct](#code-of-conduct)
+ - [Building Scalar on Windows](#building-scalar-on-windows)
+ - [Building Scalar on Mac](#building-scalar-on-mac)
+ - [Design Reviews](#design-reviews)
+ - [Platform Specific Code](#platform-specific-code)
+ - [Tracing and Logging](#tracing-and-logging)
+ - [Error Handling](#error-handling)
+ - [Background Threads](#background-threads)
+ - [Coding Conventions](#coding-conventions)
+ - [Testing](#testing)
+   - [C# Unit Tests](#c-unit-tests)
 
 ## Contributor License Agreement
 
@@ -115,19 +114,19 @@ The design review process is as follows:
   catch (Exception e)
   {
     EventMetadata metadata = new EventMetadata();
-    metadata.Add("Area", "Mount");
-    metadata.Add(nameof(enlistmentHookPath), enlistmentHookPath);
-    metadata.Add(nameof(installedHookPath), installedHookPath);
+    metadata.Add("Area", "Upgrade");
+    metadata.Add(nameof(packageVersion), packageVersion);
+    metadata.Add(nameof(packageName), packageName);
     metadata.Add("Exception", e.ToString());
-    context.Tracer.RelatedError(metadata, $"Failed to compare {hook.Name} version");
+    context.Tracer.RelatedError(metadata, $"Failed to compare {packageName} version");
   }
   ```
 
 ## Error Handling
 
 - *Fail fast: An error or exception that risks data loss or corruption should shut down Scalar immediately*
 
-  Preventing data loss and repository corruption is critical.  If an error or exception occurs that could lead to data loss, it's better to shut down Scalar than keep the repository mounted and risk corruption.
+  Preventing data loss and repository corruption is critical.  If an error or exception occurs that could lead to data loss, it's better to shut down Scalar than risk corruption.
 
 - *Do not catch exceptions that are indicative of a programmer error (e.g. `ArgumentNullException`)*
 
@@ -222,7 +221,7 @@ The design review process is as follows:
 
 - <a id="bgexceptions"></a>*Catch all exceptions on long-running tasks and background threads*
 
-  Wrap all code that runs in the background thread in a top-level `try/catch(Exception)`.  Any exceptions caught by this handler should be logged, and then Scalar should be forced to terminate with `Environment.Exit`.  It's not safe to allow Scalar to continue to run after an unhandled exception stops a background thread or long-running task.  Testing has shown that `Environment.Exit` consistently terminates the Scalar mount process regardless of how background threads are started (e.g. native thread, `new Thread()`, `Task.Factory.StartNew()`).
+  Wrap all code that runs in the background thread in a top-level `try/catch(Exception)`.  Any exceptions caught by this handler should be logged, and then Scalar should be forced to terminate with `Environment.Exit`.  It's not safe to allow Scalar to continue to run after an unhandled exception stops a background thread or long-running task.  Testing has shown that `Environment.Exit` consistently terminates the process regardless of how background threads are started (e.g. native thread, `new Thread()`, `Task.Factory.StartNew()`).
 
   An example of this pattern can be seen in [`BackgroundFileSystemTaskRunner.ProcessBackgroundTasks`](https://github.com/Microsoft/Scalar/blob/4baa37df6bde2c9a9e1917fc7ce5debd653777c0/Scalar/Scalar.Virtualization/Background/BackgroundFileSystemTaskRunner.cs#L233).
 

diff --git a/Scalar.Common/GitHubUpgrader.cs b/Scalar.Common/GitHubUpgrader.cs
@@ -19,7 +19,7 @@ public class GitHubUpgrader : ProductUpgrader
         private const string JSONMediaType = @"application/vnd.github.v3+json";
         private const string UserAgent = @"Scalar_Auto_Upgrader";
         private const string CommonInstallerArgs = "/VERYSILENT /CLOSEAPPLICATIONS /SUPPRESSMSGBOXES /NORESTART";
-        private const string ScalarInstallerArgs = CommonInstallerArgs + " /REMOUNTREPOS=false";
+        private const string ScalarInstallerArgs = CommonInstallerArgs;
         private const string GitInstallerArgs = CommonInstallerArgs + " /ALLOWDOWNGRADE=1";
         private const string GitAssetId = "Git";
         private const string ScalarAssetId = "Scalar";

diff --git a/Scalar.Common/InstallerPreRunChecker.cs b/Scalar.Common/InstallerPreRunChecker.cs
@@ -44,32 +44,6 @@ public virtual bool TryRunPreUpgradeChecks(out string consoleError)
             return true;
         }
 
-        // TODO: Move repo mount calls to Scalar.Upgrader project.
-        // https://github.com/Microsoft/Scalar/issues/293
-        public virtual bool TryMountAllScalarRepos(out string consoleError)
-        {
-            return this.TryRunScalarWithArgs("service --mount-all", out consoleError);
-        }
-
-        public virtual bool TryUnmountAllScalarRepos(out string consoleError)
-        {
-            consoleError = null;
-            this.tracer.RelatedInfo("Unmounting any mounted Scalar repositories.");
-
-            using (ITracer activity = this.tracer.StartActivity(nameof(this.TryUnmountAllScalarRepos), EventLevel.Informational))
-            {
-                if (!this.TryRunScalarWithArgs("service --unmount-all", out consoleError))
-                {
-                    this.tracer.RelatedError($"{nameof(this.TryUnmountAllScalarRepos)}: {consoleError}");
-                    return false;
-                }
-
-                activity.RelatedInfo("Successfully unmounted repositories.");
-            }
-
-            return true;
-        }
-
         public virtual bool IsInstallationBlockedByRunningProcess(out string consoleError)
         {
             consoleError = null;

diff --git a/Scalar.Common/Maintenance/GitMaintenanceStep.cs b/Scalar.Common/Maintenance/GitMaintenanceStep.cs
@@ -30,25 +30,25 @@ public GitMaintenanceStep(ScalarContext context, bool requireObjectCacheLock, Gi
         protected bool RequireObjectCacheLock { get; }
         protected GitProcessChecker GitProcessChecker { get; }
 
-        public static bool EnlistmentRootReady(ScalarContext context)
-        {
-            // If a user locks their drive or disconnects an external drive while the mount process
-            // is running, then it will appear as if the directories below do not exist or throw
-            // a "Device is not ready" error.
-            try
-            {
-                return context.FileSystem.DirectoryExists(context.Enlistment.EnlistmentRoot)
-                         && context.FileSystem.DirectoryExists(context.Enlistment.GitObjectsRoot);
-            }
-            catch (IOException)
-            {
-                return false;
-            }
+        public static bool EnlistmentRootReady(ScalarContext context)
+        {
+            // If a user locks their drive or disconnects an external drive while the process
+            // is running, then it will appear as if the directories below do not exist or throw
+            // a "Device is not ready" error.
+            try
+            {
+                return context.FileSystem.DirectoryExists(context.Enlistment.EnlistmentRoot)
+                         && context.FileSystem.DirectoryExists(context.Enlistment.GitObjectsRoot);
+            }
+            catch (IOException)
+            {
+                return false;
+            }
         }
 
-        public bool EnlistmentRootReady()
-        {
-            return EnlistmentRootReady(this.Context);
+        public bool EnlistmentRootReady()
+        {
+            return EnlistmentRootReady(this.Context);
         }
 
         public void Execute()
@@ -85,20 +85,20 @@ public void Execute()
             }
             catch (Exception e)
             {
-                if (this.EnlistmentRootReady())
-                {
-                    this.Context.Tracer.RelatedError(
-                        metadata: this.CreateEventMetadata(e),
-                        message: "Exception while running action: " + e.Message,
-                        keywords: Keywords.Telemetry);
-                }
-                else
-                {
-                    this.Context.Tracer.RelatedWarning(
-                        metadata: this.CreateEventMetadata(e),
-                        message: "Exception while running action inside a repo that's not ready: " + e.Message);
-                }
-
+                if (this.EnlistmentRootReady())
+                {
+                    this.Context.Tracer.RelatedError(
+                        metadata: this.CreateEventMetadata(e),
+                        message: "Exception while running action: " + e.Message,
+                        keywords: Keywords.Telemetry);
+                }
+                else
+                {
+                    this.Context.Tracer.RelatedWarning(
+                        metadata: this.CreateEventMetadata(e),
+                        message: "Exception while running action inside a repo that's not ready: " + e.Message);
+                }
+
                 Environment.Exit((int)ReturnCode.GenericError);
             }
         }
@@ -188,7 +188,7 @@ protected GitProcess.Result RunGitCommand(Func<GitProcess, GitProcess.Result> wo
                 {
                     this.Context.Tracer.RelatedWarning(
                         metadata: null,
-                        message: $"{this.Area}: Not launching Git process {gitCommand} because the mount is stopping",
+                        message: $"{this.Area}: Not launching Git process {gitCommand} because the process is stopping",
                         keywords: Keywords.Telemetry);
                     throw new StoppingException();
                 }

diff --git a/Scalar.Common/Platforms/Windows/WindowsPlatform.Shared.cs b/Scalar.Common/Platforms/Windows/WindowsPlatform.Shared.cs
@@ -50,7 +50,7 @@ public static bool IsProcessActiveImplementation(int processId, bool tryGetProce
                 }
                 else if (tryGetProcessById)
                 {
-                    // The process.IsInvalid may be true when the mount process doesn't have access to call
+                    // The process.IsInvalid may be true when the process doesn't have access to call
                     // OpenProcess for the specified processId. Fallback to slow way of finding process.
                     try
                     {

diff --git a/Scalar.Common/ScalarConstants.cs b/Scalar.Common/ScalarConstants.cs
@@ -83,14 +83,10 @@ public static class SpecialGitFiles
 
         public static class LogFileTypes
         {
-            public const string MountPrefix = "mount";
             public const string UpgradePrefix = "productupgrade";
 
             public const string Clone = "clone";
             public const string Maintenance = "maintenance";
-            public const string MountVerb = MountPrefix + "_verb";
-            public const string MountProcess = MountPrefix + "_process";
-            public const string MountUpgrade = MountPrefix + "_repoupgrade";
             public const string Repair = "repair";
             public const string Service = "service";
             public const string ServiceUI = "service_ui";
@@ -195,23 +191,6 @@ public static class VerbParameters
         {
             public const string InternalUseOnly = "internal_use_only";
 
-            public static class Mount
-            {
-                public const string StartedByService = "StartedByService";
-                public const string StartedByVerb = "StartedByVerb";
-                public const string Verbosity = "verbosity";
-                public const string Keywords = "keywords";
-                public const string DebugWindow = "debug-window";
-
-                public const string DefaultVerbosity = "Informational";
-                public const string DefaultKeywords = "Any";
-            }
-
-            public static class Unmount
-            {
-                public const string SkipLock = "skip-wait-for-lock";
-            }
-
             public static class Maintenance
             {
                 public const string AllTasksName = "all";

diff --git a/Scalar.Common/ScalarEnlistment.cs b/Scalar.Common/ScalarEnlistment.cs
@@ -45,7 +45,7 @@ private ScalarEnlistment(string enlistmentRoot, string workingDirectory, string
 
         public bool UsesGvfsProtocol { get; protected set; }
 
-        // These version properties are only used in logging during clone and mount to track version numbers
+        // These version properties are only used in logging during clone to track version numbers
         public string GitVersion
         {
             get { return this.gitVersion; }
@@ -85,7 +85,7 @@ public static ScalarEnlistment CreateFromDirectory(
 
         public static string GetNewScalarLogFileName(
             string logsRoot,
-            string logFileType,
+            string logFileType,
             string logId = null,
             PhysicalFileSystem fileSystem = null)
         {
@@ -100,7 +100,7 @@ public static bool TryGetScalarEnlistmentRoot(
                                 string directory,
                                 out string enlistmentRoot,
                                 out string workingDirectoryRoot,
-                                Func<string, bool> exists = null)
+                                Func<string, bool> exists = null)
         {
             if (exists == null)
             {

diff --git a/Scalar.FunctionalTests/Tests/MultiEnlistmentTests/SharedCacheTests.cs b/Scalar.FunctionalTests/Tests/MultiEnlistmentTests/SharedCacheTests.cs
@@ -111,8 +111,6 @@ public void SecondCloneSucceedsWithMissingTrees()
             result.ExitCode.ShouldEqual(0, $"git {command} failed on {nameof(enlistment2)} with error: {result.Errors}");
         }
 
-        // Override OnTearDownEnlistmentsDeleted rathern than using [TearDown] as the enlistments need to be unmounted before
-        // localCacheParentPath can be deleted (as the SQLite blob sizes database cannot be deleted while Scalar is mounted)
         protected override void OnTearDownEnlistmentsDeleted()
         {
             RepositoryHelpers.DeleteTestDirectory(this.localCacheParentPath);

diff --git a/Scalar.FunctionalTests/Tests/MultiEnlistmentTests/TestsWithMultiEnlistment.cs b/Scalar.FunctionalTests/Tests/MultiEnlistmentTests/TestsWithMultiEnlistment.cs
@@ -22,7 +22,7 @@ public void DeleteEnlistments()
         }
 
         /// <summary>
-        /// Can be overridden for custom [TearDown] steps that occur after the test enlistements have been unmounted and deleted
+        /// Can be overridden for custom [TearDown] steps that occur after the test enlistments have been deleted
         /// </summary>
         protected virtual void OnTearDownEnlistmentsDeleted()
         {

diff --git a/Scalar.FunctionalTests/Tests/TestResultsHelper.cs b/Scalar.FunctionalTests/Tests/TestResultsHelper.cs
@@ -20,18 +20,7 @@ public static void OutputScalarLogs(ScalarFunctionalTestEnlistment enlistment)
 
             foreach (string filename in GetAllFilesInDirectory(enlistment.ScalarLogsRoot))
             {
-                if (filename.Contains("mount_process"))
-                {
-                    // Validate that all mount processes started by the functional tests were started
-                    // by verbs, and that "StartedByVerb" was set to true when the mount process was launched
-                    OutputFileContents(
-                        filename,
-                        contents => contents.ShouldContain("\"StartedByVerb\":true"));
-                }
-                else
-                {
-                    OutputFileContents(filename);
-                }
+                OutputFileContents(filename);
             }
         }
 

diff --git a/Scalar.FunctionalTests/Tools/ScalarProcess.cs b/Scalar.FunctionalTests/Tools/ScalarProcess.cs
@@ -40,22 +40,6 @@ public void Clone(string repositorySource, string branchToCheckout, bool skipFet
             this.CallScalar(args, expectedExitCode: SuccessExitCode);
         }
 
-        public void Mount()
-        {
-            string output;
-            this.TryMount(out output).ShouldEqual(true, "Scalar did not mount: " + output);
-
-            // TODO: Re-add this warning after we work out the version detail information
-            // output.ShouldNotContain(ignoreCase: true, unexpectedSubstrings: "warning");
-        }
-
-        public bool TryMount(out string output)
-        {
-            this.IsEnlistmentMounted().ShouldEqual(false, "Scalar is already mounted");
-            output = this.CallScalar("mount \"" + this.enlistmentRoot + "\"");
-            return this.IsEnlistmentMounted();
-        }
-
         public string RunVerb(string task, long? batchSize = null, bool failOnError = true)
         {
             string batchArg = batchSize == null
@@ -106,21 +90,6 @@ public string CacheServer(string args)
             return this.CallScalar("cache-server " + args + " \"" + this.enlistmentRoot + "\"");
         }
 
-        public void Unmount()
-        {
-            if (this.IsEnlistmentMounted())
-            {
-                string result = this.CallScalar("unmount \"" + this.enlistmentRoot + "\"", expectedExitCode: SuccessExitCode);
-                this.IsEnlistmentMounted().ShouldEqual(false, "Scalar did not unmount: " + result);
-            }
-        }
-
-        public bool IsEnlistmentMounted()
-        {
-            string statusResult = this.CallScalar("status \"" + this.enlistmentRoot + "\"");
-            return statusResult.Contains("Mount status: Ready");
-        }
-
         public string RunServiceVerb(string argument)
         {
             return this.CallScalar("service " + argument, expectedExitCode: SuccessExitCode);

diff --git a/Scalar.Installer.Mac/scripts/preinstall b/Scalar.Installer.Mac/scripts/preinstall