-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
scalar run all
#295
scalar run all
#295
Conversation
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Scalar/CommandLine/RunVerb.cs
Outdated
@@ -52,6 +56,8 @@ protected override void Execute(ScalarEnlistment enlistment) | |||
ScalarConstants.LogFileTypes.Maintenance, | |||
logId: this.StartedByService ? "service" : null); | |||
|
|||
List<ActionAndMessage> actions = new List<ActionAndMessage>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my thoughts.
Change this to be List<GitMaintenanceStep> maintenanceSteps = new List<GitMaintenanceStep>();
Add to the GitMaintenanceStep
public abstract string UserMessage { get; }
The foreach
would become
foreach (GitMaintenanceStep step in maintenanceSteps)
{
this.ShowStatusWhileRunning(() => { step.Execute(); return true; }, step.UserMessage);
}
That would eliminate the ActionAndMessage
class and most of the Get..Action
methods in favor of the constructors.
Also if we wanted to simplify even more, in the GitMaintenanceStep class we could have a
Dictionary<string, List<GitMaintenanceStep>> availableSteps;
to make the switch statement here be something like
if (availableSteps .ContainsKey(this.MaintenanceTask)
{
foreach (GitMaintenanceStep step in availableSteps [this.MaintenanceTask])
{
this.ShowStatusWhileRunning(() => { step.Execute(); return true; }, step.UserMessage);
}
}
else
{
this.ReportErrorAndExit($"Unknown maintenance task requested: '{this.MaintenanceTask}'");
}
That might make it messier though and we would be initializing every step each time to run maybe one of them which might not be good. Up to you on that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like your easier foreach
, but calling the constructors for the different steps can be complicated (especially the FetchStep
) and it would be better to not create one when we don't need one.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
The FetchStepFailsWhenItCannotRemoveABadPrefetchPack() functional test fails at present, as it looks for specific output from the FetchStep maintenance command in the form of an "Unable to delete" message. This string is no longer written to standard output by the command, however, due to the changes in PR microsoft#295 which introduced a standard progress meter. Instead, that string is only written using this.Context.Tracer.RelatedWarning() in the PerformMaintenance() method of Scalar.Common.Maintenance.FetchStep, and thus appears only in the maintenance log files. The breakage of the test was not noticed because it belongs to the ExtraCoverage category, and therefore has not been running as part of the CI suite on Windows because the RunFunctionalTests.bat script does not supply the --full-suite option. To resolve the test failure we simply check for the continued presence of the locked bad pack file after the first fetch attempt instead of parsing for a specific text string. The test is also Windows-specific because it depends on an open file handle with a FileShare.None attribute blocking attempts by other processes to delete the file, and this only works in C# on the Windows platform. On Unix platforms, the C# core libraries implement file sharing in an advisory manner only; see: https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88 We therefore mark this test as WindowsOnly, since it effectively does not apply to other platforms. Note that the previous category assigned to the test, MacTODO.TestNeedsToLockFile, dates from VFSForGit and has been migrated to just this one test as a result of changes in PR microsoft#170. However, the meaning of that category varies, as the one other functional test in the category uses Scalar's FileBasedLock and not C# FileShare settings, so we clarify the situation by just marking this WindowsOnly.
The FetchStepFailsWhenItCannotRemoveABadPrefetchPack() functional test fails at present, as it looks for specific output from the FetchStep maintenance command in the form of an "Unable to delete" message. This string is no longer written to standard output by the command, however, due to the changes in PR microsoft#295 which introduced a standard progress meter. Instead, that string is only written using this.Context.Tracer.RelatedWarning() in the PerformMaintenance() method of Scalar.Common.Maintenance.FetchStep, and thus appears only in the maintenance log files. The breakage of the test was not noticed because it belongs to the ExtraCoverage category, and therefore has not been running as part of the CI suite on Windows because the RunFunctionalTests.bat script does not supply the --full-suite option. To resolve the test failure we simply check for the continued presence of the locked bad pack file after the first fetch attempt instead of parsing for a specific text string. The test is also Windows-specific because it depends on an open file handle with a FileShare.None attribute blocking attempts by other processes to delete the file, and this only works in C# on the Windows platform. On Unix platforms, the C# core libraries implement file sharing in an advisory manner only; see: https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88 We therefore mark this test as WindowsOnly, since it effectively does not apply to other platforms. Note that the previous category assigned to the test, MacTODO.TestNeedsToLockFile, dates from VFSForGit and has been migrated to just this one test as a result of changes in PR microsoft#170. However, the meaning of that category varies, as the one other functional test in the category uses Scalar's FileBasedLock and not C# FileShare settings, so we clarify the situation by just marking this WindowsOnly.
The FetchStepFailsWhenItCannotRemoveABadPrefetchPack() functional test fails at present, as it looks for specific output from the FetchStep maintenance command in the form of an "Unable to delete" message. This string is no longer written to standard output by the command, however, due to the changes in PR microsoft#295 which introduced a standard progress meter. Instead, that string is only written using this.Context.Tracer.RelatedWarning() in the PerformMaintenance() method of Scalar.Common.Maintenance.FetchStep, and thus appears only in the maintenance log files. The breakage of the test was not noticed because it belongs to the ExtraCoverage category, and therefore has not been running as part of the CI suite on Windows because the RunFunctionalTests.bat script does not supply the --full-suite option. To resolve the test failure we simply check for the continued presence of the locked bad pack file after the first fetch attempt instead of parsing for a specific text string. The test is also Windows-specific because it depends on an open file handle with a FileShare.None attribute blocking attempts by other processes to delete the file, and this only works in C# on the Windows platform. On Unix platforms, the C# core libraries implement file sharing in an advisory manner only; see: https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88 We therefore mark this test as WindowsOnly, since it effectively does not apply to other platforms. Note that the previous category assigned to the test, MacTODO.TestNeedsToLockFile, dates from VFSForGit and has been migrated to just this one test as a result of changes in PR microsoft#170. However, the meaning of that category varies, as the one other functional test in the category uses Scalar's FileBasedLock and not C# FileShare settings, so we clarify the situation by just marking this WindowsOnly.
Resolves #287.
The first commit refactors some functional test code to make it a bit easier to see that we are running
scalar run <task>
in these tests. Also reduces the code required to add a new task (i.e. the "all" task).The second commit adds a new "all" task. This requires modifying the code quite a bit to pull off with less duplication.
The end result looks like this:
The ordering is documented in
docs/advanced.md
, including some reasoning for the order. Basically, earlier steps may produce data that can be used by the later steps (fetch gets new packs and loose objects, commit-graph operates on new refs and may download new loose objects, loose-object step creates a pack, pack-file step repacks).Notice that an output line is being added to every task, where previously only the "fetch" task had this kind of output.
There is a slight change of behavior here that was missed when we started running the verb from the service: we had
forceRun: true
in some of the steps that check a timestamp before running. This should not be true when run from the service, as the service wants to ensure a foreground run delays background runs for the right length of time. This is really only important for multiple enlistments sharing an object cache.