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 back NuGetSourceType=Package metadata in package resolution #2133

Merged
merged 1 commit into from
Apr 16, 2018

Conversation

nguerrera
Copy link
Contributor

@nguerrera nguerrera commented Apr 10, 2018

This was used in a few places to distinguish between refs coming from NuGet from others.

In particular, it caused all nuget refs to be copied to refs/folder on Build with PreserveCompilationContext=true.

Fix #2121

This was used in a few places to distinguish between refs coming from NuGet
from others. In particular, it caused all nuget refs to be copied to refs/
folder on Build with PreserverCompilationContext=true.
@nguerrera nguerrera requested review from dsplaisted and a team April 10, 2018 23:14
@nguerrera nguerrera changed the base branch from master to release/2.1.3xx April 10, 2018 23:14
@@ -251,6 +251,7 @@ private void SetImplicitMetadataForCompileTimeAssemblies()

foreach (var item in CompileTimeAssemblies)
{
item.SetMetadata(MetadataKeys.NuGetSourceType, "Package");
Copy link
Member

Choose a reason for hiding this comment

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

If we control all the consumers of this metadata, I'd prefer we just update them to check if there is a package ID set.

However, this change is also fine if you think it's better based on risk, churn, etc.

@nguerrera
Copy link
Contributor Author

If [...], I'd prefer we just update them to check if there is a package ID set.

I've discovered that there are other refs with NuGetPackageId but no NuGetSourceType:

https://github.com/dotnet/standard/blob/9a99f024343e6a36d15e7073890c07daa81606fc/netstandard/pkg/targets/netstandard/NETStandard.Library.targets#L25

I don't know yet if it would be a fix or a bug to start treating them as we do the ones that come from assets file. I suspect a fix. cc @ericstj @eerhardt

@ericstj
Copy link
Member

ericstj commented Apr 11, 2018

The thing that NETStandard.Library was mimicking is the NuGet restore task that predates the SDK/NuGet's reimplementation. It also sets only NuGetPackageId and NuGetPackageVersion, but not NuGetSourceType: https://github.com/NuGet/NuGet.BuildTasks/blob/8316e6a9ffcec0c0f8ae6a99f1f39bac5417b4e3/src/Microsoft.NuGet.Build.Tasks/ResolveNuGetPackageAssets.cs#L821-L822

@eerhardt
Copy link
Member

@ericstj
Copy link
Member

ericstj commented Apr 11, 2018 via email

@nguerrera
Copy link
Contributor Author

@dotnet-bot Test OSX10.12 Release

(machine was out of disk space)

@nguerrera nguerrera changed the base branch from release/2.1.3xx to master April 11, 2018 22:54
@nguerrera
Copy link
Contributor Author

In the links above there is hint of NuGetSourceType being something other than Package. Project seems to be another value. What does this mean? I cannot find anywhere we ever set anything other than Package.

@pranavkm
Copy link
Contributor

@dotnet-bot Test OSX10.12 Release

@nguerrera nguerrera changed the base branch from master to release/2.1.3xx April 16, 2018 20:01
@nguerrera nguerrera added this to the 2.1.3xx milestone Apr 16, 2018
@nguerrera nguerrera merged commit 1298f16 into dotnet:release/2.1.3xx Apr 16, 2018
@nguerrera nguerrera deleted the nuget-source-type branch April 16, 2018 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants