Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dead code from WCF repository & simplify msbuild logic #5222

Merged
merged 4 commits into from
Aug 4, 2023

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Aug 1, 2023

The motivation for this change is that we in runtime owned most of the dead code in this repository and I just stumbled over it while looking into this repository. I tested the changes by diffing the assembly metadata.

Two commits:

  1. Remove dead code from WCF repository
  • .gitmirrorall, DotnetCLIVersion.txt, ., harvest.targets,
    Configurations.props, archgroups.props, configurationgroups.props,
    DefaultGenApiDocIds.txt, LicenseHeader.txt, osgroups.props,
    properties.props, TargetFramework.props, Configurations.props and
    targetgroups.props were all used by dotnet/buildtools tooling that
    has been removed more than five years ago from this repository.
  • uwp.targets, Publishing.props aren't used anymore (Publishing.props
    isn't necessary anymore as Arcade defaults to version 3).
  • build.ps1/build.sh code was copied from runtime and doesn't apply to
    this repository.
  • project files: IsBuilding, IsHarvesting are dead properties.
    PackageReference to Microsoft.NETFramework.ReferenceAssemblies is
    added automatically by the SDK. Use EnableDefaultItems=false instead
    of removing Compile items and resources. Remove outdated comment.
  1. Simplify msbuild logic
  • Remove dead properties from TestProperties.props and move the
    remaining ones directly into Directory.Build.props
  • Fix dotnet-svcutil-lib.csproj condition which was conditioning on the
    ConfigurationGroup property which wasn't set anymore.
  • Remove dead properties (which now are either the default or were
    removed) from Versions.props. Move one version property from
    Directory.Build.props into Versions.props
  • Remove the unnecessary "ToolsVersion="14.0" xmlns="..."" tag.
    MSBuild/VS doesn't require that anymore.
  • Drastically simplify logic in Versions.props (sync with what we have
    in runtime) by removing the now unnecessary targets. Move what
    remains directly into Directory.Build.*.
  • Remove dead code from ReferenceAssemblies.props and move the
    remainder into Directory.Build.props.

- .gitmirrorall, DotnetCLIVersion.txt, _._, harvest.targets,
  Configurations.props, archgroups.props, configurationgroups.props,
  DefaultGenApiDocIds.txt, LicenseHeader.txt, osgroups.props,
  properties.props, TargetFramework.props, Configurations.props and
  targetgroups.props were all used by dotnet/buildtools tooling that
  has been removed more than five years ago from this repository.
- uwp.targets, Publishing.props aren't used anymore (Publishing.props
  isn't necessary anymore as Arcade defaults to version 3).
- build.ps1/build.sh code was copied from runtime and doesn't apply to
  this repository.
- project files: IsBuilding, IsHarvesting are dead properties.
  PackageReference to Microsoft.NETFramework.ReferenceAssemblies is
  added automatically by the SDK. Use EnableDefaultItems=false instead
  of removing Compile items and resources. Remove outdated comment.
1. Remove dead properties from TestProperties.props and move the
   remaining ones directly into Directory.Build.props
2. Fix dotnet-svcutil-lib.csproj condition which was conditioning on the
   ConfigurationGroup property which wasn't set anymore.
3. Remove dead properties (which now are either the default or were
   removed) from Versions.props. Move one version property from
   Directory.Build.props into Versions.props
4. Remove the unnecessary "ToolsVersion="14.0" xmlns="..."" tag.
   MSBuild/VS doesn't require that anymore.
5. Drastically simplify logic in Versions.props (sync with what we have
   in runtime) by removing the now unnecessary targets. Move what
   remains directly into Directory.Build.*.
6. Remove dead code from ReferenceAssemblies.props and move the
   remainder into Directory.Build.props.
<IsShipping>$(Ship_WcfPackages)</IsShipping>
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
<IsPartialFacadeAssembly Condition="'$(TargetFramework)' == 'net461'">true</IsPartialFacadeAssembly>
<IsPartialFacadeAssembly Condition="'$(TargetFramework)' == 'net461'">true</IsPartialFacadeAssembly>
<EnableDefaultItems Condition="'$(TargetFramework)' == 'net461'">false</EnableDefaultItems>
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is wrong on this line. First character is a tab, then there's spaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed via 646c506

Copy link
Member

@mconnew mconnew left a comment

Choose a reason for hiding this comment

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

Just the one indentation that needs fixing, then it's good. It's possible we might have problems with the release CI, that's impossible to validate before hand. We'll ping you if that's the case.

@ViktorHofer ViktorHofer merged commit 7cd6512 into main Aug 4, 2023
8 checks passed
@ViktorHofer ViktorHofer deleted the CleanupWcfRepo branch August 4, 2023 06:11
ViktorHofer added a commit that referenced this pull request Aug 4, 2023
ViktorHofer added a commit that referenced this pull request Aug 11, 2023
* Enable CS1591 errors for undocumented API

Similar as dotnet/extensions#4230, enable the CS1591 compiler errors when a library generates a documentation file.

* Update System.Web.Services.Description.csproj

feedback from #5222

* Fix "build -vs" and adding /// comments to Federation

---------

Co-authored-by: Matt Connew <[email protected]>
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