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

7.0 source-built SDK packs are missing missing xml doc #2877

Closed
MichaelSimons opened this issue May 24, 2022 · 15 comments · Fixed by dotnet/runtime#75981
Closed

7.0 source-built SDK packs are missing missing xml doc #2877

MichaelSimons opened this issue May 24, 2022 · 15 comments · Fixed by dotnet/runtime#75981
Assignees
Labels
area-infra Source-build infrastructure and reporting blocking-release
Milestone

Comments

@MichaelSimons
Copy link
Member

The XmlDocTests caught a regression in 7.0 where it appears that all xml doc for the packs is missing.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@MichaelSimons
Copy link
Member Author

In speaking w/@crummel, he thinks this may be caused by not picking up the SBRP versions of the packs which are the only ones with the XML doc.

@MichaelSimons MichaelSimons added area-infra Source-build infrastructure and reporting and removed untriaged labels May 26, 2022
@oleksandr-didyk
Copy link
Contributor

oleksandr-didyk commented Jul 25, 2022

General overview:
Certain target packs that should contain .xml docs in the produced source-build do not contain any in 7.0.

The origin of the issue should be identified and, if plausible, fixed. The at-the-time-of-writing XmlDocTests disabled test should be updated and re-enabled to allow for catching of any similar issues in the future.

Actions to take:

  • verify that source-build does indeed differ in .xml doc content compared to MS build. If it does not - the issue should be pushed to other product teams up the chain
  • identify the reason for the issue and restore the .xml docs to the build
  • update the baseline for XmlDocTests and enable it for CI and local test runs

@oleksandr-didyk
Copy link
Contributor

I verified the available target pack .xml docs between the source-built and the default built product and found no differences.

I also compared the existing missing docs baseline that the test in question uses with the contents of the target packs and found 3 missing .xml doc files:

  • Microsoft.NETCore.App.Ref\ref\netx.y\System.Net.Quic.xml
  • Microsoft.NETCore.App.Ref\ref\netx.y\System.Runtime.InteropServices.JavaScript.xml
  • Microsoft.NETCore.App.Ref\analyzers\dotnet\cs\Microsoft.Interop.JavaScript.JSImportGenerator.xml

@MichaelSimons could you please advise on the person I should refer this issue to? Thanks

Note: the dotnet/installer commit I used to build the product is 549b7d641e2f5e90ae284c3154b04cf48aa1b68f

@MichaelSimons
Copy link
Member Author

@oleksandr-didyk, Hmmm, that doesn't match what I see with a quick inspection. Let me clarify what steps I think should be taken and let me know how/if that differs from what you did. These are the steps the test is doing.

  1. Grab a source-built SDK from CI or build one yourself.
  2. Install the MSFT built SDK for the same commit sha or one that is close.
  3. Retrieve a list of the XML doc files included with the Microsoft.NETCore.App.Ref and Microsoft.AspNetCore.App.Ref packs directory of both the MSFT and SB builds
  4. Diff the two lists - there should be no differences. I expect that you will find that most of the Microsoft.NETCore.App.Ref xml doc is missing.

The baseline file for the test represents the list of assemblies that do not container xml doc. The reason the test stores this as a baseline is so that it can avoid having to retrieve the MSFT build to compare against. This makes the test more efficient as well as supporting the ability to run in an offline environment.

@oleksandr-didyk
Copy link
Contributor

I think I probably described the findings in a non-clear way, resulting in us miss-understanding each other. What I meant is that I got the same result as in point 4 -> the MSFT and source-built SDK do not differ in targeting packs doc content. All of the docs form the baseline are missing as you mentioned + I found additional 3 .dlls that are not stated in the baseline but also don't have a doc (the ones listed in my original comment)

@MichaelSimons
Copy link
Member Author

Ah that makes sense. Please work with @crummel on this as had a speculation as why this was happening.

@oleksandr-didyk
Copy link
Contributor

Pinging @ViktorHofer as suggested by @crummel -> there seem to be missing docs in targeting packs that are missing from both MSFT build and source-build. Could you please advise on if this is something that runtime could take care? Thanks

@ViktorHofer
Copy link
Member

I will take a look

@ViktorHofer
Copy link
Member

ViktorHofer commented Aug 21, 2022

Sorry, because of conflicting priorities I hadn't had a chance to take a look at this yet.

@tkapin
Copy link
Member

tkapin commented Sep 5, 2022

Hey @ViktorHofer, I understand you are busy with .NET7 release date approaching, but could you give us at least an ETA when do you think you'd be able to look into this? Cheers!

@ViktorHofer ViktorHofer added this to the .NET 7.0 milestone Sep 6, 2022
@ViktorHofer
Copy link
Member

I just spoke offline with @ericstj about this and based on @oleksandr-didyk's latest comment this doesn't seem to be a source build issue but a more general case of xml files missing from the targeting pack. One observation: We don't think that source generators must produce xml files as those aren't referenced by the consumer's app but by the compiler itself. So whatever component/test complains about them missing from the targeting pack, might need an update to ignore them.

@carlossanlop can you please help with this request as you are the Intellisense expert on our team? The gist is that some of our assemblies in the targeting pack don't have a corresponding xml file. A list of identified files was shared above.

We want to look at this sooner than later as it impacts both .NET 7 and main (hence I added the blocking-release label).

@MichaelSimons
Copy link
Member Author

this doesn't seem to be a source build issue but a more general case of xml files missing from the targeting pack.

If I look at the Microsoft targeting pack for rc1 (e.g. Microsoft.NETCore.App.Ref), I see it contains all of the xml doc. I don't see this in the latest source built sdk

We don't think that source generators must produce xml files as those aren't referenced by the consumer's app but by the compiler itself

I am expecting users of a source-build SDK would have intellisense documentation when utilizing the source-built targeting packs.

The gist is that some of our assemblies in the targeting pack don't have a corresponding xml file.

Source build is missing all of xml doc for Microsoft.NETCore.App.Ref. It is present for Microsoft.AspNetCore.App.Ref.

@MichaelSimons
Copy link
Member Author

The issue here is that dotnet/runtime#59937 was not backported from 6.0 to main/7.0. @carlossanlop is going to backport the PR and I will update the text-only source-build-reference-package to the be latest.

@ViktorHofer
Copy link
Member

Perfect. That was my assumption as well when I took a look with Carlos earlier today. It would be cool if we could avoid this prebuilt entirely by producing these Intellisense files (and the package) ourselves in dotent/runtime. I think we have a tracking issue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infra Source-build infrastructure and reporting blocking-release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants