Fix xml intellisense generation

[carlossanlop]

Fixes https://github.com/dotnet/maintenance-packages/issues/139

Previously, we had the xmls commited to the repo, under project's src/ folder. Unfortunately, the way the infra was configured was causing these xml files to get overwritten on each build.

In this fix, these changes were made:

• The commited xml files were deleted.
  • Exception: System.Runtime.CompilerServices.Unsafe, because the xml cannot be generated from source
• Remove all the code that was manipulating the DocumentationFile property.
• Set the GenerateDocumentationFile property to true for source projects only.
  • A workaround was necessary for the SqlClient tests/ project because many of the csprojs do not end with any of the suffixes required to match the IsTestProject condition.

[carlossanlop]

I inspected the artifacts/bin folder and the nupkgs to verify the contents of each xml looked correct. Results:

Identical xml to last release:

• Microsoft.Bcl.HashCode

Same xml but APIs are in different order:

• Microsoft.IO.Redist

The xml contains more documented a few more APIs than before:

• System.Buffers
• System.Net.WebSockets.WebSocketProtocol
• System.Reflection.DispatchProxy
• System.Threading.Tasks.Extensions

The xml contains only resource strings:

• System.Json - Because none of its public APIs are documented in source

The previous xml file was empty, now it's correct:

• System.Memory
• System.Runtime.CompilerServices.Unsafe

The previous nupkg had no xml, now it's correct:

• System.Xml.XPath.XmlDocument

The two weird ones:

• System.Data.SqlClient - Seems to be splittings APIs depending on availability in the current target platform and TFM

  • net462 and netstandard2.0 files contain only 25% docs from before, only SR strings
  • net8.0-windows file contains 80% of original, contains also SR strings
  • net8.0-unix file contains 50% of original, contains also SR strings
  • The nupkg does not have -windows and -unix folders under libs/, so the files are also 25% docs from before, only SR strings
  • But nupkg runtimes//lib// do contain the full xml with 80% of original

• System.Numerics.Vectors - Similarly, the availability of APIs in xmls depend on TFM

  • net462 libs file contains only 50% docs from before, some APIs were collected, missing APIs like Vector4, but now contains SR strings
  • netstandard2.0 libs file has MORE docs than before (50% more)

[ericstj]

It sounds like you're switching to use compiler output as the source of truth?

The xml contains only resource strings: System.Json - Because none of its public APIs are documented in source

Seems like a regression, what's your plan to fix it?

The previous xml file was empty, now it's correct: System.Runtime.CompilerServices.Unsafe

I see that for this one you still copy from the project directory - which I'd expect is required because ILAsm won't emit anything.

System.Data.SqlClient

That's a problem. runtime/rid/lib (they are runtime only) will never be seen by the compiler so users never see those XML. We need accurate XML in the lib folder. I can see this being a problem for anywhere we use a PNSE.

System.Numerics.Vectors ... , missing APIs like Vector4

I think that one's forwarded on .NETFramework, so it's OK to be missing. Probably the old XML had too much. Depending on what's missing it might be OK. You could decide to enable the diagnostic that ensures public APIs are doc'ed

All those docs for SR strings are a real waste of space - it's a shame they'll be bloating the user-facing files.

What made you take this approach, rather than just fixing the checked-in XML? It sounds like you're giving yourself a larger set of bugs to go and chase down.

[carlossanlop]

Seems like a regression, what's your plan to fix it?

Sorry, I should've clarified: System.Json also didn't have any docs in its xml before. In other words, there is no regression. The APIs are simply not documented in triple slash comments. The before and after xml file is the same.

What made you take this approach, rather than just fixing the checked-in XML?

The checked-in xmls were getting overwritten by every build as reported in this bug: https://github.com/dotnet/maintenance-packages/issues/139 so I took this approach to do the right thing and generate the docs out of the source code and avoid the overwrite problem.

So there are only two things to address:

• System.Data.SqlClient runtime/rid/lib problem
• Enable diagnostic to avoid publishing resource strings as docs

[ericstj]

The checked-in xmls were getting overwritten by every build as reported in this bug: https://github.com/dotnet/maintenance-packages/issues/139 so I took this approach to do the right thing and generate the docs out of the source code and avoid the overwrite problem.

Not sure there's a clear "right thing" and "wrong thing" here. The smallest change would be to ship the docs we shipped before, and fix the mechanism you were using to package the checked in docs. Probably you would need a different way to do that inspired by what you do in runtime. By switching to compiler emitted docs you're adding a bit of risk - since you have no idea the quality of those docs. It could be OK -- but you'll need to evaluate case-by-case like you've already started doing.

System.Data.SqlClient runtime/rid/lib problem

I think that's the same problem that's blocking the use of c#-source-of-truth in runtime for platform specific and PNSE.

Enable diagnostic to avoid publishing resource strings as docs

That won't do it. I think you'd need to change the component that generates them. I think that's here: https://github.com/dotnet/arcade/blob/12f956787e1b8db30a6322c3fe24b10ac5dcab13/src/Microsoft.DotNet.Arcade.Sdk/src/GenerateResxSource.cs#L130

Enable diagnostic

I mentioned this as a way to ensure that your newly enabled C# generated docs were complete.

So there are only two things to address:

I'm counting 3 so far. I think you'd also want either library owners or doc writers to review the diff in the docs here.

IMO the "right thing to do" here for this repo -- where we are moving libraries we don't want to churn -- is to support their docs in the way similar to how were included before. If that was from a static file -- such as from the old intellisense package - then make that an option (not from the package, but a checked in file). If that was as a compiler output -- that should also be an option.

[carlossanlop]

Ok, makes sense to avoid the risk of introducing docs that are not the same.

So for those 3 problems, only one fix is needed:

• Fix the overwriting by bringing back the xmls, to actually fix the problem reported in #139
• The SqlClient problem should go away after bringing back their xml file
• If I fix the overwriting problem, then the resource strings should not get unexpectedly added where they were not in the xml before

[ericstj]

Yeah, I think that's the best course of action. While you're at it, consider what it would look like for a library to enable emitting the XML file - then you can do that case by case.