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

Implicitly add PackageReference for NETStandard.Library if targeting .NET Standard #450

Merged

Conversation

@dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented Dec 2, 2016

This PR will implicitly add a PackageReference to NETStandard.Library if the project is targeting .NETStandard and there isn't already a PackageReference to NETStandard.Library. This makes project files more succinct and avoids confusion about why there are different versions of .NET Standard in a project file: for example "netstandard1.4" in the TargetFramework property but version "1.6.0" in the PackageReference.

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Dec 2, 2016

@davidfowl @ericstj @terrajobst In addition to making the NETStandard.Library PackageReference implicit, I'd like to do the same thing with Microsoft.NETCore.App. Does that package work the same way as NETStandard.Library, where you can always reference the latest version of the package but target a lower version of the framework? Can I have a PackageReference to Microsoft.NETCore.App version 1.1.0, but have my TargetFramework be netcoreapp1.0?

@davidfowl also brought up the issue of how this would work with patch versions of .NET Core. If for Microsoft.NETCore.App, the package version and target framework version need to be the same, can I set the TargetFramework to netcoreapp1.0.1? Or does the package with that version only support 1.0 as the target framework version?

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Dec 2, 2016

@rainersigwald @AndyGerlicher @cdmihai Is there a better way of checking whether a specific item exists than using two excludes like I'm doing here?

@dsplaisted dsplaisted force-pushed the dsplaisted:implicit-netstandard-package-reference branch from 2d65e9d to 16bb902 Dec 2, 2016
@rainersigwald
Copy link
Contributor

@rainersigwald rainersigwald commented Dec 2, 2016

@dsplaisted Yes, using the WithMetadataValue Item Function:

<Project>
  <ItemGroup>
    <I Include="foo" />
    <I Include="bar" />
    <T Include="@(I->WithMetadataValue('Identity', 'foo'))" />
  </ItemGroup>
  <Target Name="Build">
    <Message Importance="high"
             Text="I: @(I)" />
    <Message Importance="high"
             Text="T: @(T)" />
  </Target>
</Project>
s:\msbuild>msbuild test.proj
Microsoft (R) Build Engine version 15.1.371.0
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 12/2/2016 5:19:36 PM.
Project "s:\msbuild\test.proj" on node 1 (default targets).
Build:
  I: foo;bar
  T: foo
Done Building Project "s:\msbuild\test.proj" (default targets).


Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.09
@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Dec 2, 2016

This needs to be coordinated with the nuget feature that allows specifying metadata on package references so the ui doesn't show updates for them. You don't want this abstraction breaking down when any action you take undoes the implicitness.

/cc @emgarten

@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Dec 5, 2016

@davidfowl @ericstj @terrajobst In addition to making the NETStandard.Library PackageReference implicit, I'd like to do the same thing with Microsoft.NETCore.App. Does that package work the same way as NETStandard.Library, where you can always reference the latest version of the package but target a lower version of the framework? Can I have a PackageReference to Microsoft.NETCore.App version 1.1.0, but have my TargetFramework be netcoreapp1.0?

Microsoft.NETCore.App should be exactly the same idea. Newer versions should work with older frameworks.

@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Dec 5, 2016


<!-- Automatically reference NETStandard.Library package if target framework is .NETStandard and the package isn't already referenced -->
<ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETStandard' and '@(_NETStandardLibraryPackageReference)' == ''">
<PackageReference Include="NETStandard.Library" Version="1.6.0"/>

This comment has been minimized.

@davidfowl

davidfowl Dec 5, 2016
Contributor

Hard coding 1.6.0? Don't we want to match the major and minor of the netstandard TFM if it's > 1.6.0?

This comment has been minimized.

This comment has been minimized.

@ericstj

ericstj Dec 5, 2016
Member

I thought @terrajobst had a spec on this.

@emgarten
Copy link
Member

@emgarten emgarten commented Dec 5, 2016

On the nuget side NuGet/Home#4044 is tracking the work for ignoring updates to references such as this.

@dsplaisted dsplaisted force-pushed the dsplaisted:implicit-netstandard-package-reference branch from 16bb902 to e9128ba Dec 7, 2016
@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Dec 7, 2016

@davidfowl

This needs to be coordinated with the nuget feature that allows specifying metadata on package references so the ui doesn't show updates for them. You don't want this abstraction breaking down when any action you take undoes the implicitness.

I've moved the implicit PackageReferences to the .props files, which means the behavior is now this:

  • You get an implicit PackageReference for NETStandard.Library or Microsoft.NETCore.App if you are targeting .NETStandard or .NETCoreApp
  • The version of the package reference is the greater of your target framework version and the highest version of that package that the Sdk knows about
  • You can put a PackageReference to a different version of one of these packages in your project file, and it will override the implicit one from the .props
  • The package (and available updates) will show up in the NuGet package manager UI. If you use the package manager UI to upgrade the package, NuGet will put the PackageReference in your .csproj for you

