Skip to content

Commit

Permalink
Make NuGet detector more conservative about applying isDevelopmentDep…
Browse files Browse the repository at this point in the history 
…endency

Most PackageDownloads will be used for runtime, so opt to treat these as non-development-dependencies.

For other packages, only consider them development dependencies when they don't contribute assets from any of the following categories: runtime, runtimeTargets, resource assemblies, native, content, build, build multitargeting, and analyzers.
  • Loading branch information
@ericstj
ericstj committed Nov 25, 2024
1 parent 952c1ce commit f562fc9
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 25 deletions.
48 changes: 32 additions & 16 deletions ...crosoft.ComponentDetection.Detectors/nuget/NuGetPackageReferenceFrameworkAwareDetector.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,28 @@ private static string[] GetFrameworkReferences(LockFile lockFile, LockFileTarget
return results.Distinct().ToArray();
}

private static bool IsADevelopmentDependency(LockFileTargetLibrary library, LockFile lockFile)
{
// a placeholder item is an empty file that doesn't exist with name _._ meant to indicate an empty folder in a nuget package, but also used by NuGet when a package's assets are excluded.
static bool IsAPlaceholderItem(LockFileItem item) => Path.GetFileName(item.Path).Equals(PackagingCoreConstants.EmptyFolder, StringComparison.OrdinalIgnoreCase);

// All(IsAPlaceholderItem) checks if the collection is empty or all items are placeholders.
return library.RuntimeAssemblies.All(IsAPlaceholderItem) &&
library.RuntimeTargets.All(IsAPlaceholderItem) &&
library.ResourceAssemblies.All(IsAPlaceholderItem) &&
library.NativeLibraries.All(IsAPlaceholderItem) &&
library.ContentFiles.All(IsAPlaceholderItem) &&
library.Build.All(IsAPlaceholderItem) &&
library.BuildMultiTargeting.All(IsAPlaceholderItem) &&

// The SDK looks at the library for analyzers using the following hueristic:
// https://github.com/dotnet/sdk/blob/d7fe6e66d8f67dc93c5c294a75f42a2924889196/src/Tasks/Microsoft.NET.Build.Tasks/NuGetUtils.NuGet.cs#L43
(!lockFile.GetLibrary(library.Name, library.Version)?.Files
.Any(file => file.StartsWith("analyzers", StringComparison.Ordinal)
&& file.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)
&& !file.EndsWith(".resources.dll", StringComparison.OrdinalIgnoreCase)) ?? false);
}

protected override Task OnFileFoundAsync(ProcessRequest processRequest, IDictionary<string, string> detectorArgs, CancellationToken cancellationToken = default)
{
try
Expand Down Expand Up @@ -89,13 +111,15 @@ protected override Task OnFileFoundAsync(ProcessRequest processRequest, IDiction
{
var frameworkReferences = GetFrameworkReferences(lockFile, target);
var frameworkPackages = FrameworkPackages.GetFrameworkPackages(target.TargetFramework, frameworkReferences);
bool IsAFrameworkPackage(string id, NuGetVersion version) => frameworkPackages.Any(fp => fp.IsAFrameworkComponent(id, version));
bool IsFrameworkOrDevelopmentDependency(LockFileTargetLibrary library) =>
frameworkPackages.Any(fp => fp.IsAFrameworkComponent(library.Name, library.Version)) ||
IsADevelopmentDependency(library, lockFile);

// This call to GetTargetLibrary is not guarded, because if this can't be resolved then something is fundamentally broken (e.g. an explicit dependency reference not being in the list of libraries)
// issue: we treat top level dependencies for all targets as top level for each target, but some may not be top level for other targets, or may not even be present for other targets.
foreach (var library in explicitReferencedDependencies.Select(x => target.GetTargetLibrary(x.Name)).Where(x => x != null))
{
this.NavigateAndRegister(target, explicitlyReferencedComponentIds, singleFileComponentRecorder, library, null, IsAFrameworkPackage);
this.NavigateAndRegister(target, explicitlyReferencedComponentIds, singleFileComponentRecorder, library, null, IsFrameworkOrDevelopmentDependency);
}
}

Expand All @@ -117,17 +141,14 @@ private void NavigateAndRegister(
ISingleFileComponentRecorder singleFileComponentRecorder,
LockFileTargetLibrary library,
string parentComponentId,
Func<string, NuGetVersion, bool> isAFrameworkComponent,
Func<LockFileTargetLibrary, bool> isDevelopmentDependency,
HashSet<string> visited = null)
{
if (library.Type == ProjectDependencyType)
{
return;
}

var isFrameworkComponent = isAFrameworkComponent(library.Name, library.Version);
var isDevelopmentDependency = IsADevelopmentDependency(library);

visited ??= [];

var libraryComponent = new DetectedComponent(new NuGetComponent(library.Name, library.Version.ToNormalizedString()));
Expand All @@ -137,7 +158,7 @@ private void NavigateAndRegister(
libraryComponent,
explicitlyReferencedComponentIds.Contains(libraryComponent.Component.Id),
parentComponentId,
isDevelopmentDependency: isFrameworkComponent || isDevelopmentDependency,
isDevelopmentDependency: isDevelopmentDependency(library),
targetFramework: target.TargetFramework?.GetShortFolderName());

foreach (var dependency in library.Dependencies)
Expand All @@ -153,15 +174,9 @@ private void NavigateAndRegister(
if (targetLibrary != null)
{
visited.Add(dependency.Id);
this.NavigateAndRegister(target, explicitlyReferencedComponentIds, singleFileComponentRecorder, targetLibrary, libraryComponent.Component.Id, isAFrameworkComponent, visited);
this.NavigateAndRegister(target, explicitlyReferencedComponentIds, singleFileComponentRecorder, targetLibrary, libraryComponent.Component.Id, isDevelopmentDependency, visited);
}
}

// a placeholder item is an empty file that doesn't exist with name _._ meant to indicate an empty folder in a nuget package, but also used by NuGet when a package's assets are excluded.
bool IsAPlaceholderItem(LockFileItem item) => Path.GetFileName(item.Path).Equals(PackagingCoreConstants.EmptyFolder, StringComparison.OrdinalIgnoreCase);

// A library is development dependency if all of the runtime assemblies and runtime targets are placeholders or empty (All returns true for empty).
bool IsADevelopmentDependency(LockFileTargetLibrary library) => library.RuntimeAssemblies.Concat(library.RuntimeTargets).All(IsAPlaceholderItem);
}

private void RegisterPackageDownloads(ISingleFileComponentRecorder singleFileComponentRecorder, LockFile lockFile)
Expand All @@ -177,12 +192,13 @@ private void RegisterPackageDownloads(ISingleFileComponentRecorder singleFileCom

var libraryComponent = new DetectedComponent(new NuGetComponent(packageDownload.Name, packageDownload.VersionRange.MinVersion.ToNormalizedString()));

// PackageDownload is always a development dependency since it's usage does not make it part of the application
// Conservatively assume that PackageDownloads are not develeopment dependencies even though usage will not effect any runtime behavior.
// Most often they are used for some runtime deployment -- runtime packs, host packs, AOT infrastructure, etc, so opt in treating them as non-development-dependencies.
singleFileComponentRecorder.RegisterUsage(
libraryComponent,
isExplicitReferencedDependency: true,
parentComponentId: null,
isDevelopmentDependency: true,
isDevelopmentDependency: false,
targetFramework: framework.FrameworkName?.GetShortFolderName());
}
}
Expand Down
19 changes: 10 additions & 9 deletions ...ponentDetection.Detectors.Tests/nuget/NuGetPackageReferenceFrameworkAwareDetectorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public async Task ScanDirectoryAsync_Base_2_2_VerificationAsync()
detectedComponents.Should().HaveCount(22);

var nonDevComponents = detectedComponents.Where(c => !componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).GetValueOrDefault());
nonDevComponents.Should().HaveCount(3);
nonDevComponents.Should().HaveCount(17);

