Refactor DotnetInstall types #51140
Refactor DotnetInstall types #51140
Conversation
| { | ||
| private readonly DotnetInstallRequest _request; | ||
| private readonly DotnetInstall _install; | ||
| private readonly ReleaseVersion _resolvedVersion; |
There was a problem hiding this comment.
I think it's a great idea to enforce aggregation over inheritance.
| namespace Microsoft.DotNet.Tools.Bootstrapper | ||
| { | ||
| public enum InstallMode | ||
| public enum InstallComponent |
There was a problem hiding this comment.
I'm happy to change this to a class called InstallComponents which has an enum if you'd like. In case we want to have something that we can add multiple components to or sub components of the sdk.
| private bool InstallAlreadyExists(DotnetInstall install) | ||
| { | ||
| var existingInstalls = GetExistingInstalls(directory); | ||
| var existingInstalls = GetExistingInstalls(install.InstallRoot); |
There was a problem hiding this comment.
Great point to remove this duplication.
| public record InstallRequestOptions() | ||
| { | ||
| // Include things such as the custom feed here. | ||
| // Do we need a GUID for the ID here? |
There was a problem hiding this comment.
I was imagining the GUID could be used to identify unique installs that point to the same component. E.g. my global.json in repo A installed .net 9.0.201 x64 SDK, but repo B also installed it. They might have a different UpdateChannel. The GUID also makes it easier in logs to see what particular component install failed, in my opinion.
| } | ||
| } | ||
|
|
||
| // TODO: InstallerOrchestratorSingleton also checks existing installs via the manifest. Which should we use and where? |
There was a problem hiding this comment.
I think we must verify both as the manifest could be wrong if the user deletes it themselves.
| { | ||
| using var releaseManifest = new ReleaseManifest(); | ||
| var archiveName = $"dotnet-{_install.Id}"; | ||
| var archiveName = $"dotnet-{Guid.NewGuid()}"; |
There was a problem hiding this comment.
I would defer back to using the in-class GUID based on the comment there.
|
|
||
| var installs = GetInstalledVersions().ToList(); | ||
| installs.RemoveAll(i => i.Id == version.Id && i.FullySpecifiedVersion == version.FullySpecifiedVersion); | ||
| installs.RemoveAll(i => DnupUtilities.PathsEqual(i.InstallRoot.Path, version.InstallRoot.Path) && i.Version.Equals(version.Version)); |
There was a problem hiding this comment.
I think we might actually want an ID comparison here if we want to have 'dependents' to an install where we remove the dependents and ref count. And we may have a force option that removes them like this logic does. With that said, I haven't thought that logic through 100% yet so it might change.
New types include:
DotnetInstallRoot- Represents a dotnet root folder on disk where components can be installedDotnetInstall- Represents a component and its version installed in an install root. (I renamed InstallMode to InstallComponent)DotnetInstallRequestRepresents a request to install a component, where the version may not be fully specifiedUpdateChannel- Represents a specific version or a "channel", which specifies which updates will apply