Cleanup netfx-specific code from projects that no longer build in netfx configurations #31818

TimLovellSmith · 2018-08-16T23:27:06Z

This is to fix #31712. It removes several old interop-only source files, and a bunch of conditional logic around '$(TargetGroup)' == 'netfx' (or 'net45', 'net46', 'net47') for those projects where configuration.props no longer actually includes any such configurations.

Side note 1: this resulted in deleting all source files for System.Runtime.Serialization.Xml, System.Threading.Overlapped, and System.Security.SecureString. I'm not sure if that means it will merit some additional cleanups.

Side note 2: while I was doing this, it looked to me like we might possible be able to cleanup some 'win8' conditionals in some projects too, but I didn't try that.

…onals from System.IO.Compression.csproj

….InteropServices.RuntimeInformation.csproj

…ves.csproj

…aphy.Algorithms.csproj

ViktorHofer · 2018-08-17T12:36:26Z

src/System.Runtime.Serialization.Xml/src/System.Runtime.Serialization.Xml.csproj

  <PropertyGroup>
    <AssemblyName>System.Runtime.Serialization.Xml</AssemblyName>
-    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly>
+    <IsPartialFacadeAssembly>true</IsPartialFacadeAssembly> <!-- actually it looks more like a 100% facade to me now?... -->


cc @ericstj @weshaggard

@ViktorHofer I'm going to delete that comment, but yes, I just wanted to point that out in case that means there's some way to simplify this project/assembly...

right, that's why I pinged my colleagues who should know if there's a way to do so.

We still need to be able to build it from source so this project needs to stay. PartialFacade just means "potentially" partial. We actually have plenty of full facades in the repo.

@ericstj Thanks for clarifying!

Maybe I can delete the <Reference> and <ProjectReference> bits of the project now though, now that there's no code which would need the references to compile.

Maybe I can delete the and bits of the project now though, now that there's no code which would need the references to compile.

Well, that didn't work for some reason that I don't understand yet. I'll roll it back.

The reason why that didn't work is that when we make it a facade, we generate some code that will create the type forwards on the assembly. If you remove all the references, then that generated code won't compile, which is why we still need at least the required references to make that generated source to compile.

krwq · 2018-08-17T17:55:41Z

src/System.Data.Common/src/System.Data.Common.csproj

    <Compile Include="System\Data\Common\IDbColumnSchemaGenerator.cs" />
  </ItemGroup>
-  <ItemGroup Condition="'$(TargetGroup)' != 'netfx'">
+  <ItemGroup>


I'd join groups with the same condition and same items types (or no condition) - make sure to sort it

@krwq I am not sure that everything was already actually sorted alphabetically, (e.g. why does Xml subfolder come beforeData? Maybe there is another convention in operation here?) but I am fine with keeping it tidy by merging groups, and maintaining approximate 'sortedness'.

Hope this is satisfactory now.

…me.Serialization.Xml and System.Security.SecureString.

…em.Runtime.Serialization.Xml and System.Security.SecureString." This reverts commit 7de0b76.

ViktorHofer

I already went through the changes last week and just looked at the new commits. Thanks a lot for doing this! The changes look good 👍

ViktorHofer · 2018-08-23T20:45:04Z

Please solve the conflicts and when CI is green I'm happy to merge.

joperezr · 2018-08-24T16:45:00Z

src/System.Net.Sockets/ref/System.Net.Sockets.csproj

-  </ItemGroup>
-  <ItemGroup Condition="'$(TargetGroup)' != 'netfx'">
    <Compile Include="System.Net.Sockets.netcoreapp.cs" />
+  </ItemGroup>


NIT: Given that System.Net.Sockets.netcoreapp.cs will be included every time, perhaps we should just fold the contents into System.Net.Sockets.cs

Sounds nice, but just a little bit more involved than I wanted this to be, so hope its OK if I pass on that one!

joperezr · 2018-08-24T16:45:47Z

src/System.Net.Sockets/ref/System.Net.Sockets.csproj

-  <ItemGroup Condition="'$(TargetGroup)' != 'netfx'">
    <Compile Include="System.Net.Sockets.netcoreapp.cs" />
+  </ItemGroup>
+  <ItemGroup>


NIT: No need for an extra <ItemGroup> here, since we can simply use the Compile one for ProjectReference as well

I must disagree. We keep Compile items separate from ProjectReferences in corefx.

yup you are right, didn't notice that it was ProjectReferences on the other ItemGroup.

joperezr · 2018-08-24T16:51:53Z

...ntimeInformation/src/System/Runtime/InteropServices/RuntimeInformation/RuntimeInformation.cs

 #if uapaot
        private const string FrameworkName = ".NET Native";
-#elif netfx || win8
+#elif win8


You are right in your comment, looks like win8 can be removed as well, but I'm fine with scoping your PR to just netfx for now, and do another pass later for other unused configurations.

joperezr · 2018-08-24T16:56:29Z

src/System.Runtime.Serialization.Primitives/ref/System.Runtime.Serialization.Primitives.csproj

-    <Reference Include="System.Runtime.Serialization" />
-  </ItemGroup>
-  <ItemGroup Condition="'$(TargetGroup)' != 'netfx'">
+  <ItemGroup>


NIT: Same here, just use the previous ItemGroup for cleanliness

We probably need Compile items separate from ProjectReferences here too.

joperezr · 2018-08-24T16:58:25Z

src/System.Runtime.Serialization.Primitives/src/System.Runtime.Serialization.Primitives.csproj

-  <ItemGroup Condition="'$(TargetGroup)'=='netfx'">
-    <Reference Include="mscorlib" />
-    <Reference Include="System.Runtime.Serialization" />
-    <Compile Include="System\Runtime\Serialization\ISerializationSurrogateProvider.cs" />


Can't we delete this file from our source as well?

Looks like we still need this one. In the old project, instead of the source file being included unconditionally, it was being conditionally included for both 'netfx' true and non-'netfx' targets. Now it will become just an unconditional include.

joperezr · 2018-08-24T17:08:27Z

src/System.Security.Cryptography.Algorithms/src/System.Security.Cryptography.Algorithms.csproj

    </Compile>
  </ItemGroup>
-  <ItemGroup Condition=" '$(TargetsWindows)' == 'true' and '$(IsPartialFacadeAssembly)' != 'true'">
+  <ItemGroup Condition=" '$(TargetsWindows)' == 'true' ">


NIT: Extra space between single quote and double-quote

joperezr · 2018-08-24T17:13:40Z

src/System.Threading.Overlapped/src/System.Threading.Overlapped.csproj

    <Configurations>netcoreapp-Unix-Debug;netcoreapp-Unix-Release;netcoreapp-Windows_NT-Debug;netcoreapp-Windows_NT-Release;uap-Windows_NT-Debug;uap-Windows_NT-Release;uapaot-Windows_NT-Debug;uapaot-Windows_NT-Release</Configurations>
  </PropertyGroup>
-  <ItemGroup Condition="'$(TargetGroup)'=='netfx'">
-    <Compile Include="System\Threading\DeferredDisposableLifetime.cs" />


Why didn't we delete this file as well?

Good one, I don't think we need it!

joperezr

Left a few easy comments to address, but overall this looks great and ready to go! Thanks so much for helping cleaning this up! 😄

ViktorHofer · 2018-08-26T13:30:17Z

Thanks a lot for the help @TimLovellSmith 👍

…d in netfx configurations (dotnet#31818)" This reverts commit 21eb9aa89fae41b73f87c073abd95041ecf1b0e0. Signed-off-by: dotnet-bot <[email protected]>

…d in netfx configurations (#31818)" This reverts commit 21eb9aa89fae41b73f87c073abd95041ecf1b0e0. Signed-off-by: dotnet-bot <[email protected]>

…fx configurations (dotnet/corefx#31818) * Remove Crc32Helper.Managed.cs and '$(TargetGroup)' == 'netfx' conditionals from System.IO.Compression.csproj * Remove netfx conditionals from System.Data.Common.csproj * Remove netfx conditionals from System.Diagnostics.StackTrace.csproj * Remove 'netfx' and 'net46' conditionals from System.Net.Http.csproj * Remove 'netfx' conditionals from System.Net.Sockets.csproj * Remove 'netfx', 'net462' and 'net47' conditionals from System.Runtime.InteropServices.RuntimeInformation.csproj * Remove 'netfx' conditionals from System.Runtime.Serialization.Primitives.csproj * Remove 'netfx' conditionals from System.Runtime.Serialization.Xml.csproj * Remove 'netfx' and 'net47' conditionals from System.Security.Cryptography.Algorithms.csproj * Remove 'netfx' conditionals from System.Security.SecureString.csproj * Remove 'netfx' conditionals from System.Threading.Overlapped.csproj * Remove 'netfx' conditionals from System.Xml.XPath.XDocument.csproj * Trim out unused source file DeferredDisposableLifetime.cs Commit migrated from dotnet/corefx@6a0ecec

TimLovellSmith added 12 commits August 15, 2018 12:51

Remove Crc32Helper.Managed.cs and '$(TargetGroup)' == 'netfx' conditi…

ec26aea

…onals from System.IO.Compression.csproj

Remove netfx conditionals from System.Data.Common.csproj

069ee78

Remove netfx conditionals from System.Diagnostics.StackTrace.csproj

b998735

Remove 'netfx' and 'net46' conditionals from System.Net.Http.csproj

4d05981

Remove 'netfx' conditionals from System.Net.Sockets.csproj

aee0348

Remove 'netfx', 'net462' and 'net47' conditionals from System.Runtime…

82d264e

….InteropServices.RuntimeInformation.csproj

Remove 'netfx' conditionals from System.Runtime.Serialization.Primiti…

f968746

…ves.csproj

Remove 'netfx' conditionals from System.Runtime.Serialization.Xml.csproj

6526fcd

Remove 'netfx' and 'net47' conditionals from System.Security.Cryptogr…

ffef6e7

…aphy.Algorithms.csproj

Remove 'netfx' conditionals from System.Security.SecureString.csproj

760a517

Remove 'netfx' conditionals from System.Threading.Overlapped.csproj

e41f7f3

Remove 'netfx' conditionals from System.Xml.XPath.XDocument.csproj

5656ebb

ViktorHofer reviewed Aug 17, 2018

View reviewed changes

krwq reviewed Aug 17, 2018

View reviewed changes

TimLovellSmith added 3 commits August 21, 2018 07:32

Remove comment, combine/sort some project items.

eca1189

Remove <References> no longer needed to compile projects System.Runti…

7de0b76

…me.Serialization.Xml and System.Security.SecureString.

Revert "Remove <References> no longer needed to compile projects Syst…

5d91275

…em.Runtime.Serialization.Xml and System.Security.SecureString." This reverts commit 7de0b76.

karelz added the area-Meta label Aug 23, 2018

karelz assigned TimLovellSmith, krwq, ViktorHofer and joperezr Aug 23, 2018

ViktorHofer approved these changes Aug 23, 2018

View reviewed changes

Merge branch 'master' into cleanupOldNetFXConditions

157cbc5

joperezr reviewed Aug 24, 2018

View reviewed changes

joperezr approved these changes Aug 24, 2018

View reviewed changes

Trim out unused source file DeferredDisposableLifetime.cs

c58b7a5

ViktorHofer merged commit 6a0ecec into dotnet:master Aug 26, 2018

ViktorHofer mentioned this pull request Aug 26, 2018

Remove unused configurations and merge files #31959

Merged

karelz added this to the 3.0 milestone Sep 6, 2018

Cleanup netfx-specific code from projects that no longer build in netfx configurations #31818

Cleanup netfx-specific code from projects that no longer build in netfx configurations #31818

Uh oh!

Conversation

TimLovellSmith commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joperezr left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Aug 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TimLovellSmith commented Aug 16, 2018 •

edited

Loading