Skip to content

Fix mismatch between GetPackOutputItemsTask and PackTask generated filenames#7137

Open
gekka wants to merge 17 commits intoNuGet:devfrom
gekka:fix-nuget-package-file-name
Open

Fix mismatch between GetPackOutputItemsTask and PackTask generated filenames#7137
gekka wants to merge 17 commits intoNuGet:devfrom
gekka:fix-nuget-package-file-name

Conversation

@gekka
Copy link
Copy Markdown

@gekka gekka commented Feb 14, 2026

Bug

Fixes: Nuget/Home#14711
Fixes: Nuget/Home#12644

Description

This pull request fixes a mismatch between the file name that GetPackOutputItemsTask expects for a generated .nupkg and the actual file name produced by PackTask when a project uses NuspecFile or NuspecProperties (including dynamically generated versions).

This PR also fixes an issue with the OutputFileNamesWithoutVersion property, ensuring Pack respects that setting when computing and emitting the final .nupkg file name so downstream targets receive the expected version‑less output.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

Note

One of this tests invoke msbuild to compare the file name computed by GetPackOutputItemsTask with the actual file produced by PackTask. As a result, the NuGet.Build.Tasks.Pack.Test project acquires a new dependency.
I’m not certain whether this test implementation and approach align with the repository’s conventions, so feedback or guidance would be appreciated, especially if anything is incorrect.

@gekka gekka requested a review from a team as a code owner February 14, 2026 03:10
@gekka gekka requested review from kartheekp-ms and zivkan February 14, 2026 03:10
@dotnet-policy-service dotnet-policy-service bot added the Community PRs created by someone not in the NuGet team label Feb 14, 2026
The build error was caused by project dependencies defined in the solution file.
Dependencies were configured at the solution level, Azure Pipelines failed to build the affected project.
@zivkan
Copy link
Copy Markdown
Member

zivkan commented Feb 17, 2026

@gekka I'm conceptually fine with what this PR claims to fix. But I see the CI failures are related to changes made by this PR, so I'm not going to do a full review until we get a green CI.

No problem adding the project reference to the test project.

<GetPackOutputItemsTask
PackageOutputPath="$(PackageOutputAbsolutePath)"
NuspecOutputPath="$(NuspecOutputAbsolutePath)"
NuspecInputFilePath="$(NuspecFile)"
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.

PackTask already takes a bunch of these properties as input.
I think for ease for maintenance, the property names should be the same.

So instead of NuspecInputFilePath, use NuspecFile.

Something else, we should use the NuspecFileAbsolutePath as https://github.com/NuGet/NuGet.Client/blob/3a2648cbab3528f4105e0dedbe60be26289379eb/src/NuGet.Core/NuGet.Build.Tasks/NuGet.Build.Tasks.Pack.targets#L265C29-L265C51 is using it.

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.

Change the property name from NuspecInputFilePath to NuspecFile

$(AbsolutePath) doesn’t exist when GetPackOutputItemsTask is called, I changed the logic to generate it from $(NuspecFile) and pass it in.

Ideally, for maintainability, PackTask should use the output filename produced by GetPackOutputItemsTask directly, which would avoid inconsistencies from generating filenames separately. However, that would require larger changes to PackTask, so this pull request applies the minimal fix.

@gekka gekka changed the title Fix mismatch between GetPackOutputItemsTask and PackTask generated filenames [WIP] Fix mismatch between GetPackOutputItemsTask and PackTask generated filenames Feb 20, 2026
@gekka gekka marked this pull request as draft February 20, 2026 11:46
@gekka
Copy link
Copy Markdown
Author

gekka commented Feb 22, 2026

This PR encountered a few failing tests. A portion of them were due to my code, but there were also intermittent failures that don’t seem to be related.
Since the final rerun passed without issues, it seems that some earlier failures were caused by test instability.

This flakiness timeout failures seem to be coming from NuGet.Protocol.Tests.PackageUpdateResourceTests class.

There’s an existing issue that appears to mention the same type of flakiness.
It’s not the exact same function, but it’s within the same part of the codebase and might be related.

