Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Sep 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec427d4
Working solution
SohamDas2021 Sep 10, 2024
7fda6dc
Addressed PR comments
SohamDas2021 Sep 11, 2024
b1ad44d
Addressed PR comments
SohamDas2021 Sep 11, 2024
26b1889
Merge branch 'main' into user/sodas/FixRustMove_090924
SohamDas2021 Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions tools/Customization/DevHome.Customization/Extensions/ServiceExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@ 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) =>
Copy link
Contributor

@bbonaby bbonaby Sep 10, 2024 •
edited by SohamDas2021
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough, we should create a refactor issue on GitHub

ActivatorUtilities.CreateInstance<OptimizeDevDriveDialogViewModel>(sp, cacheLocation, environmentVariable, exampleDevDriveLocation, existingDevDriveLetters, relatedEnvironmentVariablesToBeSet, relatedCacheDirectories));

services.AddSingleton<DevDriveInsightsViewModel>();
services.AddTransient<DevDriveInsightsPage>();

Expand Down
6 changes: 5 additions & 1 deletion tools/Customization/DevHome.Customization/Helpers/DevDriveCacheData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ public partial class DevDriveCacheData

public string? CacheName { get; set; }

public List<string>? CacheDirectory { get; set; }
public List<string>? CacheDirectory { get; set; }

public List<string>? RelatedCacheDirectories { get; set; } = new();

public List<string>? RelatedEnvironmentVariables { get; set; } = new();

public string? ExampleSubDirectory { get; set; }
}
14 changes: 12 additions & 2 deletions ...ation/DevHome.Customization/ViewModels/DevDriveInsights/DevDriveOptimizerCardViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ public partial class DevDriveOptimizerCardViewModel : ObservableObject

public string MakeTheChangeText { get; set; }

public List<string> RelatedEnvironmentVariablesToBeSet { get; set; }

public List<string> RelatedCacheDirectories { get; set; }

/// <summary>
/// User wants to optimize a dev drive.
/// </summary>
Expand All @@ -48,7 +52,9 @@ private async Task OptimizeDevDriveAsync(object sender)
ExistingCacheLocation,
EnvironmentVariableToBeSet,
ExampleLocationOnDevDrive,
ExistingDevDriveLetters);
ExistingDevDriveLetters,
RelatedEnvironmentVariablesToBeSet,
RelatedCacheDirectories);
var optimizeDevDriveDialog = new OptimizeDevDriveDialog(optimizeDevDriveViewModel);
optimizeDevDriveDialog.XamlRoot = settingsCard.XamlRoot;
optimizeDevDriveDialog.RequestedTheme = settingsCard.ActualTheme;
Expand All @@ -63,14 +69,18 @@ public DevDriveOptimizerCardViewModel(
string exampleLocationOnDevDrive,
string environmentVariableToBeSet,
List<string> existingDevDriveLetters,
bool environmentVariableHasValue)
bool environmentVariableHasValue,
List<string> relatedEnvironmentVariablesToBeSet,
List<string> relatedCacheDirectories)
{
OptimizeDevDriveDialogViewModelFactory = optimizeDevDriveDialogViewModelFactory;
ExistingDevDriveLetters = existingDevDriveLetters;
CacheToBeMoved = cacheToBeMoved;
ExistingCacheLocation = existingCacheLocation;
ExampleLocationOnDevDrive = exampleLocationOnDevDrive;
EnvironmentVariableToBeSet = environmentVariableToBeSet;
RelatedEnvironmentVariablesToBeSet = relatedEnvironmentVariablesToBeSet;
RelatedCacheDirectories = relatedCacheDirectories;
var stringResource = new StringResource("DevHome.Customization.pri", "DevHome.Customization/Resources");

if (environmentVariableHasValue)
Expand Down
39 changes: 37 additions & 2 deletions ...tion/DevHome.Customization/ViewModels/DevDriveInsights/OptimizeDevDriveDialogViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,17 @@ public partial class OptimizeDevDriveDialogViewModel : ObservableObject
[ObservableProperty]
private bool _isNotDevDrive;

private List<string> RelatedEnvironmentVariablesToBeSet { get; set; } = new List<string>();

private List<string> RelatedCacheDirectories { get; set; } = new List<string>();

public OptimizeDevDriveDialogViewModel(
string existingCacheLocation,
string environmentVariableToBeSet,
string exampleDevDriveLocation,
List<string> existingDevDriveLetters)
List<string> existingDevDriveLetters,
List<string> relatedEnvironmentVariablesToBeSet,
List<string> relatedCacheDirectories)
{
var stringResource = new StringResource("DevHome.Customization.pri", "DevHome.Customization/Resources");
ExistingDevDriveLetters = existingDevDriveLetters;
Expand All @@ -75,6 +81,8 @@ public OptimizeDevDriveDialogViewModel(
IsPrimaryButtonEnabled = true;
ErrorMessage = string.Empty;
IsNotDevDrive = false;
RelatedEnvironmentVariablesToBeSet = relatedEnvironmentVariablesToBeSet;
RelatedCacheDirectories = relatedCacheDirectories;
}

[RelayCommand]
Expand Down Expand Up @@ -244,6 +252,32 @@ private bool ChosenDirectoryInDevDrive(string directoryPath)
return false;
}

