Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public static LockFileValidationResult IsLockFileValid(DependencyGraphSpec dgSpe

if (hasChanged)
{
// P2P transitive package dependencies have changed
// P2P transitive package dependencies have changed
invalidReasons.Add(message);
}

Expand All @@ -265,12 +265,20 @@ public static LockFileValidationResult IsLockFileValid(DependencyGraphSpec dgSpe
}
else
{
invalidReasons.Add(string.Format(
CultureInfo.CurrentCulture,
Strings.PackagesLockFile_ProjectReferenceHasNoCompatibleTargetFramework,
p2pProjectName,
useAliasForMessages ? restoreMetadataFramework.TargetAlias : restoreMetadataFramework.FrameworkName.GetShortFolderName()
));
// When no compatible TFM is found, restore contributes no transitive dependencies
// for this P2P project. Non-empty deps in the lock file indicate a stale entry
// from when the P2P previously had a compatible TFM.
if (projectDependency.Dependencies.Count > 0)
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.

Hey @PHILLIPS71

Thanks for creating this PR.

I'm not sure I agree with your assessment that this is a false positive, but I'll check with some of my teammates as well.

I think it's fine if NU1004 is raised here.
The project dependencies have changed and that's all NU1004 is supposed to change.

Moreover, I'm not sure this actually tackles the problem in the linked issue, which is really about AssetTargetFallback not being applied through project refs when calculating.

{
invalidReasons.Add(
string.Format(
CultureInfo.CurrentCulture,
Strings.PackagesLockFile_ProjectReferenceHasNoCompatibleTargetFramework,
p2pProjectName,
useAliasForMessages ? restoreMetadataFramework.TargetAlias : restoreMetadataFramework.FrameworkName.GetShortFolderName()
)
);
}
}
}
else // This can't happen. When adding the queue, the referenceSpec HAS to be discovered. If the project is otherwise missing, it will be discovered in HasP2PDependencyChanged
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8531,8 +8531,8 @@ public void RestoreNetCore_PackagesLockFile_WithDependentProjectChangeOfNotCompa
string[] initialFrameworks,
string[] updatedFrameworks)
{
// A project with RestoreLockedMode should fail restore if the frameworks of a dependent project were changed
// with incompatible frameworks between restores.
// In locked mode, restore should fail when a dependent project's frameworks change to be incompatible.
// B has no packages, so the lock file is still valid (0 deps), and failure comes from NU1201 not NU1004.
using (var pathContext = new SimpleTestPathContext())
{
// Set up solution, project, and packages
Expand Down Expand Up @@ -8574,9 +8574,9 @@ public void RestoreNetCore_PackagesLockFile_WithDependentProjectChangeOfNotCompa

// Assert
r.Success.Should().BeFalse();
Assert.Contains("NU1004", r.Errors);
Assert.Contains("NU1201", r.Errors);
var logCodes = projectA.AssetsFile.LogMessages.Select(e => e.Code);
Assert.Contains(NuGetLogCode.NU1004, logCodes);
Assert.Contains(NuGetLogCode.NU1201, logCodes);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -512,14 +512,13 @@ await SimpleTestPackageUtility.CreateFolderFeedV3Async(
}

[Fact]
public async Task RestoreCommand_PackagesLockFile_InLockedMode_WhenANewDirectProjectReferenceChangesFramework_FailsWithNU1004()
public async Task RestoreCommand_PackagesLockFile_InLockedMode_WhenANewDirectProjectReferenceChangesFramework_FailsWithNU1201()
{
// Arrange
using (var pathContext = new SimpleTestPathContext())
{
var logger = new TestLogger();
var packageA = new SimpleTestPackageContext("a", "1.0.0");
var packageB = new SimpleTestPackageContext("b", "1.0.0");

await SimpleTestPackageUtility.CreateFolderFeedV3Async(
pathContext.PackageSource,
Expand Down Expand Up @@ -556,29 +555,30 @@ await SimpleTestPackageUtility.CreateFolderFeedV3Async(
restoreLockedMode: true);
logger.Clear();

// Change the P2P to an incompatible framework.
PackageSpecOperationsUtility.RemoveTargetFramework(projectReferenceSpec, "net46");
PackageSpecOperationsUtility.AddTargetFramework(projectReferenceSpec, "net47");

// Act.
result = await new RestoreCommand(ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, rootPackageSpec, projectReferenceSpec)).ExecuteAsync();

// Assert.
// Assert: the lock file remains valid (no transitive dep change), so failure is NU1201,
// not NU1004.
result.Success.Should().BeFalse();
logger.ErrorMessages.Count.Should().Be(1);
logger.ErrorMessages.Single().Should().Contain("NU1004");
logger.ErrorMessages.Single().Should().Contain($"The project IntermediateProject has no compatible target framework.");
logger.ErrorMessages.Single().Should().Contain("NU1201");
logger.ErrorMessages.Single().Should().Contain("Project IntermediateProject is not compatible");
}
}

[Fact]
public async Task RestoreCommand_PackagesLockFile_InLockedMode_WhenANewTransitiveProjectReferenceChangesFramework_FailsWithNU1004()
public async Task RestoreCommand_PackagesLockFile_InLockedMode_WhenANewTransitiveProjectReferenceChangesFramework_FailsWithNU1201()
{
// Arrange
using (var pathContext = new SimpleTestPathContext())
{
var logger = new TestLogger();
var packageA = new SimpleTestPackageContext("a", "1.0.0");
var packageB = new SimpleTestPackageContext("b", "1.0.0");

await SimpleTestPackageUtility.CreateFolderFeedV3Async(
pathContext.PackageSource,
Expand Down Expand Up @@ -622,17 +622,19 @@ await SimpleTestPackageUtility.CreateFolderFeedV3Async(
restoreLockedMode: true);
logger.Clear();

// Change the leaf P2P to an incompatible framework.
PackageSpecOperationsUtility.RemoveTargetFramework(leafProjectReferenceSpec, "net46");
PackageSpecOperationsUtility.AddTargetFramework(leafProjectReferenceSpec, "net47");

// Act.
result = await new RestoreCommand(ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, rootPackageSpec, intermediateProjectReferenceSpec, leafProjectReferenceSpec)).ExecuteAsync();

// Assert.
// Assert: the lock file remains valid (no transitive dep change), so failure is NU1201,
// not NU1004.
result.Success.Should().BeFalse();
logger.ErrorMessages.Count.Should().Be(1);
logger.ErrorMessages.Single().Should().Contain("NU1004");
logger.ErrorMessages.Single().Should().Contain($"The project LeafProject has no compatible target framework.");
logger.ErrorMessages.Single().Should().Contain("NU1201");
logger.ErrorMessages.Single().Should().Contain("Project LeafProject is not compatible");
}
}

Expand Down Expand Up @@ -1678,5 +1680,60 @@ public async Task RestoreCommand_PackagesLockFile_InLockedMode_WithAliasedFramew
logger.ErrorMessages.Single().Should().Contain("NU1004");
logger.ErrorMessages.Single().Should().Contain("The project reference project2 has changed");
}

[Fact]
public async Task RestoreCommand_PackagesLockFile_InLockedMode_WhenP2PProjectHasNoCompatibleTargetFramework_Succeeds()
{
// Arrange
// Root uses an AssetTargetFallback (ATF) framework. IsLockFileValid does not expand ATF
// when checking P2P compatibility, so the lock file sees the P2P as contributing 0 deps —
// which is valid with this fix. The actual restore does expand ATF and succeeds.
using (var pathContext = new SimpleTestPathContext())
{
var logger = new TestLogger();

var rootProjectName = "RootProject";
var rootProjectDirectory = Path.Combine(pathContext.SolutionRoot, rootProjectName);

var atfFramework = new AssetTargetFallbackFramework(
CommonFrameworks.NetStandard20,
new List<NuGetFramework> { CommonFrameworks.Net46 });
var rootTFI = new TargetFrameworkInformation { FrameworkName = atfFramework, TargetAlias = "netstandard2.0" };

var rootPackageSpec = PackageReferenceSpecBuilder.Create(rootProjectName, rootProjectDirectory)
.WithTargetFrameworks(new[] { rootTFI })
.WithPackagesLockFile()
.Build();

var p2pProjectName = "P2PProject";
var p2pProjectDirectory = Path.Combine(pathContext.SolutionRoot, p2pProjectName);

// P2P targets net46, which is only reachable from the root via ATF fallback.
var p2pPackageSpec = PackageReferenceSpecBuilder.Create(p2pProjectName, p2pProjectDirectory)
.WithTargetFrameworks(["net46"])
.Build();

PackageSpecOperationsUtility.AddProjectReference(rootPackageSpec, p2pPackageSpec, atfFramework);

// Preconditions: initial restore uses ATF expansion to resolve the P2P.
var result = await new RestoreCommand(ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, rootPackageSpec, p2pPackageSpec)).ExecuteAsync();
await result.CommitAsync(logger, CancellationToken.None);
result.Success.Should().BeTrue();

// Enable locked mode and clear the logger.
rootPackageSpec.RestoreMetadata.RestoreLockProperties = new RestoreLockProperties(
restorePackagesWithLockFile: "true",
rootPackageSpec.RestoreMetadata.RestoreLockProperties.NuGetLockFilePath,
restoreLockedMode: true);
logger.Clear();

// Act
result = await new RestoreCommand(ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, rootPackageSpec, p2pPackageSpec)).ExecuteAsync();

// Assert: the lock file is valid (P2P contributes 0 deps, which is correct), so locked restore succeeds.
result.Success.Should().BeTrue();
logger.ErrorMessages.Should().BeEmpty();
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ public void IsLockFileStillValid_TransitiveVersionsMovedToCentralFile_Invalidate
.WithType(PackageDependencyType.Transitive)))
.Build();

