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

Make NuGet detector more conservative about applying isDevelopmentDependency #1314

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Make NuGet detector more conservative about applying isDevelopmentDependency #1314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f562fc9
Make NuGet detector more conservative about applying isDevelopmentDep…
ericstj Nov 25, 2024
a387e94
Merge branch 'main' into adjustNuGetDevDependencies
grvillic Dec 4, 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
48 changes: 32 additions & 16 deletions ...crosoft.ComponentDetection.Detectors/nuget/NuGetPackageReferenceFrameworkAwareDetector.cs
View file
Open in desktop
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
Open in desktop
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
Loading