Skip to content

Commit

Permalink
Fix package reference pack when referenced directly and indirectly
Browse files Browse the repository at this point in the history 
Because of the way we were constructing the `_PrivateAssets` and `_ShouldPack` properties, we were ending with duplicates (i.e. `all;all`) if the package reference was both direct and indirectly included, since the previous item transform would basically concatenate all such values, breaking the condition in the subsequent item group.

Rather than using an item transformation, we now instead just reference the item metadata directly, which will result in MSBuild iterating all items and assigning the property multiple times, once for each item. The last such item will determine the final property value. Therefore, we invert the Implicit references so they are processed first, so that an explicit `PackageReference.PrivateAssets` or `Pack` metadata trumps implicitly defined reference values.

A new test ensures this is the case (it would fail without the fix) with the Shell.Interop VSSDK package.

Fixes #28
  • Loading branch information
@kzu
kzu committed Nov 19, 2020
1 parent 6be6012 commit 2754678
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 27 deletions.
59 changes: 32 additions & 27 deletions src/NuGetizer.Tasks/NuGetizer.Inference.targets
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<DefaultPackFolder>content</DefaultPackFolder>
<Pack Condition="'$(PackAndroidResource)' == true">true</Pack>
<BuildAction>AndroidResource</BuildAction>
</AndroidResource>
</AndroidResource>
<BundleResource>
<DefaultPackFolder>content</DefaultPackFolder>
<Pack Condition="'$(PackBundleResource)' == true">true</Pack>
Expand All @@ -117,12 +117,17 @@ Copyright (c) .NET Foundation. All rights reserved.
<TargetPath />
</InferenceCandidate>
</ItemDefinitionGroup>

<!-- Extend some built-in items with metadata we use in our inference targets -->
<ItemDefinitionGroup>
<PackageReference>
<Pack />
<PrivateAssets />
</PackageReference>
<ImplicitPackageReference>
<Pack />
<PrivateAssets />
</ImplicitPackageReference>
<ReferencePath>
<Facade>false</Facade>
<FrameworkFile>false</FrameworkFile>
Expand Down Expand Up @@ -158,7 +163,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<BuildOutputFrameworkSpecific>@(_BuildOutputFrameworkSpecific)</BuildOutputFrameworkSpecific>
</PropertyGroup>
</Target>

<Target Name="_SetDefaultPackageReferencePack" Condition="'$(PackFolder)' == 'build'"
BeforeTargets="InferPrimaryOutputDependencies;InferPackageContents">
<ItemGroup>
Expand All @@ -167,7 +172,7 @@ Copyright (c) .NET Foundation. All rights reserved.
Pack="false" />
</ItemGroup>
</Target>

<Target Name="InferPackageContents" DependsOnTargets="$(InferPackageContentsDependsOn)" Returns="@(PackageFile)">
<ItemGroup>
<InferenceCandidate Include="@(%(PackInference.Identity))" Exclude="@(%(PackInference.Identity) -> '%(PackExclude)')"/>
Expand All @@ -191,10 +196,10 @@ Copyright (c) .NET Foundation. All rights reserved.
'%(_InferenceCandidateWithTargetPath.CopyToOutputDirectory)' != 'Never'">$(PackFolder)</PackFolder>
<!-- Otherwise they cake on whichever is the default for their item type, as defined by their PackInference item -->
<PackFolder Condition="'%(_InferenceCandidateWithTargetPath.CopyToOutputDirectory)' == '' or
'%(_InferenceCandidateWithTargetPath.CopyToOutputDirectory)' == 'Never'">%(_InferenceCandidateWithTargetPath.DefaultPackFolder)</PackFolder>
'%(_InferenceCandidateWithTargetPath.CopyToOutputDirectory)' == 'Never'">%(_InferenceCandidateWithTargetPath.DefaultPackFolder)</PackFolder>
</_InferenceCandidateWithTargetPath>
<!-- Items that are copied to the output directory are included from the target path -->

<!-- Items that are copied to the output directory are included from the target path -->
<_InferredPackageFile Include="@(_InferenceCandidateWithTargetPath -> '$(_OutputFullPath)\%(TargetPath)')"
Condition="'%(_InferenceCandidateWithTargetPath.CopyToOutputDirectory)' != '' and
'%(_InferenceCandidateWithTargetPath.CopyToOutputDirectory)' != 'Never'" />
Expand All @@ -205,12 +210,12 @@ Copyright (c) .NET Foundation. All rights reserved.
</ItemGroup>

