Skip to content

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Apr 19, 2023

  1. Avoid package dependencies on inbox libraries to not bring in deprecated or no longer supported packages:
    • System.Net.Http (inbox on all TFMs)
    • System.IO.Compression (inbox on all TFMs)
    • System.Reflection.Metadata (inbox on .NETCoreApp)
    • ...
  2. Remove versions that aren't used anymore and update dependencies to latest servicing digit. Make sure that nearly all versions are encoded in Versions.props instead of hardcoded in project files except for few deprecated cases like Microsoft.Cci. Mark deprecated packages in Versions.props with a TODO to switch off them.
  3. Use NetCurrent, NetFrameworkMinimum and NetFrameworkToolCurrent for floating TFMs. Expose the latter two + NetFrameworkCurrent in the Arcade.Sdk. Change build conditions to not assert on a specific TFM version.
  4. Remove custom CodeAnaylsis package version from GenAPI, ApiCompat and GenFacades.
  5. Format project files:
    • Empty lines after Project and before closing tag
    • Empty lines between Property/Item groups
    • Remove unused or duplicated msbuild properties
    • Delete unused or non-necessary files
    • Use less verbose item include/update syntax
    • Remove unnecessary "<?xml" and "ToolsVersion" tags in files that don't need them (msbuild).
    • Add missing license headers in the package msbuild files and remove license headers from project files which don't need them.
  6. Avoid netstandard1.x dependencies and lift potential NETStandard.Library transitive references to the latest version (2.0.3).
  7. Use Microsoft.Build.NoTargets Sdk to create content only packages to avoid defining custom targets and/or invoking the compiler.
  8. Remove unnecessary needed project dependencies (package and/or project references).
  9. Fix ApiCompat output path and ApiCompatAssembly property in package build file.
  10. Use "TargetFramework" instead of "TargetFrameworks" in single targeting projects for improved performance.
  11. Avoid the external Xunit.SkippableFact dependency in a test project in favor of Microsoft.DotNet.XUnitExtensions which is used by the stack and part of Arcade.

I scanned all projects.assets.json files to make sure that this change doesn't bring in a vulnerable package dependency. The motivation for this change to remove a large part of the dependency graph and with that potential vulnerable packages.

@ViktorHofer ViktorHofer requested a review from mmitche April 19, 2023 15:06
@ViktorHofer ViktorHofer self-assigned this Apr 19, 2023
@ViktorHofer ViktorHofer force-pushed the UpgradeArcadeDependenciesAndCleanup branch 4 times, most recently from c2a6d02 to 9ba4576 Compare April 19, 2023 16:05
@ViktorHofer ViktorHofer marked this pull request as draft April 19, 2023 16:40

<PropertyGroup>
<TargetFramework>$(TargetFrameworkForNETSDK)</TargetFramework>
<TargetFramework>$(NetCurrent)</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this one. NetCurrent is coming from the arcade that arcade depends on. This means that when you update NetCurrent, it shows no immediate change in this repo. It requires a roundtrip to do so. It flows out and downstream. Presumably this works, but it's a little non-intuitive.

Copy link
Member Author

@ViktorHofer ViktorHofer Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should use NetCurrent consistently in the entire stack and remove these custom property names per repository.

In this case, when Arcade changes NetCurrent to mean net9.0 in the Arcade.Sdk, we should just temporarily until the next dependency update overwrite the value in Arcade's Directory.Build.props:

Directory.Build.props:

<!-- TODO: Remove with the next Arcade dependency update. -->
<NetCurrent>net9.0</NetCurrent>

@ViktorHofer ViktorHofer force-pushed the UpgradeArcadeDependenciesAndCleanup branch 3 times, most recently from 7484a6d to 3109284 Compare April 24, 2023 16:12
@ViktorHofer ViktorHofer marked this pull request as ready for review April 24, 2023 16:40
1. Avoid package dependencies on inbox libraries to not bring in
   deprecated or no longer supported packages:
   - System.Net.Http (inbox on all TFMs)
   - System.IO.Compression (inbox on all TFMs)
   - System.Reflection.Metadata (inbox on .NETCoreApp)
   - ...
2. Remove versions that aren't used anymore and update dependencies to
   latest servicing digit. Make sure that nearly all versions are
   encoded in Versions.props instead of hardcoded in project files
   except for few deprecated cases like Microsoft.Cci. Mark deprecated
   packages in Versions.props with a TODO to switch off them.
3. Use NetCurrent, NetFrameworkMinimum and NetFrameworkToolCurrent for
   floating TFMs. Expose the latter two + NetFrameworkCurrent in the
   Arcade.Sdk. Change build conditions to not assert on a specific TFM
   version.
4. Remove custom CodeAnaylsis package version from GenAPI, ApiCompat
   and GenFacades.
5. Format project files:
   - Empty lines after Project and before closing tag
   - Empty lines between Property/Item groups
   - Remove unused or duplicated msbuild properties
   - Delete unused or non-necessary files
   - Use less verbose item include/update syntax
   - Remove unnecessary "<?xml" and "ToolsVersion" tags in files that
     don't need them (msbuild).
   - Add missing license headers in the package msbuild files and remove
     license headers from project files which don't need them.
6. Avoid netstandard1.x dependencies and lift potential
   NETStandard.Library transitive references to the latest version
   (2.0.3).
7. Use Microsoft.Build.NoTargets Sdk to create content only packages to
   avoid defining custom targets and/or invoking the compiler.
8. Remove unnecessary needed project dependencies (package and/or
   project references).
9. Fix ApiCompat output path and ApiCompatAssembly property in package
   build file.
10. Use "TargetFramework" instead of "TargetFrameworks" in single
    targeting projects for improved performance.
11. Avoid the external Xunit.SkippableFact dependency in a test project
    in favor of Microsoft.DotNet.XUnitExtensions which is used by the
    stack and part of Arcade.
@ViktorHofer ViktorHofer force-pushed the UpgradeArcadeDependenciesAndCleanup branch from 3109284 to 93151d5 Compare April 24, 2023 17:17
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Apr 24, 2023

@mmitche this PR is now ready. Can you please trigger the extra validation that you mentioned? Ideally, would like to get this in sooner than later just to avoid continuous merge conflicts in Versions.props.

@garath
Copy link
Member

garath commented Apr 24, 2023

Avoid package dependencies on inbox libraries to not bring in deprecated or no longer supported packages

Just curious, did you have a tool to help identify these or was it all your knowledge of the SDK?

@ViktorHofer
Copy link
Member Author

or was it all your knowledge of the SDK?

It's my knowledge of packages that we brought into the shared framework over the years. In the example of System.IO.Compression or System.Net.Http, in the past the team recommended to reference these packages even on .NET Framework which then would just reference the inbox implementation anyway. These days though we don't want to reference such packages anymore as they aren't published anymore and bring in a huge dependency graph.

Similar for System.Reflection.Metadata which you don't need to reference on .NETCoreApp as it's inbox anyway. I usually use https://apisof.net which nicely tells if an API is available inbox, via a package or not all.

@ViktorHofer ViktorHofer merged commit bde465e into dotnet:main Apr 25, 2023
@ViktorHofer ViktorHofer deleted the UpgradeArcadeDependenciesAndCleanup branch April 25, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants