Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
49 changes: 42 additions & 7 deletions src/NuGet.Core/NuGet.Build.Tasks.Pack/GetPackOutputItemsTask.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,18 @@ public class GetPackOutputItemsTask : Microsoft.Build.Utilities.Task
[Required]
public string NuspecOutputPath { get; set; }

public string NuspecFile { get; set; }

public string[] NuspecProperties { get; set; }

public bool IncludeSymbols { get; set; }

public bool IncludeSource { get; set; }

public string SymbolPackageFormat { get; set; }

public bool OutputFileNamesWithoutVersion { get; set; }

/// <summary>
/// Output items
/// </summary>
Expand All @@ -43,27 +49,56 @@ public class GetPackOutputItemsTask : Microsoft.Build.Utilities.Task

public override bool Execute()
{
NuGetVersion version;
if (!NuGetVersion.TryParse(PackageVersion, out version))
var packageId = PackageId;
var packageVersion = PackageVersion;

if (!string.IsNullOrWhiteSpace(NuspecFile))
{
bool hasVersionInNuspecProperties = false;
if (NuspecProperties != null && NuspecProperties.Length > 0)
{
PackArgs packArgs = new PackArgs();
PackTaskLogic.SetPackArgsPropertiesFromNuspecProperties(packArgs, MSBuildStringUtility.TrimAndExcludeNullOrEmpty(NuspecProperties));
if (packArgs.Properties.ContainsKey("version"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given the method above only sets it if the version exists, do we even need this check?
is a null check for packargs.version enough?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The key‑existence check is necessary.
If we rely only on checking whether the value is non‑null, there is a possibility that a value from another key ends up changing the version due to the implementation.

{
packageVersion = packArgs.Version;
hasVersionInNuspecProperties = true;
}
if (packArgs.Properties.TryGetValue("id", out var idTemp))
{
packageId = idTemp;
}
}

var nuspecReader = new NuGet.Packaging.NuspecReader(NuspecFile);
packageId = nuspecReader.GetId();
if (!hasVersionInNuspecProperties)
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

When NuspecProperties contains an id override, you assign packageId = idTemp, but then unconditionally overwrite it with nuspecReader.GetId(). This makes the id property ineffective and can reintroduce filename mismatches when the nuspec <id> uses tokens replaced via nuspec properties. Consider only using nuspecReader.GetId() when no id was provided via NuspecProperties, or apply the same property substitution logic that Pack uses when reading nuspecs.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fix priority

{
packageVersion = nuspecReader.GetVersion().ToNormalizedString();
}
}

if (!NuGetVersion.TryParse(packageVersion, out var versionTemp))
{
throw new ArgumentException(string.Format(
CultureInfo.CurrentCulture,
Strings.InvalidPackageVersion,
PackageVersion));
packageVersion));
}
NuGetVersion version = versionTemp!;

var symbolPackageFormat = PackArgs.GetSymbolPackageFormat(MSBuildStringUtility.TrimAndGetNullForEmpty(SymbolPackageFormat));
var nupkgFileName = PackCommandRunner.GetOutputFileName(PackageId, version, isNupkg: true, symbols: false, symbolPackageFormat: symbolPackageFormat);
var nuspecFileName = PackCommandRunner.GetOutputFileName(PackageId, version, isNupkg: false, symbols: false, symbolPackageFormat: symbolPackageFormat);
var nupkgFileName = PackCommandRunner.GetOutputFileName(packageId, version!, isNupkg: true, symbols: false, symbolPackageFormat: symbolPackageFormat, excludeVersion: OutputFileNamesWithoutVersion);
var nuspecFileName = PackCommandRunner.GetOutputFileName(packageId, version!, isNupkg: false, symbols: false, symbolPackageFormat: symbolPackageFormat, excludeVersion: OutputFileNamesWithoutVersion);

var outputs = new List<ITaskItem>();
outputs.Add(new TaskItem(Path.Combine(PackageOutputPath, nupkgFileName)));
outputs.Add(new TaskItem(Path.Combine(NuspecOutputPath, nuspecFileName)));

if (IncludeSource || IncludeSymbols)
{
var nupkgSymbolsFileName = PackCommandRunner.GetOutputFileName(PackageId, version, isNupkg: true, symbols: true, symbolPackageFormat: symbolPackageFormat);
var nuspecSymbolsFileName = PackCommandRunner.GetOutputFileName(PackageId, version, isNupkg: false, symbols: true, symbolPackageFormat: symbolPackageFormat);
var nupkgSymbolsFileName = PackCommandRunner.GetOutputFileName(packageId, version, isNupkg: true, symbols: true, symbolPackageFormat: symbolPackageFormat, excludeVersion: OutputFileNamesWithoutVersion);
var nuspecSymbolsFileName = PackCommandRunner.GetOutputFileName(packageId, version, isNupkg: false, symbols: true, symbolPackageFormat: symbolPackageFormat, excludeVersion: OutputFileNamesWithoutVersion);

outputs.Add(new TaskItem(Path.Combine(PackageOutputPath, nupkgSymbolsFileName)));
outputs.Add(new TaskItem(Path.Combine(NuspecOutputPath, nuspecSymbolsFileName)));
Expand Down
30 changes: 22 additions & 8 deletions src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,7 @@ public PackArgs GetPackArgs(IPackTaskRequest<IMSBuildItem> request)

if (!string.IsNullOrEmpty(request.NuspecFile))
{
if (request.NuspecProperties != null && request.NuspecProperties.Any())
{
packArgs.Properties.AddRange(ParsePropertiesAsDictionary(request.NuspecProperties));
if (packArgs.Properties.TryGetValue("version", out var version))
{
packArgs.Version = version;
}
}
SetPackArgsPropertiesFromNuspecProperties(packArgs, request.NuspecProperties);
}
else
{
Expand Down Expand Up @@ -1091,6 +1084,27 @@ private static IDictionary<string, string> ParsePropertiesAsDictionary(string[]
return dictionary;
}

internal static void SetPackArgsPropertiesFromNuspecProperties(PackArgs packArgs, string[] nuspecProperties)
{
if (nuspecProperties == null || !nuspecProperties.Any())
{
return;
}

packArgs.Properties.AddRange(ParsePropertiesAsDictionary(nuspecProperties));
if (packArgs.Properties.TryGetValue("version", out var packageVersion))
{
if (!NuGetVersion.TryParse(packageVersion, out var version))
{
throw new ArgumentException(string.Format(
CultureInfo.CurrentCulture,
Strings.InvalidPackageVersion,
packageVersion));
}
packArgs.Version = version.ToNormalizedString();
}
}

private HashSet<string> InitOutputExtensions(IEnumerable<string> outputExtensions)
{
return new HashSet<string>(outputExtensions.Distinct(StringComparer.OrdinalIgnoreCase));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,15 +108,21 @@ Copyright (c) .NET Foundation. All rights reserved.

<!-- 'PackageOutputAbsolutePath' and 'NuspecOutputAbsolutePath' will be provided by '_GetAbsoluteOutputPathsForPack' target -->

<ConvertToAbsolutePath Condition="$(NuspecFile) != ''" Paths="$(NuspecFile)">
<Output TaskParameter="AbsolutePaths" PropertyName="NuspecFileAbsolutePath" />
</ConvertToAbsolutePath>

<GetPackOutputItemsTask
PackageOutputPath="$(PackageOutputAbsolutePath)"
NuspecOutputPath="$(NuspecOutputAbsolutePath)"
NuspecFile="$(NuspecFileAbsolutePath)"
NuspecProperties="$(NuspecProperties)"
PackageId="$(PackageId)"
PackageVersion="$(PackageVersion)"
IncludeSymbols="$(IncludeSymbols)"
IncludeSource="$(IncludeSource)"
SymbolPackageFormat="$(SymbolPackageFormat)">

SymbolPackageFormat="$(SymbolPackageFormat)"
OutputFileNamesWithoutVersion="$(OutputFileNamesWithoutVersion)">
<Output
TaskParameter="OutputPackItems"
ItemName="_OutputPackItems" />
Expand Down
146 changes: 146 additions & 0 deletions test/NuGet.Core.Tests/NuGet.Build.Tasks.Pack.Test/BuildFixture.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
#nullable enable

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using Microsoft.Internal.NuGet.Testing.SignedPackages.ChildProcess;
using NuGet.Frameworks;
using NuGet.Test.Utility;
using Xunit;

namespace NuGet.Build.Tasks.Pack.Test
{
[CollectionDefinition(Name)]
public class FixtureCollection
: ICollectionFixture<BuildFixture>
{
internal const string Name = "Build Tests";
}

public class BuildFixture : IDisposable
{
#if DEBUG
const string CONFIGURATION = "Debug";
#else
const string CONFIGURATION = "Release";
#endif

const string FILENAME_DLL = "NuGet.Build.Tasks.Pack.dll";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't be easier to add these tests into DotnetIntegrationTests instead of creating a whole fixture that does the same logic.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. Moving this to an integration test would introduce a larger set of changes and dificulty for me.
And the logic under test is primarily unit‑level, so I’d prefer to keep it as a unit test.

If PackTask is updated in the future to expose the output file name directly,
This test can be converted into a pure unit test instead of relying on a build step.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So there's other tests that can invoke tasks:
https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Core.Tests/NuGet.Build.Tasks.Test/GetCentralPackageVersionsTaskTests.cs

What makes this one different and incompatible with that?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As explained in this issue, NuGet.Build.Tasks.Pack.PackTask does not reveal the final output filename unless Execute is actually called.
Because PackTask has so many parameters, it is extremely difficult to assign all of them in a test without causing issues, without going through NuGet.Build.Tasks.Pack.targets.

I could not find any existing tests that execute PackTask.Execute, and it appears that no one has attempted to test this area before.

In the end, I had to go through the parameters one by one to find a combination that would not produce errors, and then rewrite the test to call the function directly.

const string FILENAME_TARGETS = "NuGet.Build.Tasks.Pack.targets";

internal readonly bool _isDotNetFramework = false;
internal readonly string _testFrameworkMoniker = "netstandard2.0";

#if IS_DESKTOP
private const string SdkVersion = "10";
private const string SdkTfm = "net10.0";
#endif
internal readonly string _pathDotnetExe;
internal readonly string _pathMSBuildExe;
internal readonly string _pathDllFile;
internal readonly string _pathTargetsFile;

internal readonly IReadOnlyDictionary<string, string> _dotnetEnvironments;

public BuildFixture()
{
_pathDotnetExe = NuGet.Test.Utility.TestFileSystemUtility.GetDotnetCli();

#if IS_DESKTOP
var _cliDirectory = TestDotnetCLiUtility.CopyAndPatchLatestDotnetCli(SdkVersion, SdkTfm);
#else
string testAssemblyPath = Path.GetFullPath(System.Reflection.Assembly.GetExecutingAssembly().Location);
var _cliDirectory = TestDotnetCLiUtility.CopyAndPatchLatestDotnetCli(testAssemblyPath);
#endif
var dotnetExecutableName = NuGet.Common.RuntimeEnvironmentHelper.IsWindows ? "dotnet.exe" : "dotnet";
_pathDotnetExe = Path.Combine(_cliDirectory, dotnetExecutableName);

var sdkPath = Directory.EnumerateDirectories(Path.Combine(_cliDirectory, "sdk"))
.Single(d => !string.Equals(Path.GetFileName(d), "NuGetFallbackFolder", StringComparison.OrdinalIgnoreCase));

_pathMSBuildExe = GetMsBuildExePath();
_testFrameworkMoniker = GetFrameworkMoniker(typeof(NuGet.Build.Tasks.Pack.GetPackOutputItemsTask), out var isDotNetFramework);
_isDotNetFramework = isDotNetFramework;

var artifactsDirectory = NuGet.Test.Utility.TestFileSystemUtility.GetArtifactsDirectoryInRepo();
var dllLocation = typeof(NuGet.Build.Tasks.Pack.GetPackOutputItemsTask).Assembly.Location;
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

dllLocation is declared but never used, which will produce a compiler warning that becomes an error because TreatWarningsAsErrors is enabled. Remove the variable (or use it if it was meant to influence path resolution).

Suggested change
var dllLocation = typeof(NuGet.Build.Tasks.Pack.GetPackOutputItemsTask).Assembly.Location;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this variable is used by next code

var dllDirectory = Path.Combine(artifactsDirectory, "NuGet.Build.Tasks.Pack", "bin", CONFIGURATION, _testFrameworkMoniker);
if (!System.IO.Directory.Exists(dllDirectory))
{
dllDirectory = Path.Combine(artifactsDirectory, "NuGet.Build.Tasks.Pack", "bin", CONFIGURATION, _testFrameworkMoniker);
}
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

This if (!Directory.Exists(dllDirectory)) block reassigns dllDirectory to the exact same value, so it has no effect and makes the path resolution logic harder to follow. Either remove the block, or update it to the intended fallback path (if a different directory was meant here).

Suggested change
if (!System.IO.Directory.Exists(dllDirectory))
{
dllDirectory = Path.Combine(artifactsDirectory, "NuGet.Build.Tasks.Pack", "bin", CONFIGURATION, _testFrameworkMoniker);
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fix the issue where the same code was executed twice by correcting the first call's arguments.


_pathDllFile = Path.Combine(dllDirectory, FILENAME_DLL);

// https://github.com/NuGet/NuGet.Client/pull/6712
// NuGet.Build.Tasks.Pack.targets has been moved to in NuGet.Build.Tasks project.
// Therefore, NuGet.Build.Tasks project must be built before running this test.
var tfmTargets = GetFrameworkMoniker(typeof(PackageFileNameTests), out var _);
_pathTargetsFile = Path.Combine(artifactsDirectory, "NuGet.Build.Tasks", "bin", CONFIGURATION, tfmTargets, FILENAME_TARGETS);
if (!System.IO.File.Exists(_pathTargetsFile))
{
_pathTargetsFile = Path.Combine(artifactsDirectory, "NuGet.Build.Tasks", "bin", CONFIGURATION, _testFrameworkMoniker, FILENAME_TARGETS);
if (!System.IO.File.Exists(_pathTargetsFile))
{
_pathTargetsFile = Path.Combine(dllDirectory, FILENAME_TARGETS);
}
}

_dotnetEnvironments = new Dictionary<string, string>()
{
["MSBuildSDKsPath"] = Path.Combine(sdkPath, "Sdks"),
["DOTNET_MULTILEVEL_LOOKUP"] = "0",
["DOTNET_ROOT"] = _cliDirectory,
["MSBuildExtensionsPath"] = new DirectoryInfo(sdkPath).FullName,
["PATH"] = $"{_cliDirectory}{Path.PathSeparator}{Environment.GetEnvironmentVariable("PATH")}"
};

Assert.True(System.IO.File.Exists(_pathDllFile), $"{FILENAME_DLL} missing");
Assert.True(System.IO.File.Exists(_pathTargetsFile), $"{FILENAME_TARGETS} missing");
}

private static string GetFrameworkMoniker(Type typeInAssembly, out bool isDotNetFramework)
{
var assembly = typeInAssembly.Assembly;
var targetFrameworkAttribute
= assembly.GetCustomAttributes(typeof(System.Runtime.Versioning.TargetFrameworkAttribute), false)
.OfType<System.Runtime.Versioning.TargetFrameworkAttribute>().FirstOrDefault();

Assert.True(targetFrameworkAttribute != null, "Can't get targetFramework version");

isDotNetFramework = targetFrameworkAttribute.FrameworkName.Contains(".NETFramework");
return NuGetFramework.Parse(targetFrameworkAttribute.FrameworkName).GetShortFolderName();
}

private static string GetMsBuildExePath()
{
if (System.Environment.OSVersion.Platform == System.PlatformID.Win32NT)
{
var msbuildexe = System.IO.Path.Combine(System.Environment.GetFolderPath(System.Environment.SpecialFolder.Windows), "Microsoft.NET", "Framework", "v4.0.30319", "msbuild.exe");

var vswhereexe = System.IO.Path.Combine(System.Environment.GetFolderPath(System.Environment.SpecialFolder.ProgramFilesX86), "Microsoft Visual Studio", "Installer", "vswhere.exe");
var runresult = CommandRunner.Run(
vswhereexe,
System.Environment.CurrentDirectory,
@" -latest -find MSBuild\**\Bin\MSBuild.exe");
if (runresult.Success)
{
msbuildexe = new System.IO.StringReader(runresult.Output).ReadLine() ?? "";
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

GetMsBuildExePath always tries to execute vswhere.exe without checking that the file exists. If VS isn't installed on the test machine, CommandRunner.Run will throw (ProcessStartInfo can't start a missing executable) and the fixture will fail before falling back to the .NET Framework MSBuild path. Guard with File.Exists(vswhereexe) (and only run vswhere when present), otherwise keep the fallback path.

Suggested change
var runresult = CommandRunner.Run(
vswhereexe,
System.Environment.CurrentDirectory,
@" -latest -find MSBuild\**\Bin\MSBuild.exe");
if (runresult.Success)
{
msbuildexe = new System.IO.StringReader(runresult.Output).ReadLine() ?? "";
if (File.Exists(vswhereexe))
{
var runresult = CommandRunner.Run(
vswhereexe,
System.Environment.CurrentDirectory,
@" -latest -find MSBuild\**\Bin\MSBuild.exe");
if (runresult.Success)
{
msbuildexe = new System.IO.StringReader(runresult.Output).ReadLine() ?? "";
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Add assert

}
return msbuildexe;
}
else
{
return "";
}
}

public void Dispose()
{
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\..\src\NuGet.Core\NuGet.Build.Tasks\NuGet.Build.Tasks.csproj" />
<ProjectReference Include="..\..\TestUtilities\Test.Utility\Test.Utility.csproj" />
<ProjectReference Include="..\..\..\src\NuGet.Core\NuGet.Build.Tasks.Pack\NuGet.Build.Tasks.Pack.csproj" />
</ItemGroup>
Expand Down
Loading