Bump AssemblyVersion for nestandard.dll to 2.1.0.0 #1047
Conversation
netstandard/ref/netstandard.csproj
Outdated
| @@ -1,6 +1,6 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <AssemblyVersion>2.0.0.0</AssemblyVersion> | |||
| <AssemblyVersion>2.1.0.0</AssemblyVersion> | |||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
| <OutputType>Library</OutputType> | |||
| <TargetFramework>netstandard2.0</TargetFramework> | |||
There was a problem hiding this comment.
You may also want to look into here:
As well as what you want the behavior to be when that package is installed in older netstandard versions. Previously it tried to support the older TFMs but that might not be so scalable now that the package contains the binaries. It also may not be a requirement anymore since we don't persist the package as a dependency. Whatever you decide would need to be coordinated with the SDK folks.
|
@livarcocc @dsplaisted will the SDK have any issues with us bumping the Package's |
|
@wtgodbe I don't know how the |
|
Updated. @ericstj I'm also unclear on if we need the same bump in other places, like https://github.com/dotnet/standard/blob/master/src/netstandard/pkg/NETStandard.Library.dependencies.props#L136-L140, https://github.com/dotnet/standard/blob/master/src/netstandard/pkg/shims/netstandard/Directory.Build.props#L5, and https://github.com/dotnet/standard/blob/master/src/netstandard/pkg/shims/netfx/Directory.Build.props#L5 |
Looks like we're hitting https://github.com/dotnet/sdk/blob/7b0fe5695ba8d8efe57cb1ae7ff95a87c194e46a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets#L155-L162. I'm not sure how we're setting |
|
|
@wtgodbe You should be able to set As far as the CodeDom error, that seems valid. .NET 4.6.1 won't support .NET Standard 2.1, so that reference isn't valid. I don't know the details of how your repo works, it seems like that project probably shouldn't have a P2P reference to the NETStandard project. |
|
Looks like the old |
|
@wtgodbe You don't want to update it in the bundled versions file. You have a chicken and egg problem, you are using an SDK that doesn't support targeting .NET Standard 2.1 in order to build the .NET Standard 2.1 reference assemblies. So you should override the maximum .NET Standard version property just in your repo. |
|
I've fixed the
https://github.com/dotnet/standard/blob/master/src/netstandard/pkg/shims/netfx/System.csproj#L10 https://github.com/dotnet/standard/blob/master/src/platforms/extensions/System.CodeDom.csproj#L9 Should I just remove the references to these projects from netfx proj's (like System.csproj)? I'm not confident that that's the right path, since there must be a good reason those specific projects were referenced in the first place. @ericstj @terrajobst any idea of what the original reasoning was here? |
|
Looks to me like those projects are responsible for representing extensions built in other repos for the purposes of dangling type-forwards out of netfx shims. Per our discussion, I think we should just try to make those target netstandard2.1 if possible. If not you can probably use AssetTargetFallBack to make it work. |
|
Everything should build successfully with my latest commit (build is good & |
|
We still need to answer the question of "what should this package support". With this change, the package supports: If we want it to support netstandard2.0 then we'll need to harvest those assets and put them in the package. Alternatively we could drop support for the old TFMs with the expectation that the SDK will multiplex based on the TFM (and folks who have a manual package reference will never click the "Upgrade" button). I really want the SDK folks and @terrajobst to weigh in on this. |
|
For now, the least-breaking way to fix this may be to harvest all of the assets that used to live in |
|
@ericstj We are planning to use a targeting pack for .NET Standard 2.1. So probably we don't need to even publish a 2.1 version of NETStandard.Library to NuGet.org in the end. In the interim until we have a targeting pack, we will probably continue to use a package similar to what currently exists. But it is fine for you to drop support for all previous versions of .NET Standard from the NuGet package, if that makes it easier for you. See also #1051 |
|
@dsplaisted thanks, |
|
@ViktorHofer we still use all of those things for APICompat, so at the least we want to continue building them. The netfx/netstandard shims are what wind up in the |
|
So to be clear, we want to change instances of |
|
I think @ViktorHofer already took care of the targetframework issue, so we just need to add the 2.0 stuff to the package. |
|
This should be good to go now, @ericstj @ViktorHofer could you take a final look? It's now producing a package with both the |
eng/Tools.props
Outdated
| @@ -11,6 +11,9 @@ | |||
|
|
|||
| <!-- Need to keep in sync with CodeAnalysis.targets file. --> | |||
| <AnalyzerPropsFile>$(ArtifactsToolsetDir)Common\Tools.Analyzers.props</AnalyzerPropsFile> | |||
|
|
|||
| <!-- Restore specific version of NetStandard.library --> | |||
| <NetStandardImplicitPackageVersion>$(NetStandardLibraryPackageVersion)</NetStandardImplicitPackageVersion> | |||
There was a problem hiding this comment.
Did you need to set this? I would have expected that we would have disabled all the implicit package references in this repository (so it doesn't end up referencing what we are actually building).
There was a problem hiding this comment.
The SDK packages all implicitly reference NetStandard.Library/Microsoft.NETCore.App, if I don't set this we'll restore the default version & I won't have the path to find that package in my package cache. If I set it here, this project will restore the version that I want so I know where to find it.
There was a problem hiding this comment.
Did you consider DisableImplicitFrameworkReferences?
There was a problem hiding this comment.
Actually I see we already use that in some projects.
There was a problem hiding this comment.
Yes, but I figured it was easier to use the implicit reference & just give it a specific version to restore, rather than disabling the reference to NetStandard.Library then adding a manual reference to it.
There was a problem hiding this comment.
I see, are you sure we'll restore one of those projects that references it? The other places where we're working around missing assets file make me think we may not be. If that's the case maybe we can just add an explicit reference somewhere that we're sure will be restored.
| </PropertyGroup> | ||
|
|
||
| <!-- Disable code paths that require project.assets.json files to be present or to be computed. --> |
There was a problem hiding this comment.
Did you need these? They all look netcoreapp specific, I wouldn't expect them to be causing a problem for netstandard build.
There was a problem hiding this comment.
Yes to disable the Target that tries to resolve stuff from project.assets.json, without it I get the following error:
E:\code\standard.dotnet\sdk\2.1.401\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(129,5): error NETSDK1004: Assets file 'E:\code\standard\artifacts\obj\System.CodeDom\project.assets.json' not found. Run a NuGet package restore to generate this file. [E:\code\standard\src\platforms\extensions\System.CodeDom.csproj]
There was a problem hiding this comment.
I see, that's annoying. It looks like SDK does indeed try to do this for netstandard projects. What changed that caused that to start breaking?
There was a problem hiding this comment.
I guess just the retargeting to ns2.1.
There was a problem hiding this comment.
I see we're doing this elsewhere: https://github.com/dotnet/standard/search?q=GenerateDependencyFile&unscoped_q=GenerateDependencyFile. Can we centralize it?
There was a problem hiding this comment.
Please consider a followup PR to centralize these properties.
There was a problem hiding this comment.
Lol. One of my follow-up PRs would have been to bring back project restore so that we don't need these hacks.
|
@ericstj I updated this so we explicitly restore NETStandard.library 2.0.3, and throw an error if there are no NETStandard2.0 package files to restore |
| <ExcludeRestorePackageImports>true</ExcludeRestorePackageImports> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="$(NetStandardLibraryPackage)" Version="$(NetStandardLibraryPackageVersion)" /> |
There was a problem hiding this comment.
You might be able to get away with a hack to put this in tools.props as
<PackageReference Include="$(NetStandardLibraryPackage)" Version="$(NetStandardLibraryPackageVersion)" ExcludeAssets="All" />
There was a problem hiding this comment.
@wtgodbe I agree with Eric. A PackageReference in Tools.props would be cleaner.
|
@ericstj casing wasn't the issue, the Linux build is failing with https://github.com/dotnet/standard/pull/1047/files#diff-2fd532b34b8b1db3ee2c2ad7008d692eR43 & I thought maybe it was a casing issue. Do you think it's ok to disable that Target for non-Windows builds? As far as I can tell from the binlog (from DNCEng), the paths look good to me |
|
Looks to me like it might be coming from the fallback folder. I see lots of NuGetFallbackFolder messages in the log. Elsewhere we've fixed such issues by doing stuff like this: https://github.com/dotnet/arcade/blob/289a8e607ef2a7358c351ddf3d08056186d4e554/src/Microsoft.DotNet.Build.Tasks.Packaging/tests/Microsoft.DotNet.Build.Tasks.Packaging.Tests.csproj#L38. I'm guessing you could hit the same issue on Windows if the machine had a fallback folder installed. We could just disable fallback folders for this repo. Since it doesn't consume a ton of packages that seems reasonable. To do that you'd make a change like this: https://github.com/dotnet/corefx/blob/f84927d4a85444e63ede2da38a49eff7116790db/NuGet.config#L5-L7 |
|
I'm still seeing the same error with the Fallback folder fix, looking |
|
We're green! Anything else I should address @ViktorHofer @ericstj ? |
|
@ViktorHofer @ericstj I updated this to use a PackageReference in Tools.props, everything works locally. Will wait for CI then merge. |
Fixes #929
CC @ericstj @terrajobst