[Dev Drive Insights] Move %userprofile$/.rustup and set RUSTUP_HOME as part of optimizing Cargo cache #3827

SohamDas2021 · 2024-09-10T19:35:48Z

Summary of the pull request

Cargo cache has dependencies on %userprofile$/.rustup. So we move this directory into the dev drive.

References and relevant issues

#2743

Detailed description of the pull request / Additional comments

Cargo cache has dependencies on %userprofile$/.rustup. So we move this directory into the dev drive.

Validation steps performed

Manually validated that the cache move works as expected.

PR checklist

Closes Dev drive optimization suggestion for Rust #2743

bbonaby · 2024-09-10T20:26:42Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

+            if (!MoveDirectory(relatedCacheDirectory, targetDirectory + "\\Related Directories\\" + relatedCacheDirectoryName))
+            {


Suggested change

if (!MoveDirectory(relatedCacheDirectory, targetDirectory + "\\Related Directories\\" + relatedCacheDirectoryName))

{

if (!MoveDirectory(relatedCacheDirectory, $@"{targetDirectory}\Related Directories\{relatedCacheDirectoryName}"))

Just a cleaner way to do it in C# see https://learn.microsoft.com/en-us/dotnet/csharp/tutorials/string-interpolation#how-to-use-escape-sequences-in-an-interpolated-string #Resolved

Also are we choosing to call the directory "Related Directories" or is that always the name we expect for a folder containing directories related to the cache. Just wanted to confirm for my future knowledge.

We are choosing the name. The dir structure on the dev drive is as follows:

The main would move into the dev drive E: (for example) as E:\packages<cache>.

A related dir to this cache would go into E:\packages<cache>\RelatedDirectories<reldir>

Hope that clears confusion.

bbonaby · 2024-09-10T20:48:24Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

+    private string GetDirectoryNameFromPath(string path)
+    {
+        var lastIndex = path.LastIndexOf('\\');
+        return path.Substring(lastIndex + 1);
+    }


could we use the static method in .Nets Path class to get the directory name? I mean this one

Then on line 267 below we can do something like:

var relatedCacheDirectoryName = Path.GetDirectoryName(relatedCacheDirectory);

Then we can leave the edge cases to that. Unless there is a reason we don't want to use this. #Resolved

Path.GetFileName is what we need here. Thanks for the recommendation. Updated.

bbonaby · 2024-09-10T20:53:25Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

+        var index = 0;
+        foreach (var relatedEnvironmentVariableToBeSet in relatedEnvironmentVariablesToBeSet)
+        {
+            var relatedCacheDirectoryName = GetDirectoryNameFromPath(relatedCacheDirectories[index]);


is the length of relatedEnvironmentVariableToBeSet always guaranteed to be the same length of relatedCacheDirectories? #ByDesign

That is a good question, for the current case- yes. Generally speaking I would assume the same because it would be through the Env variables that the paths to these dirs would be determined.

bbonaby · 2024-09-10T20:54:01Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

+        foreach (var relatedEnvironmentVariableToBeSet in relatedEnvironmentVariablesToBeSet)
+        {
+            var relatedCacheDirectoryName = GetDirectoryNameFromPath(relatedCacheDirectories[index]);
+            SetEnvironmentVariable(relatedEnvironmentVariableToBeSet, directoryPath + "\\Related Directories\\" + relatedCacheDirectoryName);


Same comment about string interpolation above and the naming of "Related Directories" #Resolved

bbonaby · 2024-09-10T21:01:21Z

tools/Customization/DevHome.Customization/Views/OptimizeDevDriveDialog.xaml.cs

-    List<string> existingDevDriveLetters);
+    List<string> existingDevDriveLetters,
+    List<string> relatedEnvironmentVariablesToBeSet,
+    List<string> relatedCacheDirectories);


In the future we should move this delegate to the OptimizeDevDriveDialogViewModel.cs since its a factory delegate to create it. Would help with readability since we'd be able to see the OptimizeDevDriveDialogViewModel.cs class in the same file when an edit to the factory delegate is made.

But also see comment in the first file. We probably don't need to add these two to the factory delegate if we pass in the cache directly. #Resolved

bbonaby · 2024-09-10T23:10:29Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

+
+    private bool MoveDirectories(string sourceDirectory, string targetDirectory, List<string> relatedCacheDirectories)
+    {
+        foreach (var relatedCacheDirectory in relatedCacheDirectories)


Depending on how many directories we get, we might want to think about doing this in parallel with a Parallel.ForEach loop. #WontFix

That is a great suggestion. Currently we only have single RelatedDirectories. So I am holding off to making the change. I have left a TODO comment to do this in future.

Fair, we could probably remove the TODO word itself though. I think the comment is good as is, incase a future dev is looking at the code

bbonaby · 2024-09-10T23:12:32Z

tools/Customization/DevHome.Customization/Extensions/ServiceExtensions.cs

@@ -21,8 +21,8 @@ public static IServiceCollection AddWindowsCustomization(this IServiceCollection
        services.AddTransient<FileExplorerPage>();

        services.AddSingleton<OptimizeDevDriveDialogViewModelFactory>(sp =>
-            (cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters) =>
-                ActivatorUtilities.CreateInstance<OptimizeDevDriveDialogViewModel>(sp, cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters));
+            (cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters, relatedEnvironmentVariablesToBeSet, relatedCacheDirectories) =>


looking at how we use these and seeing how many we could potentially add now and in the future. What if we passed the DevDriveCacheData directly instead of passing its properties?I believe this would eliminate 4 of these parameters in
OptimizeDevDriveDialogViewModel's constructor. Thoughts? This also saves us in the future if we add more cache properties #WontFix

While I agree with your comment. I am going to hold off making this change for now since this has a potential to regress existing functionality.

fair enough, we should create a refactor issue on GitHub

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs

tools/Customization/DevHome.Customization/Helpers/DevDriveCacheData.cs

bbonaby · 2024-09-12T19:02:50Z

...ization/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs

+
+    private bool MoveDirectories(string sourceDirectory, string targetDirectory, List<string> relatedCacheDirectories)
+    {
+        foreach (var relatedCacheDirectory in relatedCacheDirectories)


Fair, we could probably remove the TODO word itself though. I think the comment is good as is, incase a future dev is looking at the code

bbonaby · 2024-09-12T19:03:20Z

tools/Customization/DevHome.Customization/Extensions/ServiceExtensions.cs

@@ -21,8 +21,8 @@ public static IServiceCollection AddWindowsCustomization(this IServiceCollection
        services.AddTransient<FileExplorerPage>();

        services.AddSingleton<OptimizeDevDriveDialogViewModelFactory>(sp =>
-            (cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters) =>
-                ActivatorUtilities.CreateInstance<OptimizeDevDriveDialogViewModel>(sp, cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters));
+            (cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters, relatedEnvironmentVariablesToBeSet, relatedCacheDirectories) =>


fair enough, we should create a refactor issue on GitHub

Working solution

ec427d4

SohamDas2021 requested review from bbonaby and kanismohammed September 10, 2024 19:35

SohamDas2021 changed the title ~~Move %userprofile$/.rustup and set RUSTUP_HOME as part of optimizing Cargo cache~~ [Dev Drive Insights] Move %userprofile$/.rustup and set RUSTUP_HOME as part of optimizing Cargo cache Sep 10, 2024

bbonaby reviewed Sep 11, 2024

View reviewed changes

SohamDas2021 added 2 commits September 11, 2024 12:18

Addressed PR comments

7fda6dc

Addressed PR comments

b1ad44d

SohamDas2021 requested a review from bbonaby September 11, 2024 20:11

bbonaby approved these changes Sep 12, 2024

View reviewed changes

kanismohammed approved these changes Sep 12, 2024

View reviewed changes

Merge branch 'main' into user/sodas/FixRustMove_090924

26b1889

SohamDas2021 merged commit 92c6eba into main Sep 12, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dev Drive Insights] Move %userprofile$/.rustup and set RUSTUP_HOME as part of optimizing Cargo cache #3827

[Dev Drive Insights] Move %userprofile$/.rustup and set RUSTUP_HOME as part of optimizing Cargo cache #3827

SohamDas2021 commented Sep 10, 2024

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024

SohamDas2021 Sep 11, 2024

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Sep 11, 2024

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Sep 11, 2024

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Sep 11, 2024

bbonaby Sep 12, 2024

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

SohamDas2021 Sep 11, 2024

bbonaby Sep 12, 2024

bbonaby Sep 12, 2024

bbonaby Sep 12, 2024

		if (!MoveDirectory(relatedCacheDirectory, targetDirectory + "\\Related Directories\\" + relatedCacheDirectoryName))
		{

[Dev Drive Insights] Move %userprofile$/.rustup and set RUSTUP_HOME as part of optimizing Cargo cache #3827

[Dev Drive Insights] Move %userprofile$/.rustup and set RUSTUP_HOME as part of optimizing Cargo cache #3827

Conversation

SohamDas2021 commented Sep 10, 2024

Summary of the pull request

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

bbonaby Sep 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbonaby Sep 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbonaby Sep 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbonaby Sep 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

bbonaby Sep 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

bbonaby Sep 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbonaby Sep 10, 2024 • edited by SohamDas2021 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading

bbonaby Sep 10, 2024 •

edited by SohamDas2021

Loading