foreach (var component in detectedComponents)
{
Expand All @@ -68,7 +68,7 @@ public async Task ScanDirectoryAsync_Base_2_2_additional_VerificationAsync()
detectedComponents.Should().HaveCount(68);

var nonDevComponents = detectedComponents.Where(c => !componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).GetValueOrDefault());
nonDevComponents.Should().HaveCount(22);
nonDevComponents.Should().HaveCount(27);
nonDevComponents.Select(x => x.Component).Cast<NuGetComponent>().FirstOrDefault(x => x.Name.Contains("Polly")).Should().NotBeNull();
nonDevComponents.Select(x => x.Component).Cast<NuGetComponent>().Count(x => x.Name.Contains("System.Composition")).Should().Be(5);

Expand All @@ -90,9 +90,10 @@ public async Task ScanDirectoryAsync_ExcludedFrameworkComponent_2_2_Verification
.WithFile(this.projectAssetsJsonFileName, osAgnostic)
.ExecuteDetectorAsync();

var developmentDependencies = componentRecorder.GetDetectedComponents().Where(c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).GetValueOrDefault());
developmentDependencies.Should().HaveCountGreaterThan(5, "Ommitted framework assemblies are missing. There should be more than ten, but this is a gut check to make sure we have data.");
developmentDependencies.Should().Contain(c => c.Component.Id.StartsWith("Microsoft.NETCore.App "), "Microsoft.NETCore.App should be treated as a development dependency.");
var dependencies = componentRecorder.GetDetectedComponents();
var developmentDependencies = dependencies.Where(c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).GetValueOrDefault());
developmentDependencies.Should().HaveCount(5);
developmentDependencies.Should().Contain(c => c.Component.Id.StartsWith("Microsoft.NETCore.Platforms "), "Microsoft.NETCore.Platforms should be treated as a development dependency.");
}

[TestMethod]
Expand Down Expand Up @@ -326,10 +327,10 @@ public async Task ScanDirectoryAsync_PackageDownload_VerificationAsync()
.WithFile(this.projectAssetsJsonFileName, TestResources.project_assets_packageDownload)
.ExecuteDetectorAsync();

var developmentDependencies = componentRecorder.GetDetectedComponents().Where(c => componentRecorder.GetEffectiveDevDependencyValue(c.Component.Id).GetValueOrDefault());
developmentDependencies.Should().HaveCount(3, "PackageDownload dev dependencies should exist.");
developmentDependencies.Select(c => c.Component).Should().AllBeOfType<NuGetComponent>();
developmentDependencies.Select(c => c.TargetFrameworks).Should().AllSatisfy(tfms => tfms.Should().BeEquivalentTo(["net8.0"]));
var dependencies = componentRecorder.GetDetectedComponents();
dependencies.Should().HaveCount(3, "PackageDownload dependencies should exist.");
dependencies.Select(c => c.Component).Should().AllBeOfType<NuGetComponent>();
dependencies.Select(c => c.TargetFrameworks).Should().AllSatisfy(tfms => tfms.Should().BeEquivalentTo(["net8.0"]));
}

[TestMethod]
Expand Down

0 comments on commit f562fc9

Please sign in to comment.