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.

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

Add package id and version to Reference and ReferenceCopyLocalPaths Items #603

Merged
merged 4 commits into from
Mar 1, 2017

Conversation

natidea
Copy link
Contributor

@natidea natidea commented Jan 7, 2017

Fixes #567

This adds NuGetPackageId and NuGetPackageVersion metadata to Reference and ReferenceCopyLocalPaths Items. These are used downstream by a few tasks.

This leads to additional metadata on a few definitions from the ResolvePackageDependencies and ProduceContentAssets tasks, which matches the old ResolveNuGetPackageAssets task, but I'm not sure if there are significant MSBuild performance implications.

Also not sure if we would take this for escrow as these are not .NET Core scenarios, but making this solution available to unblock downstream work.

/cc @ericstj @srivatsn @nguerrera @dsplaisted

{
throw new BuildErrorException(Strings.ContentFileDoesNotContainExpectedParentPackageInformation, contentFile.ItemSpec);
}
string packageName = parts[0], packageVersion = parts[1];
Copy link
Member

Choose a reason for hiding this comment

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

(nit) I think it is uncommon to make assignments like this in our current code base. Can these be split into 2 lines?

@@ -296,6 +298,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<Private>false</Private>
<NuGetIsFrameworkReference>true</NuGetIsFrameworkReference>
<NuGetSourceType>Package</NuGetSourceType>
<NuGetPackageId>%(_FrameworkAssemblies.PackageName)</NuGetPackageId>
Copy link
Member

Choose a reason for hiding this comment

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

Question for my understanding (no change necessary): Why the difference in metadata names here? NuGetPackageXXX vs. just PackageXXX? Why is one prefixed with NuGet and the other isn't?

@eerhardt
Copy link
Member

eerhardt commented Jan 9, 2017

Also not sure if we would take this for escrow as these are not .NET Core scenarios

It is my understanding that this change is needed for .NET Core 2.0 scenarios.

@@ -280,6 +280,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<Private>false</Private>
Copy link
Member

Choose a reason for hiding this comment

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

existing: why use batching here instead of a transform? If you use @(_CompileFileDefinitions->'%(ResolvedPath)') it will preserve all the meta-data and you don't need to do things like

 +        <NuGetPackageId>%(_CompileFileDefinitions.PackageName)</NuGetPackageId>
 +        <NuGetPackageVersion>%(_CompileFileDefinitions.PackageVersion)</NuGetPackageVersion>

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Let me know when this goes in. We have a dependency on it.

@srivatsn
Copy link
Contributor

Tagging @MattGertz for RTM approval.

@srivatsn srivatsn added this to the 1.0 RTM milestone Jan 23, 2017
@MattGertz
Copy link

Needs template -- I'm not clear on this v.v. the Escrow bar.

@srivatsn srivatsn removed this from the 1.0 RTM milestone Feb 15, 2017
@natidea natidea changed the base branch from master to future February 16, 2017 05:59
@nguerrera
Copy link
Contributor

Retarget to master after #917 goes in

@333fred
Copy link
Member

333fred commented Feb 27, 2017

@natidea, #917 is in. Please retarget.

@natidea natidea changed the base branch from future to master February 27, 2017 18:05
@natidea
Copy link
Contributor Author

natidea commented Feb 28, 2017

/cc @nguerrera @dsplaisted for one more review before I merge

@natidea natidea merged commit b76e22d into dotnet:master Mar 1, 2017
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…ableProject

Set WarnOnPackingNonPackableProject to true when IsPackable is false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants