Remove .NET Standard 1.x dependencies and cleanup #6403

ViktorHofer · 2023-04-17T09:06:33Z

Avoid .NET Standard 1.x dependencies in the repository completely. The remaining test package dependency that brings netstandard1.x is FakeItEasy. For that one, upgrade the transitive Castle.Core dependency to 5.x. I also opened an issue in FakeItEasy to discuss upgrading the Castle.Core dependency. This results in the repository not being affected by CVEs / vulnerable packages like System.Private.Uri that are part of the .NET Standard 1.x dependency graph. (cc @MichaelSimons, @mmitche, @ericstj)
Upgrade projects that use "System.Runtime.Loader" from netstandard2.0 to .NETCoreApp as that API is only availble on .NETCoreApp anyway but keep the netstandard2.0 TFM. The package that was used hasn't shipped for years anymore and was never intended to be used on .NET Standard (support for it was later removed). Because of that, new nullable reference type errors were raised by the compiler which I suppressed temporarily by downgrading the Nullable property to "annotations".
Clean-up code files
- Use consistent indentation
- Empty lines after Project and before closing tag
- Empty lines between Property/Item groups
- Remove unused msbuild properties
- Delete unused or non-necessary files (i.e. Build.props).
- Remove unnecessary "<?xml" and "ToolsVersion" tags in files that don't need them (msbuild).
- Define Newtonsoft.Json package version in Packages.props instead of hardcoding it where it is used.
- Use the implicitly (by the SDK) defined #if NETFRAMEWORK compiler preprocessor directive instead of a custom "NETFULL" one.
- Remove one "#nullable enable" statement that wasn't necessary anymore as all projects that included the source file already enabled nullable.
Use floating TFM version that is defined in Arcade: "NetCurrent"
For the .NET Framework TFM msbuild property, use the same nomenclature as in other repos.
Do not define a floating TFM msbuild property for netstandard2.0 as that TFM will never change to a higher version.
Change how the live Microsoft.NETCore.App targeting/runtime pack is upgraded in this repository to allow previous TFMs to be targeted as well (for msbuild tasks).
Remove the "PackSpecific" msbuild property and its conditions as .NET Framework assets can be built and packaged on Unix machines these days with the dynamic reference on .NET Framework targeting packs.

1. Avoid .NET Standard 1.x dependencies in the repository completely. The remaining test package dependency that brings netstandard1.x is FakeItEasy. For that one, upgrade the transitive Castle.Core dependency to 5.x. I also opened an issue in FakeItEasy to discuss upgrading the Castle.Core dependency. This results in the repository not being affected by CVEs / vulnerable packages like System.Private.Uri that are part of the .NET Standard 1.x dependency graph. 2. Upgrade projects that use "System.Runtime.Loader" from netstandard2.0 to .NETCoreApp as that API is only availble on .NETCoreApp anyway. The package that was used hasn't shipped for years anymore and was never intended to be used on .NET Standard (support for it was later removed). Because of that, new nullable reference type errors were raised by the compiler which I suppressed temporarily by downgrading the Nullable property to "annotations". 3. Clean-up code files - Use consistent indentation - Empty lines after Project and before closing tag - Empty lines between Property/Item groups - Remove unused msbuild properties - Delete unused or non-necessary files (i.e. Build.props). - Remove unnecessary "<?xml" and "ToolsVersion" tags in files that don't need them (msbuild). - Define Newtonsoft.Json package version in Packages.props instead of hardcoding it where it is used. - Use the implicitly (by the SDK) defined #if NETFRAMEWORK compiler preprocessor directive instead of a custom "NETFULL" one. - Remove one "#nullable enable" statement that wasn't necessary anymore as all projects that included the source file already enabled nullable. 4. Use floating TFM version that is defined in Arcade: "NetCurrent" 5. For the .NET Framework TFM msbuild property, use the same nomenclature as in other repos. 6. Do not define a floating TFM msbuild property for netstandard2.0 as that TFM will never change to a higher version. 7. Change how the live Microsoft.NETCore.App targeting/runtime pack is upgraded in this repository to allow previous TFMs to be targeted as well (for msbuild tasks). 8. Remove the "PackSpecific" msbuild property and its conditions as .NET Framework assets can be built and packaged on Unix machines these days with the dynamic reference on .NET Framework targeting packs.

mmitche · 2023-04-17T15:21:06Z

Spring cleaning in .NET! Lovely @ViktorHofer

vlada-shubina

There are couple of change requests inline.

For future, we try to do all formatting changes in separate commit, so they can be ignored when reviewing.

vlada-shubina · 2023-04-18T13:55:36Z

Directory.Build.props

-    </PropertyGroup>
+  <PropertyGroup>
+    <LangVersion>preview</LangVersion>
+    <NetFrameworkToolCurrent>net472</NetFrameworkToolCurrent>


I'm not sure if this change is for better, for this repo full framework is not use as tool. We equally release full framework and .NET version

vlada-shubina · 2023-04-18T13:58:17Z

src/Microsoft.TemplateEngine.Abstractions/Microsoft.TemplateEngine.Abstractions.csproj

-    <TargetFrameworks Condition="'$(PackSpecific)' == 'true'">$(NETStandardTargetFramework)</TargetFrameworks>
+    <TargetFrameworks>netstandard2.0;$(NetFrameworkToolCurrent)</TargetFrameworks>


Why the decision was made to remove $(NETStandardTargetFramework) in favor of specific reference?
It is much easier to control all the versions in single place?

vlada-shubina · 2023-04-18T13:58:48Z

src/Microsoft.TemplateEngine.Edge/Microsoft.TemplateEngine.Edge.csproj

+    <!-- TODO: Change nullable to enable. -->
+    <Nullable>annotations</Nullable>