// The central package version cpvm2 has was changed from transitive to central
// The central package version cpvm2 has was changed from transitive to central
var actual = PackagesLockFileUtilities.IsLockFileValid(dgSpec, lockFile);
Assert.False(actual.IsValid);
Assert.Contains("Transitive dependency cpvm2 moved to be centraly managed invalidated the lock file.", actual.InvalidReasons);
Expand Down Expand Up @@ -932,7 +932,7 @@ public void IsLockFileStillValid_WithProjectToProjectWithPrivateAssets_IgnoresSu
var projectB = ProjectTestHelpers.GetPackageSpec("B", framework: frameworkShortName).WithTestRestoreMetadata();
var projectC = ProjectTestHelpers.GetPackageSpec("C", framework: frameworkShortName).WithTestRestoreMetadata();

// B (PrivateAssets.All) -> C
// B (PrivateAssets.All) -> C
projectB = projectB.WithTestProjectReference(projectC, privateAssets: LibraryIncludeFlags.All);

// A -> B
Expand Down Expand Up @@ -982,7 +982,7 @@ public void IsLockFileStillValid_WithProjectToProject_MultipleEdgesWithDifferent
aliases: null,
versionOverride: null);

// B (PrivateAssets.All) -> C
// B (PrivateAssets.All) -> C
projectB = projectB.WithTestProjectReference(projectC, privateAssets: LibraryIncludeFlags.All);

