Project file hygiene

[ViktorHofer]

Commit 1 - Infrastructure changes

1. The generators.targets file had a bug that complained when a project
   uses the default targeting pack references because the
   ArrayMarshaller.cs file was not imported.
   I also cleaned the generators.targets file by simplifying
   conditions.
2. Enable "default items" via the <EnableDefaultItems /> SDK switch
   for any project except tests (for now).
3. Remove outdated infrastructure that isn't needed anymore based on
   project file changes.

Commit 2 - Delete dead code

Commit 3 - Ref/Src/Gen-Project file hygiene

1. Use the SDK's default items feature so that compile items in the
   project directory are imported automatically. Disable that feature
   in few projects which heavily depend on manually including compile
   items based on the TargetPlatformIdentifier.

   Changes made mimic the usage in the Microsoft.Extensions.* projects
   which already used the default items feature.

   I made sure that the numer of comile items included before the change
   and after the change is identical.

   In a follow-up change it would make sense to clean-up all the
   BCL files to use the same set of suffixes, i.e.

   • .NETFramework only files: .netframework.cs instead of
     • .net46.cs
     • .netfx.cs
     • .netframework.cs
     • .net48.cs
     • etc.

   That would allow to define central glob
   exclusion patterns for i.e. .NETFramework builds:

   • TFI .NETFramework: *\**.netcoreapp.cs
   • TFI .NETCoreApp: *\**.netframework.cs
   • ...

2. Remove the unnecessary declaration of <Reference /> and
   <ProjectReference /> items in out-of-band projects as they already
   build on top of the shared framework (targeting pack) and make that
   the default behavior for anything that is out-of-band.

3. Clean-up Reference and ProjectReference items that weren't used by
   the compiler anymore, i.e. System.Runtime.ComilerServices.Unsafe and
   System.Runtime.Extensions (as those are now facades).

4. Add empty lines to separate property and item groups from each other
   so that project files are easier to read and look the same as
   project files that "dotnet new" produces.

Commit 4 - React to analyzer warnings/errors

Projects which previously didn't auto-reference System.Memory,
do so now (projects which are out-of-band and sit on top of the
shared framework). Analyzers which are conditioned on the reference
being available now kick in and hence these two files needed to be
updated.

[ghost]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Commit 1 - Infrastructure changes

1. The generators.targets file had a bug that complained when a project
   uses the default targeting pack references because the
   ArrayMarshaller.cs file was not imported.
   I also cleaned the generators.targets file by simplifying
   conditions.
2. Enable "default items" via the "" SDK switch
   for both ref and source projects.
3. Remove outdated infrastructure that isn't needed anymore based on
   project file changes.

Commit 2 - Delete dead code that dates back to mono BCL build

Commit 3 - Ref/Src/Gen-Project file hygiene

1. Use the SDK's default items feature so that compile items in the
   project directory are imported automatically. Disable that feature
   in few projects which heavily depend on manually including compile
   items based on the TargetPlatformIdentifier.

   Changes made mimic the usage in the Microsoft.Extensions.* projects
   which already used the default items feature.

   I made sure that the numer of comile items included before the change
   and after the change is identical.

   In a follow-up change it would make sense to clean-up all the
   BCL files to use the same set of suffixes, i.e.

   • .NETFramework only files: .netframework.cs instead of
     • .net46.cs
     • .netfx.cs
     • .netframework.cs
     • .net48.cs
     • etc.

   That would allow to define central glob
   exclusion patterns for i.e. .NETFramework builds:

   • TFI .NETFramework: "*.netcoreapp.cs"
   • TFI .NETCoreApp: "*.netframework.cs"
   • ...

2. Remove the unnecessary declaration of "" and
   "" items in out-of-band projects as they already
   build on top of the shared framework (targeting pack) and make that
   the default behavior for anything that is out-of-band.

3. Clean-up Reference and ProjectReference items that weren't used by
   the compiler anymore, i.e. System.Runtime.ComilerServices.Unsafe and
   System.Runtime.Extensions (as those are now facades).

4. Add empty lines to separate property and item groups from each other
   so that project files are easier to read and look the same as
   project files that "dotnet new" produces.

Commit 4 - React to analyzer warnings/errors

Projects which previously didn't auto-reference System.Memory,
do so now (projects which are out-of-band and sit on top of the
shared framework). Analyzers which are conditioned on the reference
being available now kick in and hence these two files needed to be
updated.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	area-Meta
Milestone:	7.0.0

[stephentoub]

Use the SDK's default items feature so that compile items in the project directory are imported automatically.

Did you confirm that there are zero changes in what code's actually being included? e.g. that the size of the binaries built before and after the changes are identical?

[ViktorHofer]

I did verify that the compile items passed to CSC/VBC are identical before and after but I think it's a good idea to also compare the produced assemblies. Let me do that.

[hoyosjs]

hmm, I thought we were pretty selective about source inclusions dependent on platforms (say with *.Unix.cs files and all), say with System.Private.Corelib. Are we sure this currently works? Or is there also some massaging in this PR to do that.

[ViktorHofer]

The third commit handles updating all projects to work with that setting. Less than 50% of our projects multi-target based on the OS, the majority doesn't. For the projects that don't multi-target based on the OS, this settings work nicely.

For the ones that multi-target based on the OS, this setting still works fine but a bit more work was required to make sure that nothing unexpected is included which previously wasn't. I recommend to look into the third commit yourself as the diff mostly speaks for itself.

[bartonjs]

FWIW, I really, really, really (insert a string of "really"s until your ears bleed) do not want us to use EnableDefaultItems.

I don't even want the SDK to support it.

It's bad engineering practice that's only good for bootstrapping CS101 students.

[stephentoub]

I agree with Jeremy (at least on the first part... I think EnableDefaultItems is reasonable for simple, small projects and is an ok default for the SDK to help bootstrap in general). If the overwhelming majority of others disagree, I'll acquiesce, but understanding exactly what's being included and what's not being included is really helpful. And with the variations in our builds and our need to include some files in some configurations and not in others, we're never going to get to a place where everything is just always included, which from my perspective also means the consistent thing is always specifying exactly what should be in every build.

[stephentoub]

(Also, for fundamental overhauls like this, that impact everyone working on the project, that impact every project, etc., I'd prefer if we have a discussion about the direction rather than an ~500 file diff just appearing. If it was necessary for you to do the work in order to come up with the proposal, then ok, and it being potentially throw-away is just part of the process; but if that could have been achieved just by experimenting with a couple of projects, presumably that would have resulted in less work for everyone?)

[danmoseley]

Explicit inclusions feels like it supports more predictable engineering at a relatively small cost of adding a line in XML when you create a file. Also, as someone who greps more than using an IDE, it allows me to see exactly what's going on without also look at what file is positioned where on disk.

[eerhardt]

My thinking is that the lower-level your code is, the more you are writing platform-specific code. And the more platform-specific code you write, the more you need to be explicit about what is and what isn't included in your project. I don't think it has anything to do with size of your project. dotnet/aspnetcore is by no means "small" or CS101, but they use EnableDefaultItems in all their projects.

The same can be said for using #ifs in your code. If you only need them in one or two spots, it is fine. But at the level we need to specialize code in the shared fx, it would be a nightmare.

That's why I think the current situation we have is acceptable. We have code that is both low-level (shared fx) and high-level (Microsoft.Extensions). Using the approach that best fits the project makes sense to me.

[radical]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ViktorHofer]

@stephentoub (cc @eerhardt) good that you were asking about the diff. I noticed that the Extensions projects which already use the EnableDefaultItems feature in main embed their Strings.resx file twice. One time because of the EnableDefaultItems feature (as expected) and a second time in resources.targets which results in the compiler receiving it twice:

/resource:C:\git\runtime5\artifacts\obj\Microsoft.Extensions.Configuration.Json\Debug\net7.0\FxResources.Microsoft.Extensions.Configuration.Json.SR.resources /resource:C:\git\runtime5\artifacts\obj\Microsoft.Extensions.Configuration.Json\Debug\net7.0\Microsoft.Extensions.Configuration.Json.Resources.Strings.resources

That fix which is part of the first commit makes the Extensions assemblies now actually smaller :)

[radical]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ghost]

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Commit 1 - Infrastructure changes

1. The generators.targets file had a bug that complained when a project
   uses the default targeting pack references because the
   ArrayMarshaller.cs file was not imported.
   I also cleaned the generators.targets file by simplifying
   conditions.
2. Enable "default items" via the <EnableDefaultItems /> SDK switch
   for any project except tests (for now).
3. Remove outdated infrastructure that isn't needed anymore based on
   project file changes.

Commit 2 - Delete dead code that dates back to the mono BCL build

Commit 3 - Ref/Src/Gen-Project file hygiene

1. Use the SDK's default items feature so that compile items in the
   project directory are imported automatically. Disable that feature
   in few projects which heavily depend on manually including compile
   items based on the TargetPlatformIdentifier.

   Changes made mimic the usage in the Microsoft.Extensions.* projects
   which already used the default items feature.

   I made sure that the numer of comile items included before the change
   and after the change is identical.

   In a follow-up change it would make sense to clean-up all the
   BCL files to use the same set of suffixes, i.e.

   • .NETFramework only files: .netframework.cs instead of
     • .net46.cs
     • .netfx.cs
     • .netframework.cs
     • .net48.cs
     • etc.

   That would allow to define central glob
   exclusion patterns for i.e. .NETFramework builds:

   • TFI .NETFramework: *\**.netcoreapp.cs
   • TFI .NETCoreApp: *\**.netframework.cs
   • ...

2. Remove the unnecessary declaration of <Reference /> and
   <ProjectReference /> items in out-of-band projects as they already
   build on top of the shared framework (targeting pack) and make that
   the default behavior for anything that is out-of-band.

3. Clean-up Reference and ProjectReference items that weren't used by
   the compiler anymore, i.e. System.Runtime.ComilerServices.Unsafe and
   System.Runtime.Extensions (as those are now facades).

4. Add empty lines to separate property and item groups from each other
   so that project files are easier to read and look the same as
   project files that "dotnet new" produces.

Commit 4 - React to analyzer warnings/errors

Projects which previously didn't auto-reference System.Memory,
do so now (projects which are out-of-band and sit on top of the
shared framework). Analyzers which are conditioned on the reference
being available now kick in and hence these two files needed to be
updated.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	area-Infrastructure-libraries
Milestone:	7.0.0

[ViktorHofer]

Resolved merge conflicts.

[ViktorHofer]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ViktorHofer]

Resolved merge conflicts.

[ViktorHofer]

/azp run runtime-wasm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ViktorHofer]

Spoke with @stephentoub offline and we agreed on that it's easier to review the changes by splitting them into separate logical units. Please hold reviewing this PR.

[ViktorHofer]

Thanks everyone for their comments. Just to clarify, I didn't intend to put up such a big change without an upfront discussion. As noted in #67650 (comment) I unfortunately mis-interpreted a comment when I tested enabling the default items feature on a single library to get feedback from the area owners.

I apologize here and now for the way this was perceived by some of you and want to emphasize that this wasn't my intention.

On the upside at least trying out the enable default items feature helped me find:

1. Dead code: Delete dead code #67648 and
2. Resources being embedded into assemblies twice for Microsoft.Extensions projects: Project file hygiene #67532 (comment)

I will make sure that the rest of the changes won't be lost but I will close these PRs to first have another discussion around these other changes to make sure that there isn't another disconnect in communication.

[stephentoub]

Thanks, @ViktorHofer. Sorry for any miscommunication on my part.