private bool MoveDirectories(string sourceDirectory, string targetDirectory, List<string> relatedCacheDirectories)
{
// TODO: If in future we support some cache with multiple relatedCacheDirectories, we should consider using Parallel.ForEachAsync
foreach (var relatedCacheDirectory in relatedCacheDirectories)
Copy link
Contributor

@bbonaby bbonaby Sep 10, 2024 •
edited by SohamDas2021
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
var relatedCacheDirectoryName = Path.GetFileName(relatedCacheDirectory);
if (!MoveDirectory(relatedCacheDirectory, $@"{targetDirectory}\Related Directories\{relatedCacheDirectoryName}"))
{
return false;
}
}

return MoveDirectory(sourceDirectory, targetDirectory);
}

private void SetRelatedEnvironmentVariables(List<string> relatedEnvironmentVariablesToBeSet, List<string> relatedCacheDirectories, string directoryPath)
{
var index = 0;
foreach (var relatedEnvironmentVariableToBeSet in relatedEnvironmentVariablesToBeSet)
{
var relatedCacheDirectoryName = Path.GetFileName(relatedCacheDirectories[index]);
SetEnvironmentVariable(relatedEnvironmentVariableToBeSet, $@"{directoryPath}\Related Directories\{relatedCacheDirectoryName}");
index++;
}
}

[RelayCommand]
private void DirectoryInputConfirmed()
{
Expand All @@ -255,8 +289,9 @@ private void DirectoryInputConfirmed()
{
Task.Run(() =>
{
if (MoveDirectory(ExistingCacheLocation, directoryPath))
if (MoveDirectories(ExistingCacheLocation, directoryPath, RelatedCacheDirectories))
{
SetRelatedEnvironmentVariables(RelatedEnvironmentVariablesToBeSet, RelatedCacheDirectories, directoryPath);
SetEnvironmentVariable(EnvironmentVariableToBeSet, directoryPath);
var existingCacheLocationVetted = RemovePrivacyInfo(ExistingCacheLocation);
Log.Debug($"Moved cache from {existingCacheLocationVetted} to {directoryPath}");
Expand Down
6 changes: 5 additions & 1 deletion tools/Customization/DevHome.Customization/ViewModels/DevDriveInsightsViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,8 @@ public void UpdateListViewModelList()
CacheName = "Cargo cache (Rust)",
EnvironmentVariable = "CARGO_HOME",
CacheDirectory = new List<string> { Path.Join(_userProfilePath, ".cargo") },
RelatedEnvironmentVariables = new List<string> { "RUSTUP_HOME" },
RelatedCacheDirectories = new List<string> { Path.Join(_userProfilePath, ".rustup") },
ExampleSubDirectory = Path.Join(PackagesStr, "cargo"),
},
new DevDriveCacheData
Expand Down Expand Up @@ -395,7 +397,9 @@ public void UpdateOptimizerListViewModelList()
exampleDirectory!, // example location on dev drive to move cache to
cache.EnvironmentVariable!, // environmentVariableToBeSet
existingDevDriveLetters,
!string.IsNullOrEmpty(environmentVariablePath));
!string.IsNullOrEmpty(environmentVariablePath),
cache.RelatedEnvironmentVariables!,
cache.RelatedCacheDirectories!);
DevDriveOptimizerCardCollection.Add(card);
}

Expand Down
4 changes: 3 additions & 1 deletion tools/Customization/DevHome.Customization/Views/OptimizeDevDriveDialog.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ public delegate OptimizeDevDriveDialogViewModel OptimizeDevDriveDialogViewModelF
string existingCacheLocation,
string environmentVariableToBeSet,
string exampleDevDriveLocation,
List<string> existingDevDriveLetters);
List<string> existingDevDriveLetters,
List<string> relatedEnvironmentVariablesToBeSet,
List<string> relatedCacheDirectories);
Copy link
Contributor

@bbonaby bbonaby Sep 10, 2024 •
edited by SohamDas2021
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public sealed partial class OptimizeDevDriveDialog : ContentDialog
{
Expand Down
Loading