From 094879c1be9e77e2527ed9a2c825a16a2c4466b7 Mon Sep 17 00:00:00 2001 From: David Federman Date: Fri, 9 Aug 2024 17:02:51 -0700 Subject: [PATCH] Simplify code --- src/PackagesConfigConverter/PackageInfo.cs | 15 +- src/PackagesConfigConverter/PackageUsage.cs | 18 +- .../ProjectConverter.cs | 302 ++++++++++-------- 3 files changed, 179 insertions(+), 156 deletions(-) diff --git a/src/PackagesConfigConverter/PackageInfo.cs b/src/PackagesConfigConverter/PackageInfo.cs index 36480d4..6bbffce 100644 --- a/src/PackagesConfigConverter/PackageInfo.cs +++ b/src/PackagesConfigConverter/PackageInfo.cs @@ -9,29 +9,24 @@ namespace PackagesConfigConverter { - internal sealed class PackageInfo + internal sealed class PackageInfo : IEquatable { public PackageInfo(PackageIdentity identity, PackagePathResolver packagePathResolver, VersionFolderPathResolver versionFolderPathResolver) { Identity = identity; - - InstalledPackageFilePath = PackagePathHelper.GetInstalledPackageFilePath(Identity, packagePathResolver ?? throw new ArgumentNullException(nameof(packagePathResolver))); - - RepositoryInstalledPath = Path.GetDirectoryName(InstalledPackageFilePath); - + RepositoryInstalledPath = packagePathResolver.GetInstallPath(Identity); GlobalInstalledPath = Path.GetFullPath(versionFolderPathResolver.GetInstallPath(identity.Id, identity.Version)); - - HasAnalyzers = Directory.Exists(Path.Combine(RepositoryInstalledPath, "analyzers")); + HasAnalyzers = Directory.Exists(Path.Combine(GlobalInstalledPath, "analyzers")); } public string GlobalInstalledPath { get; } - public string InstalledPackageFilePath { get; } - public PackageIdentity Identity { get; } public string RepositoryInstalledPath { get; } public bool HasAnalyzers { get; } + + public bool Equals(PackageInfo other) => Identity.Equals(other.Identity); } } \ No newline at end of file diff --git a/src/PackagesConfigConverter/PackageUsage.cs b/src/PackagesConfigConverter/PackageUsage.cs index 3b21f41..e880582 100644 --- a/src/PackagesConfigConverter/PackageUsage.cs +++ b/src/PackagesConfigConverter/PackageUsage.cs @@ -2,33 +2,21 @@ // // Licensed under the MIT license. -using System.Collections.Generic; -using System.Linq; -using CommandLine; -using Microsoft.Build.Construction; +using NuGet.LibraryModel; using NuGet.Versioning; namespace PackagesConfigConverter { internal sealed class PackageUsage { - public PackageUsage(PackageInfo packageInfo, bool isDevelopmentDependency) + public PackageUsage(PackageInfo packageInfo) { PackageInfo = packageInfo; - IsDevelopmentDependency = isDevelopmentDependency; } public PackageInfo PackageInfo { get; } - public bool IsDevelopmentDependency { get; } - - public IEnumerable AllElements => AnalyzerItems.Cast().Concat(AssemblyReferences).Concat(Imports); - - public List AnalyzerItems { get; } = new(); - - public List AssemblyReferences { get; } = new(); - - public List Imports { get; } = new(); + public LibraryIncludeFlags UsedAssets { get; set; } public bool GeneratePathProperty { get; set; } diff --git a/src/PackagesConfigConverter/ProjectConverter.cs b/src/PackagesConfigConverter/ProjectConverter.cs index cb64c57..1c60929 100644 --- a/src/PackagesConfigConverter/ProjectConverter.cs +++ b/src/PackagesConfigConverter/ProjectConverter.cs @@ -142,60 +142,34 @@ private static ISettings GetNuGetSettings(ProjectConverterSettings converterSett return Settings.LoadDefaultSettings(converterSettings.RepositoryRoot, Settings.DefaultSettingsFileName, new XPlatMachineWideSetting()); } - private ProjectItemElement AddPackageReference(ProjectItemGroupElement itemGroupElement, PackageUsage package, LockFileTargetLibrary lockFileTargetLibrary) + private ProjectItemElement AddPackageReference(ProjectItemGroupElement itemGroupElement, PackageUsage package, LockFileTargetLibrary lockFileTargetLibrary, bool isDevelopmentDependency) { - LibraryIncludeFlags includeAssets = LibraryIncludeFlags.All; - LibraryIncludeFlags excludeAssets = LibraryIncludeFlags.None; - LibraryIncludeFlags privateAssets = LibraryIncludeFlags.None; - LibraryIncludeFlags existingAssets = LibraryIncludeFlags.None; - if (lockFileTargetLibrary == null) - { - Log.LogWarning($"Package: {package.PackageId}"); - } - if (HasAnyRealFiles(lockFileTargetLibrary.Build)) { existingAssets |= LibraryIncludeFlags.Build; - if (package.Imports.Count == 0) - { - excludeAssets |= LibraryIncludeFlags.Build; - } } if (HasAnyRealFiles(lockFileTargetLibrary.CompileTimeAssemblies)) { existingAssets |= LibraryIncludeFlags.Compile; - if (package.AssemblyReferences.Count == 0) - { - excludeAssets |= LibraryIncludeFlags.Compile; - } } if (HasAnyRealFiles(lockFileTargetLibrary.RuntimeAssemblies)) { existingAssets |= LibraryIncludeFlags.Runtime; - if (package.AssemblyReferences.Count == 0) - { - excludeAssets |= LibraryIncludeFlags.Runtime; - } } // LockFileTargetLibrary doesn't include analyzer information, so we have to figure it out ourselves. if (package.PackageInfo.HasAnalyzers) { existingAssets |= LibraryIncludeFlags.Analyzers; - if (package.AnalyzerItems.Count == 0) - { - excludeAssets |= LibraryIncludeFlags.Analyzers; - } } - if (package.IsDevelopmentDependency) - { - privateAssets |= LibraryIncludeFlags.All; - } + LibraryIncludeFlags includeAssets; + LibraryIncludeFlags excludeAssets; + LibraryIncludeFlags privateAssets; if (package.IsMissingTransitiveDependency) { @@ -203,6 +177,12 @@ private ProjectItemElement AddPackageReference(ProjectItemGroupElement itemGroup excludeAssets = LibraryIncludeFlags.None; privateAssets = LibraryIncludeFlags.All; } + else + { + includeAssets = LibraryIncludeFlags.All; + excludeAssets = existingAssets & ~package.UsedAssets; + privateAssets = isDevelopmentDependency ? LibraryIncludeFlags.All : LibraryIncludeFlags.None; + } // Simplify if the inclusions or exclusions exactly equal what's in the package. if (includeAssets == existingAssets) @@ -220,9 +200,37 @@ private ProjectItemElement AddPackageReference(ProjectItemGroupElement itemGroup privateAssets = LibraryIncludeFlags.All; } - ProjectItemElement itemElement = itemGroupElement.AppendItem("PackageReference", package.PackageInfo.Identity.Id); + return AddPackageReference(itemGroupElement, package.PackageInfo.Identity, includeAssets, excludeAssets, privateAssets, package.GeneratePathProperty); + + static bool HasAnyRealFiles(IList items) + { + if (items.Count > 0) + { + // Ignore the special "_._" file. + foreach (LockFileItem item in items) + { + if (!Path.GetFileName(item.Path).Equals("_._")) + { + return true; + } + } + } + + return false; + } + } + + private ProjectItemElement AddPackageReference( + ProjectItemGroupElement itemGroupElement, + PackageIdentity packageIdentity, + LibraryIncludeFlags includeAssets, + LibraryIncludeFlags excludeAssets, + LibraryIncludeFlags privateAssets, + bool generatePathProperty) + { + ProjectItemElement itemElement = itemGroupElement.AppendItem("PackageReference", packageIdentity.Id); - itemElement.AddMetadataAsAttribute("Version", package.PackageInfo.Identity.Version.ToNormalizedString()); + itemElement.AddMetadataAsAttribute("Version", packageIdentity.Version.ToNormalizedString()); if (includeAssets != LibraryIncludeFlags.All) { @@ -239,29 +247,12 @@ private ProjectItemElement AddPackageReference(ProjectItemGroupElement itemGroup itemElement.AddMetadataAsAttribute("PrivateAssets", privateAssets.ToString()); } - if (package.GeneratePathProperty) + if (generatePathProperty) { itemElement.AddMetadataAsAttribute("GeneratePathProperty", bool.TrueString); } return itemElement; - - static bool HasAnyRealFiles(IList items) - { - if (items.Count > 0) - { - // Ignore the special "_._" file. - foreach (LockFileItem item in items) - { - if (!Path.GetFileName(item.Path).Equals("_._")) - { - return true; - } - } - } - - return false; - } } private bool ConvertProject(string projectPath, string packagesConfigPath) @@ -272,29 +263,48 @@ private bool ConvertProject(string projectPath, string packagesConfigPath) PackagesConfigReader packagesConfigReader = new PackagesConfigReader(XDocument.Load(packagesConfigPath)); - List packages = packagesConfigReader + HashSet developmentDependencies = new(); + List packages = packagesConfigReader .GetPackages(allowDuplicatePackageIds: true) .Select(i => { - PackageInfo packageInfo = _packageInfoFactory.GetPackageInfo(i.PackageIdentity); - return new PackageUsage(packageInfo, i.IsDevelopmentDependency); + if (i.IsDevelopmentDependency) + { + developmentDependencies.Add(i.PackageIdentity); + } + + return _packageInfoFactory.GetPackageInfo(i.PackageIdentity); }) .ToList(); + Log.LogDebug(" Current package references:"); + + foreach (PackageInfo package in packages) + { + Log.LogDebug($" {package.Identity}"); + } + ProjectRootElement project = ProjectRootElement.Open(projectPath, _projectCollection, preserveFormatting: true); NuGetFramework targetFramework = GetNuGetFramework(project); + RestoreResult restoreResult = Restore(packages, projectPath, targetFramework); RestoreTargetGraph restoreTargetGraph = restoreResult.RestoreGraphs.FirstOrDefault(); LockFile lockFile = restoreResult.LockFile; - DetectMissingTransitiveDependencies(packages, projectPath, restoreTargetGraph); + Dictionary packageUsages = new(packages.Count); + foreach (PackageInfo package in packages) + { + packageUsages.Add(package, new PackageUsage(package)); + } + + DetectMissingTransitiveDependencies(packageUsages, projectPath, restoreTargetGraph); ProjectItemGroupElement packageReferenceItemGroupElement = null; foreach (ProjectElement element in project.AllChildren) { - ProcessElement(element, packages); + ProcessElement(element, packageUsages); if (packageReferenceItemGroupElement == null && element is ProjectItemElement { ItemType: "Reference" }) { @@ -305,32 +315,23 @@ private bool ConvertProject(string projectPath, string packagesConfigPath) packageReferenceItemGroupElement ??= project.AddItemGroup(); - Log.LogDebug(" Current package references:"); - - foreach (PackageUsage package in packages.Where(i => !i.IsMissingTransitiveDependency)) - { - Log.LogDebug($" {package.PackageInfo.Identity}"); - } - - foreach (ProjectElement element in packages.Where(i => !i.IsMissingTransitiveDependency).SelectMany(i => i.AllElements)) - { - Log.LogDebug($" {element.Location}: Removing element {element.ToXmlString()}"); - element.Remove(); - } - if (_converterSettings.TrimPackages) { - TrimPackages(packages, projectPath, restoreTargetGraph.Flattened); + TrimPackages(packageUsages, projectPath, restoreTargetGraph); } Log.LogDebug(" Converted package references:"); - foreach (PackageUsage package in packages) + LockFileTarget lockFileTarget = lockFile.GetTarget(targetFramework, runtimeIdentifier: null); + foreach (KeyValuePair kvp in packageUsages) { - LockFileTargetLibrary lockFileTargetLibrary = lockFile.GetTarget(targetFramework, runtimeIdentifier: null).GetTargetLibrary(package.PackageId); + PackageInfo package = kvp.Key; + PackageUsage packageUsage = kvp.Value; - ProjectItemElement packageReferenceItemElement = AddPackageReference(packageReferenceItemGroupElement, package, lockFileTargetLibrary); + LockFileTargetLibrary lockFileTargetLibrary = lockFileTarget.GetTargetLibrary(packageUsage.PackageId); + bool isDevelopmentDependency = developmentDependencies.Contains(package.Identity); + ProjectItemElement packageReferenceItemElement = AddPackageReference(packageReferenceItemGroupElement, packageUsage, lockFileTargetLibrary, isDevelopmentDependency); Log.LogDebug($" {packageReferenceItemElement.ToXmlString()}"); } @@ -356,28 +357,27 @@ private bool ConvertProject(string projectPath, string packagesConfigPath) return true; } - private void DetectMissingTransitiveDependencies(List packages, string projectPath, RestoreTargetGraph restoreTargetGraph) + private void DetectMissingTransitiveDependencies(Dictionary packages, string projectPath, RestoreTargetGraph restoreTargetGraph) { IEnumerable> flatPackageDependencies = restoreTargetGraph.Flattened.Where(i => i.Key.Type == LibraryType.Package); foreach (GraphItem packageDependency in flatPackageDependencies) { - (LibraryIdentity library, _) = (packageDependency.Key, packageDependency.Data); - PackageUsage package = packages.FirstOrDefault(i => i.PackageId.Equals(library.Name, StringComparison.OrdinalIgnoreCase)); + LibraryIdentity library = packageDependency.Key; + PackageIdentity packageIdentity = new PackageIdentity(library.Name, library.Version); + PackageInfo packageInfo = _packageInfoFactory.GetPackageInfo(packageIdentity); - if (package == null) + if (!packages.ContainsKey(packageInfo)) { Log.LogWarning($"{projectPath}: The transitive package dependency \"{library.Name} {library.Version}\" was not in the packages.config. After converting to PackageReference, new dependencies will be pulled in transitively which could lead to restore or build errors"); - PackageIdentity packageIdentity = new PackageIdentity(library.Name, library.Version); - PackageInfo packageInfo = _packageInfoFactory.GetPackageInfo(packageIdentity); - PackageUsage missingTransitiveDependency = new PackageUsage(packageInfo, isDevelopmentDependency: false); + PackageUsage missingTransitiveDependency = new PackageUsage(packageInfo); missingTransitiveDependency.IsMissingTransitiveDependency = true; - packages.Add(missingTransitiveDependency); + packages.Add(packageInfo, missingTransitiveDependency); } } } - private Match GetElementMatch(ElementPath elementPath, PackageUsage package) + private Match GetElementMatch(ElementPath elementPath, PackageUsage packageUsage) { Match match = null; @@ -389,35 +389,38 @@ private Match GetElementMatch(ElementPath elementPath, PackageUsage package) if (itemElement.ItemType.Equals("Analyzer", StringComparison.OrdinalIgnoreCase)) { - match = RegularExpressions.Analyzers[package.PackageInfo.Identity].Match(elementPath.FullPath); + match = RegularExpressions.Analyzers[packageUsage.PackageInfo.Identity].Match(elementPath.FullPath); if (match.Success) { - package.AnalyzerItems.Add(itemElement); + packageUsage.UsedAssets |= LibraryIncludeFlags.Analyzers; + RemoveElement(elementPath.Element); } } else if (itemElement.ItemType.Equals("Reference", StringComparison.OrdinalIgnoreCase)) { if (File.Exists(elementPath.FullPath)) { - match = RegularExpressions.AssemblyReferences[package.PackageInfo.Identity].Match(elementPath.FullPath); + match = RegularExpressions.AssemblyReferences[packageUsage.PackageInfo.Identity].Match(elementPath.FullPath); if (match.Success) { - package.AssemblyReferences.Add(itemElement); + packageUsage.UsedAssets |= LibraryIncludeFlags.Compile | LibraryIncludeFlags.Runtime; + RemoveElement(elementPath.Element); } } } break; - case ProjectImportElement importElement: + case ProjectImportElement: - match = RegularExpressions.Imports[package.PackageInfo.Identity].Match(elementPath.FullPath); + match = RegularExpressions.Imports[packageUsage.PackageInfo.Identity].Match(elementPath.FullPath); if (match.Success) { - package.Imports.Add(importElement); + packageUsage.UsedAssets |= LibraryIncludeFlags.Build; + RemoveElement(elementPath.Element); } break; @@ -425,6 +428,12 @@ private Match GetElementMatch(ElementPath elementPath, PackageUsage package) } return match; + + void RemoveElement(ProjectElement element) + { + Log.LogDebug($" {element.Location}: Removing element {element.ToXmlString()}"); + element.Remove(); + } } private NuGetFramework GetNuGetFramework(ProjectRootElement project) @@ -445,7 +454,7 @@ private NuGetFramework GetNuGetFramework(ProjectRootElement project) return _converterSettings.DefaultTargetFramework; } - private RestoreResult Restore(List packages, string projectPath, NuGetFramework framework) + private RestoreResult Restore(List packages, string projectPath, NuGetFramework framework) { List targetFrameworks = new List { @@ -457,7 +466,7 @@ private RestoreResult Restore(List packages, string projectPath, N { Dependencies = packages.Select(i => new LibraryDependency { - LibraryRange = new LibraryRange(i.PackageId, new VersionRange(i.PackageVersion), LibraryDependencyTarget.Package), + LibraryRange = new LibraryRange(i.Identity.Id, new VersionRange(i.Identity.Version), LibraryDependencyTarget.Package), }).ToList(), RestoreMetadata = new ProjectRestoreMetadata { @@ -497,7 +506,7 @@ private bool IsPathRootedInRepositoryPath(string path) return path != null && path.StartsWith(_repositoryPath, StringComparison.OrdinalIgnoreCase); } - private void ProcessElement(ProjectElement element, List packages) + private void ProcessElement(ProjectElement element, Dictionary packageUsages) { switch (element) { @@ -536,76 +545,107 @@ private void ProcessElement(ProjectElement element, List packages) bool elementPathIsValid = false; - foreach (PackageUsage package in packages.Where(i => !i.IsMissingTransitiveDependency)) + foreach (KeyValuePair kvp in packageUsages) { - Match match = GetElementMatch(elementPath, package); + PackageInfo package = kvp.Key; + PackageUsage packageUsage = kvp.Value; + if (packageUsage.IsMissingTransitiveDependency) + { + continue; + } + + Match match = GetElementMatch(elementPath, packageUsage); if (match != null && match.Success && match.Groups["version"] != null && match.Groups["version"].Success && NuGetVersion.TryParse(match.Groups["version"].Value, out NuGetVersion version)) { elementPathIsValid = true; - if (!version.Equals(package.PackageInfo.Identity.Version)) + if (!version.Equals(package.Identity.Version)) { - Log.LogWarning($" {element.Location}: The package version \"{version}\" specified in the \"{element.ElementName}\" element does not match the package version \"{package.PackageVersion}\". After conversion, the project will reference ONLY version \"{package.PackageVersion}\""); + Log.LogWarning($" {element.Location}: The package version \"{version}\" specified in the \"{element.ElementName}\" element does not match the package version \"{package.Identity.Version}\". After conversion, the project will reference ONLY version \"{packageUsage.PackageVersion}\""); } } } if (!elementPathIsValid && IsPathRootedInRepositoryPath(elementPath.FullPath)) { - PackageUsage package = packages.FirstOrDefault(i => !string.IsNullOrEmpty(i.PackageInfo.RepositoryInstalledPath) && elementPath.FullPath.StartsWith(i.PackageInfo.RepositoryInstalledPath)); - - if (package == null) - { - // TODO: Using a package that isn't referenced - } - else + bool foundPackage = false; + foreach (KeyValuePair kvp in packageUsages) { - string path = Path.Combine(package.PackageInfo.GlobalInstalledPath, elementPath.FullPath.Substring(package.PackageInfo.RepositoryInstalledPath.Length + 1)); + PackageInfo package = kvp.Key; + PackageUsage packageUsage = kvp.Value; - if (!File.Exists(path) && !Directory.Exists(path)) + if (!string.IsNullOrEmpty(kvp.Key.RepositoryInstalledPath) && elementPath.FullPath.StartsWith(kvp.Key.RepositoryInstalledPath)) { - // TODO: Using a path that doesn't exist? - } - else - { - string rootedPath = path.Substring(_globalPackagesFolder.Length); - string[] splitPath = - rootedPath.Split( - new[] - { + foundPackage = true; + string path = Path.Combine(package.GlobalInstalledPath, elementPath.FullPath.Substring(package.RepositoryInstalledPath.Length + 1)); + + if (!File.Exists(path) && !Directory.Exists(path)) + { + Log.LogWarning($"Using path from package which doesn't exist in the package. This needs manual intervention. Path: {elementPath.FullPath}"); + } + else + { + string rootedPath = path.Substring(_globalPackagesFolder.Length); + string[] splitPath = + rootedPath.Split( + new[] + { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar, - }, - StringSplitOptions.RemoveEmptyEntries); - string relativePath = string.Join(Path.DirectorySeparatorChar.ToString(), splitPath.Skip(2)); - string generatedProperty = $"$(Pkg{package.PackageId.Replace(".", "_")})"; - path = $"{generatedProperty}\\{relativePath}"; - - package.GeneratePathProperty = true; - elementPath.Set(path); + }, + StringSplitOptions.RemoveEmptyEntries); + string relativePath = string.Join(Path.DirectorySeparatorChar.ToString(), splitPath.Skip(2)); + string generatedProperty = $"$(Pkg{package.Identity.Id.Replace(".", "_")})"; + path = $"{generatedProperty}\\{relativePath}"; + + packageUsage.GeneratePathProperty = true; + elementPath.Set(path); + } + + break; } } + + if (!foundPackage) + { + Log.LogWarning($"Package is being used which is not referenced. This needs manual intervention. Usage: {elementPath.FullPath}"); + } } } - private void TrimPackages(List packages, string projectPath, ICollection> flatPackageDependencies) + private void TrimPackages(Dictionary packageUsages, string projectPath, RestoreTargetGraph restoreTargetGraph) { - IEnumerable GetPackageDependencies(PackageUsage package) => flatPackageDependencies.First(p => p.Key.Name.Equals(package.PackageId, StringComparison.OrdinalIgnoreCase)).Data.Dependencies; + HashSet nonTopLevelPackages = new(StringComparer.OrdinalIgnoreCase); + foreach (GraphItem item in restoreTargetGraph.Flattened) + { + // Skip non-packages, like the project itself which will obviously depend on the top-level packages. + if (item.Key.Type != LibraryType.Package) + { + continue; + } - bool IsPackageInDependencySet(PackageUsage package, IEnumerable dependencies) => dependencies.Any(d => d.Name.Equals(package.PackageId, StringComparison.OrdinalIgnoreCase)); + foreach (LibraryDependency dependency in item.Data.Dependencies) + { + nonTopLevelPackages.Add(dependency.Name); + } + } - packages.RemoveAll(package => + HashSet packagesToTrim = new(); + foreach (KeyValuePair kvp in packageUsages) { - bool willRemove = packages.Select(GetPackageDependencies).Any(dependencies => IsPackageInDependencySet(package, dependencies)); - - if (willRemove) + PackageInfo package = kvp.Key; + if (nonTopLevelPackages.Contains(package.Identity.Id)) { - Log.LogWarning($"{projectPath}: The transitive package dependency {package.PackageId} {package.PackageVersion} will be removed because it is referenced by another package in this dependency graph."); + Log.LogWarning($"{projectPath}: The transitive package dependency {package.Identity} will be removed because it is referenced by another package in this dependency graph."); + packagesToTrim.Add(package); } + } - return willRemove; - }); + foreach (PackageInfo package in packagesToTrim) + { + packageUsages.Remove(package); + } } } } \ No newline at end of file