<ItemGroup Label="BuildOutput Inference" Condition="'$(PackBuildOutput)' == 'true'">
<!-- Unfortunately, even with https://github.com/Microsoft/msbuild/pull/1115, when multi-targeting
<!-- Unfortunately, even with https://github.com/Microsoft/msbuild/pull/1115, when multi-targeting
.NETFramework, the desktop WinFX.targets are imported which don't have the fix, so we need to
do it "the old way" for this particular output group -->
<_SatelliteDllsProjectOutputGroupOutput Include="@(SatelliteDllsProjectOutputGroupOutput)"
<_SatelliteDllsProjectOutputGroupOutput Include="@(SatelliteDllsProjectOutputGroupOutput)"
FinalOutputPath="%(FullPath)" />

<_InferredProjectOutput Include="@(BuiltProjectOutputGroupOutput -> '%(FinalOutputPath)');
@(BuiltProjectOutputGroupKeyOutput -> '%(FinalOutputPath)');
@(DocumentationProjectOutputGroupOutput -> '%(FinalOutputPath)');
Expand Down Expand Up @@ -263,9 +268,9 @@ Copyright (c) .NET Foundation. All rights reserved.
</ItemGroup>
</Target>

<Target Name="_UpdatePackageReferenceVersions"
Inputs="@(PackageReference)"
Outputs="|%(Identity)|"
<Target Name="_UpdatePackageReferenceVersions"
Inputs="@(PackageReference)"
Outputs="|%(Identity)|"
Condition="'$(UsingMicrosoftNETSdk)' == 'true' and '$(ManagePackageVersionsCentrally)' == 'true'"
DependsOnTargets="RunResolvePackageDependencies"
Returns="@(_CentrallyManagedDependency)">
Expand All @@ -277,7 +282,7 @@ Copyright (c) .NET Foundation. All rights reserved.
</PropertyGroup>

<ItemGroup Condition="'$(_CandidatePackageVersion)' == '' and '$(_CandidatePackageIsImplicit)' != 'true'">
<_CentrallyManagedDependency Include="@(PackageDependencies)"
<_CentrallyManagedDependency Include="@(PackageDependencies)"
Condition="$([MSBuild]::ValueOrDefault('%(Identity)', '').StartsWith('$(_CandidatePackageId)/')) and
$([MSBuild]::ValueOrDefault('%(ParentPackage)', '')) == ''" />
</ItemGroup>
Expand All @@ -286,13 +291,13 @@ Copyright (c) .NET Foundation. All rights reserved.
<_CentrallyManagedDependency>%(_CentrallyManagedDependency.Identity)</_CentrallyManagedDependency>
<_CentrallyManagedVersion>$(_CentrallyManagedDependency.Replace('$(_CandidatePackageId)/', ''))</_CentrallyManagedVersion>
</PropertyGroup>

<ItemGroup Condition="'$(_CentrallyManagedVersion)' != ''">
<PackageReference Update="@(PackageReference)" Version="$(_CentrallyManagedVersion)" />
</ItemGroup>

</Target>

<Target Name="_CollectPrimaryOutputDependencies" DependsOnTargets="BuildOnlySettings;ResolveReferences" Returns="@(ImplicitPackageReference)">
<Error Code="NG1003" Text="Centrally managed package versions is only supported when using the Microsoft.NET.Sdk."
Condition="'$(ManagePackageVersionsCentrally)' == 'true' and '$(UsingMicrosoftNETSdk)' != 'true'" />
Expand All @@ -305,34 +310,34 @@ Copyright (c) .NET Foundation. All rights reserved.
<_PrivateAssetsPackageReference Include="@(PackageReference -> WithMetadataValue('PrivateAssets', 'all'))"
Condition="'%(PackageReference.IsImplicitlyDefined)' != 'true' and '%(PackageReference.Pack)' != 'false'"/>
</ItemGroup>
<InferImplicitPackageReference Condition="'@(_PrivateAssetsPackageReference)' != '' and '@(PackageDependencies)' != ''"
PackageReferences="@(_PrivateAssetsPackageReference)"
<InferImplicitPackageReference Condition="'@(_PrivateAssetsPackageReference)' != '' and '@(PackageDependencies)' != ''"
PackageReferences="@(_PrivateAssetsPackageReference)"
PackageDependencies="@(PackageDependencies)">
<Output TaskParameter="ImplicitPackageReferences" ItemName="ImplicitPackageReference" />
</InferImplicitPackageReference>
</Target>