// A -> B
Expand Down Expand Up @@ -1056,7 +1056,7 @@ public void IsLockFileStillValid_WithProjectToProjectPackagesConfig_IncludesAllD
aliases: null,
versionOverride: null);

// B -> C
// B -> C
projectB = projectB.WithTestProjectReference(projectC);

// A -> B
Expand Down Expand Up @@ -1162,5 +1162,74 @@ public void IsLockFileValid_WithNullVersionRange_DoesNotThrow()
var invalidReason = actual.InvalidReasons.Single();
Assert.Contains("PackageA", invalidReason);
}

/// <summary>
/// A(PR) -> B(PR), where A targets netstandard2.0 and B targets net46 (incompatible frameworks).
/// The lock file records B with no deps, consistent with what restore produces for an incompatible TFM.
/// Regression for https://github.com/NuGet/Home/issues/12010
/// </summary>
[Fact]
public void IsLockFileValid_WithPackageReferenceP2PAndIncompatibleFramework_WithNoDepsInLockFile_IsValid()
{
// Arrange
var parentFramework = CommonFrameworks.NetStandard20;
var projectA = ProjectTestHelpers.GetPackageSpec("A", framework: parentFramework.GetShortFolderName()).WithTestRestoreMetadata();
var projectB = ProjectTestHelpers.GetPackageSpec("B", framework: "net46").WithTestRestoreMetadata();

// A -> B
projectA = projectA.WithTestProjectReference(projectB);
var dgSpec = ProjectTestHelpers.GetDGSpecForFirstProject(projectA, projectB);

var lockFile = new PackagesLockFileBuilder()
.WithTarget(target => target
.WithFramework(parentFramework)
.WithDependency(dep => dep
.WithId("B")
.WithType(PackageDependencyType.Project)
.WithRequestedVersion(VersionRange.Parse("1.0.0"))))
.Build();

// Act
var actual = PackagesLockFileUtilities.IsLockFileValid(dgSpec, lockFile);

// Assert
actual.IsValid.Should().BeTrue();
actual.InvalidReasons.Should().BeEmpty();
}

/// <summary>
/// A(PR) -> B(PR), where A targets netstandard2.0 and B targets net46 (incompatible frameworks).
/// A lock file with non-empty deps for an incompatible-TFM P2P is stale and must be detected as invalid.
/// </summary>
[Fact]
public void IsLockFileValid_WithPackageReferenceP2PAndIncompatibleFramework_WithNonEmptyDepsInLockFile_IsInvalid()
{
// Arrange
var parentFramework = CommonFrameworks.NetStandard20;
var projectA = ProjectTestHelpers.GetPackageSpec("A", framework: parentFramework.GetShortFolderName()).WithTestRestoreMetadata();
var projectB = ProjectTestHelpers.GetPackageSpec("B", framework: "net46").WithTestRestoreMetadata();

// A -> B
projectA = projectA.WithTestProjectReference(projectB);
var dgSpec = ProjectTestHelpers.GetDGSpecForFirstProject(projectA, projectB);

// B has a stale transitive dependency from when it previously had a compatible TFM.
var lockFile = new PackagesLockFileBuilder()
.WithTarget(target => target
.WithFramework(parentFramework)
.WithDependency(dep => dep
.WithId("B")
.WithType(PackageDependencyType.Project)
.WithRequestedVersion(VersionRange.Parse("1.0.0"))
.WithDependency(new PackageDependency("SomePackage", VersionRange.Parse("1.0.0")))))
.Build();

// Act
var actual = PackagesLockFileUtilities.IsLockFileValid(dgSpec, lockFile);

// Assert
actual.IsValid.Should().BeFalse();
actual.InvalidReasons.Should().ContainSingle().Which.Should().Contain("The project B has no compatible target framework");
}
}
}