To me, this seems like ideal behavior. I don't think we need to hide updates to implicit package references from the UI if the UI knows how to correctly apply them.

@srivatsn @nguerrera for review

@gulbanana
Copy link

@gulbanana gulbanana commented Dec 7, 2016

This doesn't seem like it would always be ideal behaviour. Imagine a world where NETStandard.Library 2.0 has been released, and two developers in a team are sharing code:

  • Developer A updates their cli/sdk, and the new version knows about package 2.0
  • Developer A is now implicitly using 2.0 and begins to rely on new APIs
  • Developer B hasn't noticed an update notification or hasn't had time to upgrade, so they're still using the previous SDK version
  • Developer A's changes don't compile on Developer B's machine even though their project files are identical!
@DamianEdwards
Copy link

@DamianEdwards DamianEdwards commented Dec 7, 2016

@gulbanana it shouldn't be based on the SDK you are using, but the TFM you're targeting. They're independent.


<PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' And '$(NetCoreAppDefaultPackageVersion)' == ''" >
<NetCoreAppDefaultPackageVersion>1.0.1</NetCoreAppDefaultPackageVersion>
<NetCoreAppDefaultPackageVersion Condition="'$(_TargetFrameworkVersionWithoutV)' > '$(NetCoreAppDefaultPackageVersion)' ">$(_TargetFrameworkVersionWithoutV)</NetCoreAppDefaultPackageVersion>

This comment has been minimized.

@davidfowl

davidfowl Dec 7, 2016
Contributor

Is this a version comparison?

This comment has been minimized.

@dsplaisted

dsplaisted Dec 7, 2016
Author Member

Yes. When you do a greater/less than comparison with strings, MSBuild tries to parse them as Versions and compare them as such.

This comment has been minimized.

@davidfowl

davidfowl Dec 8, 2016
Contributor

lol, that is so random

@gulbanana
Copy link

@gulbanana gulbanana commented Dec 7, 2016

@DamianEdwards it's the greater-of behaviour dplaisted describes that concerns me a little. if the tfm was left at netstandard1.6 you'd get library 1.6 or 2.0 based on where it was built.

@DamianEdwards
Copy link

@DamianEdwards DamianEdwards commented Dec 7, 2016

@DamianEdwards
Copy link

@DamianEdwards DamianEdwards commented Dec 7, 2016

But to your point, the version implied should be the latest version the SDK knows about that matches the TFM being targeted.

@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Dec 7, 2016

Higher is always safe but I agree different behavior based on the SDK installed isn't ideal.

The version of the package reference is the greater of your target framework version and the highest version of that package that the Sdk knows about

It might be better to just use the major and minor of the TFM so that things are consistent no matter what the SDK knows about.

@DamianEdwards
Copy link

@DamianEdwards DamianEdwards commented Dec 7, 2016

Yeah that's what I'm proposing.

@DamianEdwards
Copy link

@DamianEdwards DamianEdwards commented Dec 7, 2016

Although I think it's fine to have it float the patch level to highest the SDK is aware of, maybe? Still potential for edge case differences, but vastly reduced and removes the need to have a PackageReference to get latest patch levels by default.

@gulbanana
Copy link

@gulbanana gulbanana commented Dec 7, 2016

If it's an equal-minor check that sounds like it won't cause any serious problems. You could still get slightly weird behaviour when, like, two different people are making release of a nuget package, and its dependencies keep switching between 1.6.1 and 1.6.2 - but this isn't likely to cause real issues.

@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Dec 7, 2016

Although I think it's fine to have it float the patch level to highest the SDK is aware of, maybe? Still potential for edge case differences, but vastly reduced and removes the need to have a PackageReference to get latest patch levels by default.

Yes that would be ok, though it would be great if we avoided patches in general 😄 /cc @terrajobst .

@DamianEdwards
Copy link

@DamianEdwards DamianEdwards commented Dec 7, 2016

Sure, just don't have bugs in the BCL, easy.

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Dec 7, 2016

If you're worried about newer versions of the implicitly referenced packages behaving differently, you should also be worried about differences in behavior between newer versions of the .NET Sdk. So you should pin the Sdk version and then you'll always get the same version of the NuGet packages too.

Right now you can use globals.json to specify which version of the .NET CLI should be used, and we are in the early stages of designing something similar that would refer to the MSBuild Sdks referenced with the Sdk attribute on the Project element.

So I think the logic to use the latest version of the package that the Sdk knows about is a good choice. Does that make sense?

@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Dec 7, 2016

If you're worried about newer versions of the implicitly referenced packages behaving differently, you should also be worried about differences in behavior between newer versions of the .NET Sdk. So you should pin the Sdk version and then you'll always get the same version of the NuGet packages too.

Seems orthogonal. Explicitly updating the SDK is one thing but it shouldn't affect my compilation references if I changed nothing else. Less variance is better here and we should try our best to keep the behavior as similar as possible even when the SDK revs with respect to the runtime behavior.

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Dec 7, 2016