@gekka gekka changed the title [WIP] Fix mismatch between GetPackOutputItemsTask and PackTask generated filenames Fix mismatch between GetPackOutputItemsTask and PackTask generated filenames Feb 22, 2026
@gekka gekka marked this pull request as ready for review February 22, 2026 01:57
@jeffkl jeffkl requested a review from nkolev92 February 24, 2026 23:59
@nkolev92 nkolev92 requested a review from Copilot February 25, 2026 23:30
{
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.

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.

runresultDotnetPack = CommandRunner.Run(
_testFixture._pathMSBuildExe,
testDirectory,
$"/t:Restore;Build;Pack /p:Configuration={CONFIGURATION} /p:UsingMicrosoftNetSdk=true {FILENAME_PROJECT_FILE} ",
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.

restore should not be chained with any other high level target.

It has a special evaluation context and can cause build issues.

We're comfortable just having these tests run against .NET only.

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.

I separated the process into two msbuild calls. Running msbuild.exe without /t:Restore results in a NETSDK1004 error, so the first call performs the Restore target, and the second runs Build and Pack.

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.

I attempted to split the msbuild targets as recommended, and it worked in my local environment.
However, the Azure Pipeline run fails with this approach, so I reverted the change and roll back to /t:Restore;Build;Pack invocation.

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.

We can't/won't accept using that.

We don't want to suggest that's an acceptable thing.

That being said, my suggestion is to just move these tests to the Dotnet.Integration.Test project and just run them there.

Then everything works.

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.

I also tried moving the test to the integration test as suggested, but it did not improve the situation.
When I attempted to run it in Azure Pipeline, it only produced a large number of errors, and the underlying problem remained unchanged.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses inconsistencies in how pack output filenames are computed vs. produced when packing with a .nuspec and/or NuspecProperties, and ensures OutputFileNamesWithoutVersion is respected consistently so downstream MSBuild targets see the expected outputs.

Changes:

  • Update _GetOutputItemsFromPack to pass NuspecFile, NuspecProperties, and OutputFileNamesWithoutVersion into GetPackOutputItemsTask.
  • Align PackTaskLogic/GetPackOutputItemsTask handling of NuspecProperties (notably version) and apply excludeVersion when generating filenames.
  • Add new tests/fixtures (including an MSBuild invocation test) to validate that computed output item names match the files actually produced.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
test/NuGet.Core.Tests/NuGet.Build.Tasks.Pack.Test/PackageNameTests.cs Adds theory-based coverage comparing GetPackOutputItemsTask output vs. produced pack artifacts.
test/NuGet.Core.Tests/NuGet.Build.Tasks.Pack.Test/NuGet.Build.Tasks.Pack.Test.csproj Adds a project reference needed for the new MSBuild-based test setup.
test/NuGet.Core.Tests/NuGet.Build.Tasks.Pack.Test/BuildFixture.cs Introduces a fixture to locate patched dotnet/MSBuild and built task/targets artifacts for integration-style tests.
src/NuGet.Core/NuGet.Build.Tasks/NuGet.Build.Tasks.Pack.targets Passes nuspec inputs and versionless-output option into _GetOutputItemsFromPack’s GetPackOutputItemsTask.
src/NuGet.Core/NuGet.Build.Tasks.Pack/PackTaskLogic.cs Factors nuspec property parsing into a helper and normalizes/validates version.
src/NuGet.Core/NuGet.Build.Tasks.Pack/GetPackOutputItemsTask.cs Uses nuspec inputs to compute output names consistently and respects OutputFileNamesWithoutVersion.

Comment on lines +67 to +75
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

Comment on lines +91 to +97
string[] outputExtensions = GetOutputExtensions(testCase.IncludeSymbols, testCase.SymbolPackageFormat);

var outputItemTask = new NuGet.Build.Tasks.Pack.GetPackOutputItemsTask();
outputItemTask.PackageId = testCase.IdProjProp;
outputItemTask.PackageVersion = testCase.VersionProjProp;
outputItemTask.IncludeSymbols = testCase.IncludeSymbols;
outputItemTask.SymbolPackageFormat = GetSymbolPackageFormatText(testCase.SymbolPackageFormat);
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.

outputExtensions is assigned but never used, which will trigger CS0219. This repo sets TreatWarningsAsErrors=true, so this will fail the build. Remove the local, or use it in an assertion (for example, verifying the expected number of output items).

Suggested change
string[] outputExtensions = GetOutputExtensions(testCase.IncludeSymbols, testCase.SymbolPackageFormat);
var outputItemTask = new NuGet.Build.Tasks.Pack.GetPackOutputItemsTask();
outputItemTask.PackageId = testCase.IdProjProp;
outputItemTask.PackageVersion = testCase.VersionProjProp;
outputItemTask.IncludeSymbols = testCase.IncludeSymbols;
outputItemTask.SymbolPackageFormat = GetSymbolPackageFormatText(testCase.SymbolPackageFormat);
var outputItemTask = new NuGet.Build.Tasks.Pack.GetPackOutputItemsTask();
outputItemTask.PackageId = testCase.IdProjProp;
outputItemTask.PackageVersion = testCase.VersionProjProp;
outputItemTask.IncludeSymbols = testCase.IncludeSymbols;
outputItemTask.SymbolPackageFormat = GetSymbolPackageFormatText(testCase.SymbolPackageFormat);
outputItemTask.SymbolPackageFormat = GetSymbolPackageFormatText(testCase.SymbolPackageFormat);

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.

remove this variable

Comment on lines +163 to +172
var nupkgGneratedFiles = outputExtensions
.SelectMany(outputExtension => Directory.GetFiles(testDirectory, $"*{outputExtension}", SearchOption.AllDirectories))
.Where(line => !line.StartsWith(objFolder))
.Distinct().ToArray();
Assert.Equal(outputExtensions.Length, nupkgGneratedFiles.Length);

foreach (string outputNupkgName in testCase.OutputNupkgNames)
{
var matchCountInFileSystem = GetNameMatchFilePathCount(outputNupkgName, nupkgGneratedFiles);
Assert.True(matchCountInFileSystem == 1, $"{outputNupkgName} is not found in filesystem. [{string.Join(" , ", nupkgGneratedFiles.Select(_ => System.IO.Path.GetFileName(_)))}]");
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.

nupkgGneratedFiles looks like a typo (missing 'e' in "Generated"). Consider renaming to nupkgGeneratedFiles to improve readability and avoid propagating the misspelling into future edits/searches.

Suggested change
var nupkgGneratedFiles = outputExtensions
.SelectMany(outputExtension => Directory.GetFiles(testDirectory, $"*{outputExtension}", SearchOption.AllDirectories))
.Where(line => !line.StartsWith(objFolder))
.Distinct().ToArray();
Assert.Equal(outputExtensions.Length, nupkgGneratedFiles.Length);
foreach (string outputNupkgName in testCase.OutputNupkgNames)
{
var matchCountInFileSystem = GetNameMatchFilePathCount(outputNupkgName, nupkgGneratedFiles);
Assert.True(matchCountInFileSystem == 1, $"{outputNupkgName} is not found in filesystem. [{string.Join(" , ", nupkgGneratedFiles.Select(_ => System.IO.Path.GetFileName(_)))}]");
var nupkgGeneratedFiles = outputExtensions
.SelectMany(outputExtension => Directory.GetFiles(testDirectory, $"*{outputExtension}", SearchOption.AllDirectories))
.Where(line => !line.StartsWith(objFolder))
.Distinct().ToArray();
Assert.Equal(outputExtensions.Length, nupkgGeneratedFiles.Length);
foreach (string outputNupkgName in testCase.OutputNupkgNames)
{
var matchCountInFileSystem = GetNameMatchFilePathCount(outputNupkgName, nupkgGeneratedFiles);
Assert.True(matchCountInFileSystem == 1, $"{outputNupkgName} is not found in filesystem. [{string.Join(" , ", nupkgGeneratedFiles.Select(_ => System.IO.Path.GetFileName(_)))}]");

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 typo

Comment on lines +286 to +331
public record class PackageFileNameTestCase
(string TestNumber
, string[] OutputNupkgNames
, string IdProjProp
, string IdNuspecMeta
, string VersionProjProp
, string VersionNuspecProperties
, string VersionNuspecMeta
, bool UseNuspecFile
, bool OutputFileNamesWithoutVersion = false
, bool IncludeSymbols = false
, NuGet.Commands.SymbolPackageFormat SymbolPackageFormat = Commands.SymbolPackageFormat.Snupkg
) : IXunitSerializable
{


#region IXunitSerializable

[System.Obsolete]
public PackageFileNameTestCase() : this("", [], "", "", "", "", "", false) { }

private const string TestObjectKey = nameof(PackageFileNameTests);
private readonly Newtonsoft.Json.JsonSerializerSettings _settings = new Newtonsoft.Json.JsonSerializerSettings { MaxDepth = null };

void IXunitSerializable.Serialize(IXunitSerializationInfo info)
{
info.AddValue(TestObjectKey, Newtonsoft.Json.JsonConvert.SerializeObject(this));

}
void IXunitSerializable.Deserialize(IXunitSerializationInfo info)
{
var json = (string)info.GetValue(TestObjectKey, typeof(string));
var x = Newtonsoft.Json.JsonConvert.DeserializeObject<PackageFileNameTestCase>(json, _settings);
if (x == null)
{
return;
}

var t = typeof(PackageFileNameTestCase);
var c = t.GetConstructors().Where(_ => _.GetParameters().Length != 0).ToArray()[0];

foreach (System.Reflection.ParameterInfo p in c.GetParameters())
{
var pi = t.GetProperty(p.Name!);
pi!.SetValue(this, pi.GetValue(x));
}
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.

The IXunitSerializable.Deserialize implementation relies on reflection (constructor discovery, GetProperty, SetValue) to mutate the record. This adds fragility and overhead, and it conflicts with the repo guideline to avoid reflection. Prefer a reflection-free approach (e.g., serialize individual fields via info.AddValue/GetValue and assign to mutable properties, or change test data to use only xUnit-serializable primitives and construct the test case inside the test).

Suggested change
public record class PackageFileNameTestCase
(string TestNumber
, string[] OutputNupkgNames
, string IdProjProp
, string IdNuspecMeta
, string VersionProjProp
, string VersionNuspecProperties
, string VersionNuspecMeta
, bool UseNuspecFile
, bool OutputFileNamesWithoutVersion = false
, bool IncludeSymbols = false
, NuGet.Commands.SymbolPackageFormat SymbolPackageFormat = Commands.SymbolPackageFormat.Snupkg
) : IXunitSerializable
{
#region IXunitSerializable
[System.Obsolete]
public PackageFileNameTestCase() : this("", [], "", "", "", "", "", false) { }
private const string TestObjectKey = nameof(PackageFileNameTests);
private readonly Newtonsoft.Json.JsonSerializerSettings _settings = new Newtonsoft.Json.JsonSerializerSettings { MaxDepth = null };
void IXunitSerializable.Serialize(IXunitSerializationInfo info)
{
info.AddValue(TestObjectKey, Newtonsoft.Json.JsonConvert.SerializeObject(this));
}
void IXunitSerializable.Deserialize(IXunitSerializationInfo info)
{
var json = (string)info.GetValue(TestObjectKey, typeof(string));
var x = Newtonsoft.Json.JsonConvert.DeserializeObject<PackageFileNameTestCase>(json, _settings);
if (x == null)
{
return;
}
var t = typeof(PackageFileNameTestCase);
var c = t.GetConstructors().Where(_ => _.GetParameters().Length != 0).ToArray()[0];
foreach (System.Reflection.ParameterInfo p in c.GetParameters())
{
var pi = t.GetProperty(p.Name!);
pi!.SetValue(this, pi.GetValue(x));
}
public record class PackageFileNameTestCase : IXunitSerializable
{
public PackageFileNameTestCase(
string testNumber,
string[] outputNupkgNames,
string idProjProp,
string idNuspecMeta,
string versionProjProp,
string versionNuspecProperties,
string versionNuspecMeta,
bool useNuspecFile,
bool outputFileNamesWithoutVersion = false,
bool includeSymbols = false,
NuGet.Commands.SymbolPackageFormat symbolPackageFormat = Commands.SymbolPackageFormat.Snupkg)
{
TestNumber = testNumber;
OutputNupkgNames = outputNupkgNames;
IdProjProp = idProjProp;
IdNuspecMeta = idNuspecMeta;
VersionProjProp = versionProjProp;
VersionNuspecProperties = versionNuspecProperties;
VersionNuspecMeta = versionNuspecMeta;
UseNuspecFile = useNuspecFile;
OutputFileNamesWithoutVersion = outputFileNamesWithoutVersion;
IncludeSymbols = includeSymbols;
SymbolPackageFormat = symbolPackageFormat;
}
[System.Obsolete]
public PackageFileNameTestCase()
: this(
string.Empty,
System.Array.Empty<string>(),
string.Empty,
string.Empty,
string.Empty,
string.Empty,
string.Empty,
useNuspecFile: false)
{
}
public string TestNumber { get; set; } = string.Empty;
public string[] OutputNupkgNames { get; set; } = System.Array.Empty<string>();
public string IdProjProp { get; set; } = string.Empty;
public string IdNuspecMeta { get; set; } = string.Empty;
public string VersionProjProp { get; set; } = string.Empty;
public string VersionNuspecProperties { get; set; } = string.Empty;
public string VersionNuspecMeta { get; set; } = string.Empty;
public bool UseNuspecFile { get; set; }
public bool OutputFileNamesWithoutVersion { get; set; }
public bool IncludeSymbols { get; set; }
public NuGet.Commands.SymbolPackageFormat SymbolPackageFormat { get; set; } = Commands.SymbolPackageFormat.Snupkg;
#region IXunitSerializable
void IXunitSerializable.Serialize(IXunitSerializationInfo info)
{
info.AddValue(nameof(TestNumber), TestNumber);
info.AddValue(nameof(OutputNupkgNames), OutputNupkgNames);
info.AddValue(nameof(IdProjProp), IdProjProp);
info.AddValue(nameof(IdNuspecMeta), IdNuspecMeta);
info.AddValue(nameof(VersionProjProp), VersionProjProp);
info.AddValue(nameof(VersionNuspecProperties), VersionNuspecProperties);
info.AddValue(nameof(VersionNuspecMeta), VersionNuspecMeta);
info.AddValue(nameof(UseNuspecFile), UseNuspecFile);
info.AddValue(nameof(OutputFileNamesWithoutVersion), OutputFileNamesWithoutVersion);
info.AddValue(nameof(IncludeSymbols), IncludeSymbols);
info.AddValue(nameof(SymbolPackageFormat), SymbolPackageFormat);
}
void IXunitSerializable.Deserialize(IXunitSerializationInfo info)
{
TestNumber = (string)info.GetValue(nameof(TestNumber), typeof(string));
OutputNupkgNames = (string[])info.GetValue(nameof(OutputNupkgNames), typeof(string[]));
IdProjProp = (string)info.GetValue(nameof(IdProjProp), typeof(string));
IdNuspecMeta = (string)info.GetValue(nameof(IdNuspecMeta), typeof(string));
VersionProjProp = (string)info.GetValue(nameof(VersionProjProp), typeof(string));
VersionNuspecProperties = (string)info.GetValue(nameof(VersionNuspecProperties), typeof(string));
VersionNuspecMeta = (string)info.GetValue(nameof(VersionNuspecMeta), typeof(string));
UseNuspecFile = (bool)info.GetValue(nameof(UseNuspecFile), typeof(bool));
OutputFileNamesWithoutVersion = (bool)info.GetValue(nameof(OutputFileNamesWithoutVersion), typeof(bool));
IncludeSymbols = (bool)info.GetValue(nameof(IncludeSymbols), typeof(bool));
SymbolPackageFormat = (NuGet.Commands.SymbolPackageFormat)info.GetValue(
nameof(SymbolPackageFormat),
typeof(NuGet.Commands.SymbolPackageFormat));

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.

Updated the code exactly as suggested.

_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

Comment on lines +71 to +74
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.

Comment on lines +125 to +131
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

@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Mar 4, 2026
@dotnet-policy-service dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Mar 8, 2026
@jeffkl jeffkl assigned nkolev92 and unassigned zivkan Mar 17, 2026
@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Mar 25, 2026
@dotnet-policy-service dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community PRs created by someone not in the NuGet team

Projects

None yet

4 participants