-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Introducing netstandard13aot target group in System.IO #6597
Conversation
@weshaggard @ericstj could you please have a look? |
5ff8d09
to
fff4f2c
Compare
@@ -0,0 +1,19 @@ | |||
{ | |||
"frameworks": { | |||
"netstandard1.5": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you were going to make this netstandard13aot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this has been changed to netstandard13aot
, is the commit message still valid?
The PR title and commit message: "Introducing netstandard51aot
target.."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do that. Appending aot to the framework moniker is not valid. 'netstandard13aot' is just an arbitrary string the build tooling is using to enable cross-compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The project.json should just be netstandard1.3 for the framework. netstandard13aot is just shorthand for our build system.
fff4f2c
to
10974ad
Compare
@@ -308,6 +309,13 @@ | |||
<NuGetTargetMoniker>.NETCore,Version=v5.0</NuGetTargetMoniker> | |||
</PropertyGroup> | |||
</When> | |||
<When Condition="'$(TargetGroup)'=='netstandard13aot'"> | |||
<PropertyGroup> | |||
<PackageTargetFramework>netstandard13aot</PackageTargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be just nestandard1.3
10974ad
to
7aeee56
Compare
@weshaggard & @ericstj I have fixed the code according to your comment and also included the packaging change as Eric described to me. |
<SkipCommonResourcesIncludes>true</SkipCommonResourcesIncludes> | ||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
<ExcludeAssemblyInfoPartialFile>true</ExcludeAssemblyInfoPartialFile> | ||
<StringResourcesPath>Resources/Strings.netcore50aot.resx</StringResourcesPath> | ||
<SkipValidatePackageTargetFramework Condition="'$(TargetGroup)' == 'netstandard13aot'">true</SkipValidatePackageTargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just needed to exclude the System.Private references you can use @(ValidateIgnoreReference) to do so.
7aeee56
to
36fe3bd
Compare
@ericstj fixed the code according to your comments. |
You may as well want to run |
36fe3bd
to
99d2365
Compare
@jasonwilliams200OK I have updated the commit message. thanks |
@dotnet-bot could you please test this? |
Builds are failing because you missed the update to system.io.builds. |
99d2365
to
f861a33
Compare
|
||
<ItemGroup Condition="'$(TargetGroup)' == 'netstandard13aot'"> | ||
<TargetingPackReference Include="System.Private.Interop" /> | ||
<ValidateIgnoreReference Include="System.Private.Interop" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be generalized to <ValidateIgnoreReference Inlcude="@(TargetingPackReference)" />
I know it is only one in this case but it might be better to generalize this. If we start getting more of these libraries we should actually add this in a common target in BuildTools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated that. it is a good idea anyway
LGTM, OK to merge once all builds pass. |
f861a33
to
25715bf
Compare
thanks @ericstj for your help and review here |
We need to build System.IO that work with corert which we need to get rid of WinRT dependencies and that will require to have different code in System.IO which require creating a new target group. This change should be no-op for other target groups. Conflicts: src/System.IO/src/System.IO.csproj
25715bf
to
34154e9
Compare
Introducing netstandard13aot target group in System.IO
Introducing netstandard13aot target group in System.IO Commit migrated from dotnet/corefx@36f0fab
We need to build System.IO that work with corert which we need to get rid of WinRT
dependencies and that will require to have different code in System.IO which require
creating a new target group. This change should be no-op for other target groups.