vlada-shubina · 2023-04-18T13:59:57Z

src/Microsoft.TemplateEngine.IDE/Microsoft.TemplateEngine.IDE.csproj

  <PropertyGroup>
-    <TargetFrameworks Condition="'$(PackSpecific)' != 'true'">$(NETStandardTargetFramework);$(NETFullTargetFramework)</TargetFrameworks>
-    <TargetFrameworks Condition="'$(PackSpecific)' == 'true'">$(NETStandardTargetFramework)</TargetFrameworks>
+    <TargetFrameworks>$(NetCurrent);$(NetFrameworkToolCurrent)</TargetFrameworks>


netstandard2.0 should be used here.

vlada-shubina · 2023-04-18T14:01:32Z

test/Directory.Build.targets

-    <PackageReference Condition="'$(IsTestProject)' == 'true'" Include="System.Net.Http" Version="4.3.4" />
-    <PackageReference Condition="'$(IsTestProject)' == 'true'" Include="System.Security.Cryptography.X509Certificates" Version="4.3.0" /> 
-    <PackageReference Condition="'$(IsTestProject)' == 'true'" Include="Newtonsoft.Json" Version="13.0.1" />


We had to add other package references to ensure no issues in CG. Has something changed since then?

vlada-shubina · 2023-04-18T14:03:02Z

...tActions/RestoreNuGet/TwoProjectsFiles/Custom/MyTestProject.Tests/MyTestProject.Tests.csproj

@@ -5,8 +5,4 @@
    <TargetFramework>net7.0</TargetFramework>
  </PropertyGroup>

-  <ItemGroup>


This package reference is expected by tests and should be kept.

vlada-shubina · 2023-04-18T14:03:10Z

...RestoreNuGet/TwoProjectsPrimaryOutputs/Custom/MyTestProject.Tests/MyTestProject.Tests.csproj

@@ -5,8 +5,4 @@
    <TargetFramework>net7.0</TargetFramework>
  </PropertyGroup>

-  <ItemGroup>
-    <PackageReference Include="Newtonsoft.Json" Version="13.0.1" />


This package reference is expected by tests and should be kept.

vlada-shubina · 2023-04-18T14:04:15Z

tools/legacy-tools/BaselineComparer/BaselineComparer/BaselineComparer.csproj

@@ -2,7 +2,7 @@

  <PropertyGroup>
    <OutputType>Exe</OutputType>
-    <TargetFramework>$(NETCoreTargetFramework)</TargetFramework>
+    <TargetFramework>$(NetCurrent)</TargetFramework>


legacy tools are not used, and those changes might be skipped.

vlada-shubina · 2023-04-18T14:06:46Z

Directory.Build.props

-        <NETCoreTargetFramework>net8.0</NETCoreTargetFramework>
-        <NETStandardTargetFramework>netstandard2.0</NETStandardTargetFramework>
-        <NETFullTargetFramework>net472</NETFullTargetFramework>


imo removing those properties is not for good.
They name intention correctly and I prefer to keep them, also for .NET Standard.

They may be changed as

<NETTargetFramework>$(NetCurrent)</NETTargetFramework> <NETStandardTargetFramework>netstandard2.0</NETStandardTargetFramework> <NETFullTargetFramework>net472</NETFullTargetFramework>

to remove Core and make use of automatic .NET (core) updates.

YuliiaKovalova · 2023-06-14T10:10:08Z

Hi @ViktorHofer,

Do you still plan to deliver these changes?

ViktorHofer · 2023-06-14T10:45:37Z

Do you still plan to deliver these changes?

Yes, I will get back to in the coming weeks. Changing to draft PR.

ViktorHofer requested a review from a team as a code owner April 17, 2023 09:06

ViktorHofer self-assigned this Apr 17, 2023

ViktorHofer requested a review from GangWang01 as a code owner April 17, 2023 09:06

ViktorHofer force-pushed the NetStandard1xDepRemovalAndCodeStyling branch from 4f32c9c to 2c4e447 Compare April 17, 2023 14:39

ViktorHofer force-pushed the NetStandard1xDepRemovalAndCodeStyling branch from 2c4e447 to afad323 Compare April 17, 2023 15:09

vlada-shubina requested changes Apr 18, 2023

View reviewed changes

ViktorHofer mentioned this pull request Apr 27, 2023

Avoid .NET Standard 1.x dependencies in core stack repositories dotnet/arcade#13315

Open

ViktorHofer marked this pull request as draft June 14, 2023 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove .NET Standard 1.x dependencies and cleanup #6403

Remove .NET Standard 1.x dependencies and cleanup #6403

ViktorHofer commented Apr 17, 2023 •

edited

mmitche commented Apr 17, 2023

vlada-shubina left a comment

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

vlada-shubina Apr 18, 2023

YuliiaKovalova commented Jun 14, 2023

ViktorHofer commented Jun 14, 2023

		<TargetFrameworks Condition="'$(PackSpecific)' == 'true'">$(NETStandardTargetFramework)</TargetFrameworks>
		<TargetFrameworks>netstandard2.0;$(NetFrameworkToolCurrent)</TargetFrameworks>

		<!-- TODO: Change nullable to enable. -->
		<Nullable>annotations</Nullable>

Remove .NET Standard 1.x dependencies and cleanup #6403

Are you sure you want to change the base?

Remove .NET Standard 1.x dependencies and cleanup #6403

Conversation

ViktorHofer commented Apr 17, 2023 • edited

mmitche commented Apr 17, 2023

vlada-shubina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliiaKovalova commented Jun 14, 2023

ViktorHofer commented Jun 14, 2023

ViktorHofer commented Apr 17, 2023 •

edited