Explicitly updating the SDK is one thing but it shouldn't affect my compilation references if I changed nothing else

Updating to a newer version of the NETStandard.Library or Microsoft.NETCore.App packages shouldn't change your compilation references either, unless we make a mistake or fix a bug.

That said, I don't think it's super critical either way, so I'd be happy to change it to just use the version from the target framework version if there's a chance we can make the PackageReferences implicit for RC2. We can always change the logic later.

@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Dec 7, 2016

Updating to a newer version of the NETStandard.Library or Microsoft.NETCore.App packages shouldn't change your compilation references either, unless we make a mistake or fix a bug.

I agree but we messed this up before, and we can do it again (it's actually broken today in pre release versions of Microsoft.NETCore.App 1.2.0-*).

That said, I don't think it's super critical either way, so I'd be happy to change it to just use the version from the target framework version if there's a chance we can make the PackageReferences implicit for RC2. We can always change the logic later.

Yea, I think we should just change it for now. Users can always override it anyways. This is a good change to make now so we can dogfood the experience.

@nguerrera
Copy link
Member

@nguerrera nguerrera commented Dec 7, 2016

if there's a chance we can make the PackageReferences implicit for RC2

I don't think this should go in to RC2 so late.

@nguerrera
Copy link
Member

@nguerrera nguerrera commented Dec 7, 2016

One thing that was mentioned but maybe not called out explicitly enough is that while a change in the package version should not affect runtime or even compilation behaviour, it will change the nupkg dependencies produced by pack. Are we OK with that varying based on the SDK used?

… DisableImplicitFrameworkReferences property
@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jan 2, 2017

@nguerrera OK, I've changed it so that the DisableImplicitFrameworkReferences property applies to the implicit reference to Microsoft.NETCore.App.

Copy link
Collaborator

@srivatsn srivatsn left a comment

Looks good modulo the extra mismatched ItemGroup in the templates.

@@ -5,7 +5,4 @@
</PropertyGroup>

<ItemGroup>

This comment has been minimized.

@srivatsn

srivatsn Jan 4, 2017
Collaborator

This ItemGroup should be deleted

@@ -5,7 +5,4 @@
</PropertyGroup>

<ItemGroup>

This comment has been minimized.

@srivatsn

srivatsn Jan 4, 2017
Collaborator

This too.

@clairernovotny
Copy link
Member

@clairernovotny clairernovotny commented Jan 4, 2017

Before this gets merged, I'd strongly suggest that the default .NET Standard Library be bumped to 1.6.1. There are issues with 1.6 libraries installing into Xamarin Studio that 1.6.1 fixes. I realize there's an issue with Publish, but there's already a separate issue to track that. It shouldn't affect what's in this PR...?

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jan 4, 2017

@onovotny Without this change the default templates will reference version 1.6.0 of NETStandard.Library. With this change, it will be referenced by default, but you can still upgrade either in the Package Manager UI, or by adding the following to your project (which is what the UI will do if you upgrade):

<PackageReference Update="NETStandard.Library" Version="1.6.1" />

So this change doesn't make the situation significantly better or worse, so I'd like to handle updating the default separately since it introduces other issues (#571).

@clairernovotny
Copy link
Member

@clairernovotny clairernovotny commented Jan 4, 2017

@dsplaisted It doesn't make things better or worse, but it doesn't set people up in the right direction either. People won't necessarily update (or won't bother to update because they don't know there's a potential tooling issue around Xamarin that 1.6.1 fixes). That's why I'm suggesting that 1.6.1 be the version referenced by default.

I've just seen too many issues filed against my projects that pulled in 1.6.0 by Xamarin users.

@dsplaisted dsplaisted requested review from nguerrera and natidea Jan 4, 2017
@dsplaisted dsplaisted merged commit de0daf7 into dotnet:master Jan 4, 2017
4 checks passed
4 checks passed
Ubuntu14.04 Debug Build finished.
Details
Ubuntu14.04 Release Build finished.
Details
Windows_NT Debug Build finished.
Details
Windows_NT Release Build finished.
Details
@dsplaisted dsplaisted deleted the dsplaisted:implicit-netstandard-package-reference branch Jan 4, 2017
@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jan 4, 2017

@onovotny I looked more at updating the version of NETStandard.Library to 1.6.1 and ran into some more issues. I'm continuing to work through those but I didn't want to hold up this PR and it will probably be best to review the changes I make separately, so I've gone ahead and merged this.

<!-- Automatically reference NETStandard.Library or Microsoft.NETCore.App package if targeting the corresponding target framework.
We can refer here in the .props file to properties set in the .targets files because items and their conditions are
evaluated in the second pass of evaluation, after all properties have been evaluated. -->
<ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETStandard'">

This comment has been minimized.

@ramarag

ramarag Jan 12, 2017
Member

Apart from the project and command line, do you see that this variable getting set for specific targets.
For example The Target for cache command will not want this implicit reference

cc @schellap
#591

mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
* Fixing the package path for publish targets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.