Stabilize configurations for compat pack - #25017
Conversation
| </PackageConfigurations> | ||
| <BuildConfigurations> | ||
| $(PackageConfigurations); | ||
| netcoreapp-Windows_NT; |
There was a problem hiding this comment.
I suppose these guys are here for the build from source effort. If so, wouldn't the netstandard configurations be good enough for that? cc: @weshaggard
There was a problem hiding this comment.
I suppose these guys are here for the build from source effort. If so, wouldn't the netstandard configurations be good enough for that? cc: @weshaggard
Yes. That is why I'm adding netcoreapp as a build configuration:
#24719 (comment)
There was a problem hiding this comment.
Anything in the shared framework should build against netcoreapp instead of netstandard.
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netstandard-Debug|AnyCPU'" /> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netstandard-Release|AnyCPU'" /> | ||
| <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp2.0'"> | ||
| <ItemGroup Condition="$(TargetGroup.Contains('netcoreapp'))"> |
There was a problem hiding this comment.
Does this work on all msbuilds that we support? MSBuild 14.0, CLI? If so, perhaps we should do a pass in the future so that we make all the conditions be like this.
There was a problem hiding this comment.
I built it locally and it worked by running build.cmd is that enough?
There was a problem hiding this comment.
I expect that this works in all the msbuilds we support.
There was a problem hiding this comment.
MSBuild has had property functions since at least 2010.
| <ProjectReference Include="..\src\System.IO.Ports.csproj" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <InboxOnTargetFramework Include="$(UAPvNextTFM)" /> |
There was a problem hiding this comment.
Because it was duplicated, it is right above before the project reference. It was just a cleanup
| @@ -0,0 +1 @@ | |||
| advapi32.dll!LsaNtStatusToWinError No newline at end of file | |||
There was a problem hiding this comment.
Why does this need baselined?
There was a problem hiding this comment.
The .netcoreapp2.0 was part of the filename.
Note this is legal in UWP RS3 so we could arguably add it to our OneCore baseline depending on how far back we support OneCore. I have never updated that baseline. For now exclusion is fine.
There was a problem hiding this comment.
Yes. Since we are building for netcoreapp2.0 and netcoreapp I added the netcoreapp exclusion since the other one is explicit to netcoreapp2.0. I was thinking of also removing the netcoreapp part but that would add the exclusion for all the other frameworks.
weshaggard
left a comment
weshaggard
left a comment
There was a problem hiding this comment.
A couple questions otherwise looks good.
|
Thanks for the reviews :) |
…sions (dotnet#25017) * Address last nullable feedback from System.Runtime * Address nullable feedback for System.Runtime.Extensions * PR Feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
…sions (#25017) * Address last nullable feedback from System.Runtime * Address nullable feedback for System.Runtime.Extensions * PR Feedback Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Stabilize configurations for Microsoft.Win32.Registry * Stabilize configurations for System.IO.FileSystem.AccessControl * Stabilize configurations for System.IO.Packaging * Stabilize configurations for System.Security.AccessControl * Stabilize configurations for System.Principal.Windows * Stabilize configurations for System.ServiceProcess.ServiceController * Stabilize configurations for System.Text.Encoding.CodePages * Some extra cleanup * Fix CI failures Commit migrated from dotnet/corefx@39cdb93
5 participants
With this change all the packages included in the compat pack are netstandard2.0 and netcoreapp2.0 compatible and none of them now include a 2.1 asset.
I also did a small cleanup of not necessary configurations and conditions in the csproj files.
I split every project in a different commit so it is easier to review.
cc: @weshaggard @joperezr @danmosemsft