<Target Name="_ResolvePackageDependencies" Condition="'$(UsingMicrosoftNETSdk)' == 'true'" DependsOnTargets="RunResolvePackageDependencies" />

<Target Name="InferPrimaryOutputDependencies"
Inputs="@(_PrimaryOutputRelatedFile)"
Outputs="%(_PrimaryOutputRelatedFile.NuGetPackageId)"
Returns="@(_InferredPackageFile)"
DependsOnTargets="_ResolvePackageDependencies;_CollectPrimaryOutputDependencies">

<ItemGroup>
<_NuGetPackageId Include="@(_PrimaryOutputRelatedFile -> '%(NuGetPackageId)')" Condition="'%(NuGetPackageId)' != 'NETStandard.Library'" />
</ItemGroup>
<PropertyGroup>
<_NuGetPackageId>@(_NuGetPackageId -> Distinct())</_NuGetPackageId>
</PropertyGroup>
<ItemGroup>
<_PrimaryPackageReference Include="@(PackageReference);@(ImplicitPackageReference)" Condition="'$(_NuGetPackageId)' != '' and '%(Identity)' == '$(_NuGetPackageId)'" />
<_PrimaryPackageReference Include="@(ImplicitPackageReference);@(PackageReference)" Condition="'$(_NuGetPackageId)' != '' and '%(Identity)' == '$(_NuGetPackageId)'" />
</ItemGroup>

<PropertyGroup>
<_PrivateAssets>@(_PrimaryPackageReference -> '%(PrivateAssets)')</_PrivateAssets>
<_ShouldPack>@(_PrimaryPackageReference -> '%(Pack)')</_ShouldPack>
<_PrivateAssets>%(_PrimaryPackageReference.PrivateAssets)</_PrivateAssets>
<_ShouldPack>%(_PrimaryPackageReference.Pack)</_ShouldPack>
<_ShouldIncludeAssetsRegex>$(_NuGetPackageId)\\.+\\$(_PrivateAssets)\\.*</_ShouldIncludeAssetsRegex>
</PropertyGroup>

Expand All @@ -347,7 +352,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- In this case, we only add files that have a matching path to the private assets value.
i.e. for Mono.Options, PrivateAssets=lib, we'll include the file if its full path contains
'Mono.Options\*\lib\*', meaning the file is a lib. -->
<_InferredPackageFile Include="@(_PrimaryOutputRelatedFile)"
<_InferredPackageFile Include="@(_PrimaryOutputRelatedFile)"
Condition="$([System.Text.RegularExpressions.Regex]::IsMatch('%(_PrimaryOutputRelatedFile.FullPath)', '$(_ShouldIncludeAssetsRegex)', 'RegexOptions.IgnoreCase')) == 'true'">
<PackFolder>$(PackFolder)</PackFolder>
<FrameworkSpecific>$(BuildOutputFrameworkSpecific)</FrameworkSpecific>
Expand Down Expand Up @@ -376,7 +381,7 @@ Copyright (c) .NET Foundation. All rights reserved.
'$(ManagePackageVersionsCentrally)' == 'true'">
$(InferPackageContentsDependsOn);
_UpdatePackageReferenceVersions
</InferPackageContentsDependsOn>
</InferPackageContentsDependsOn>
<InferPackageContentsDependsOn>
$(InferPackageContentsDependsOn);
GetPackageTargetPath
Expand Down
24 changes: 24 additions & 0 deletions src/NuGetizer.Tests/given_packinference.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,5 +540,29 @@ public void when_adding_new_inference_then_can_change_defaults()
Extension = ".sql",
}));
}

[Fact]
public void when_direct_and_indirect_packagereference_then_packs_once()
{
var result = Builder.BuildProject(@"
<Project Sdk='Microsoft.NET.Sdk'>
<PropertyGroup>
<PackageId>Library</PackageId>
<TargetFramework>net472</TargetFramework>
</PropertyGroup>
<ItemGroup>
<PackageReference Include='Microsoft.VisualStudio.Shell.Interop' Version='16.7.30328.74' PrivateAssets='all' />
<PackageReference Include='Microsoft.VisualStudio.Shell.Interop.12.0' Version='16.7.30328.74' PrivateAssets='all' />
</ItemGroup>
</Project>",
"GetPackageContents", output);

result.AssertSuccess(output);
Assert.Contains(result.Items, item => item.Matches(new
{
Filename = "Microsoft.VisualStudio.Shell.Interop",
Extension = ".dll",
}));
}
}
}

0 comments on commit 2754678

Please sign in to comment.