Skip to content

Conversation

MattKotsenas
Copy link
Member

@MattKotsenas MattKotsenas commented Jun 13, 2025

Description

Add xmldocs to the generated metadata files to prevent CS1591 warnings in projects that have documentation generation enabled.

To test the change, I added a reference to Verify. Verify adds an implicit using for Xunit, which then means dropping using Xunit throughout the project to avoid IDE0005 errors.

Fixes #9197

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@Copilot Copilot AI review requested due to automatic review settings June 13, 2025 02:14
@MattKotsenas MattKotsenas requested a review from mitchdenny as a code owner June 13, 2025 02:14
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 13, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 13, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds XML documentation to generated metadata classes to suppress CS1591 warnings and updates the test project to use Verify for implicit Xunit imports.

  • Inject XML doc comments into the generated .targets output for project metadata classes
  • Remove explicit using Xunit; directives now covered by Verify’s implicit usings
  • Add Verify.XunitV3 package reference to the test project

Reviewed Changes

Copilot reviewed 70 out of 70 changed files in this pull request and generated no comments.

File Description
src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.in.targets Added <summary> XML comments for generated metadata classes and properties
tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj Introduced Verify.XunitV3 package reference
tests/Aspire.Hosting.Tests/**/*.cs Removed redundant using Xunit; imports

@davidfowl
Copy link
Member

I'm looking at fixing this unnecessary using issue

@MattKotsenas
Copy link
Member Author

@davidfowl friendly ping here. Anything I can do to help get this in? Thanks so much!

@MattKotsenas MattKotsenas force-pushed the bugfix/9197 branch 3 times, most recently from 46f23ca to c2efb7a Compare July 8, 2025 03:13
@MattKotsenas
Copy link
Member Author

also /cc @mitchdenny as code owner. Thanks so much!

@MattKotsenas
Copy link
Member Author

I'm getting a failure only on Windows that looks like this:

  D:\a\aspire\aspire\.dotnet\sdk\10.0.100-preview.5.25277.114\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.DefaultItems.Shared.targets(152,5): error MSB4062: The "CheckForImplicitPackageReferenceOverrides" task could not be loaded from the assembly D:\a\aspire\aspire\.dotnet\sdk\10.0.100-preview.5.25277.114\Sdks\Microsoft.NET.Sdk\targets\..\tools\net10.0\Microsoft.NET.Build.Tasks.dll. Could not load file or assembly 'System.Runtime, Version=10.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified. Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [C:\Users\runneradmin\AppData\Local\Temp\.aspire-testsyaxh5e2w.2cv\App\App.csproj]

I haven't been able to root cause yet. It seems to either be that MSBuild build servers are hanging around with a different version of .NET 10 (less likely) or the local / arcade install isn't being picked up by the test. However, I'm using the same pattern as the test above (which looks to be disabled for unrelated reasons).

I feel bad trying to merge a PR without tests, however I'm not sure how best to proceed, so any help is appreciated.

@eerhardt
Copy link
Member

eerhardt commented Jul 8, 2025

I'm getting a failure only on Windows that looks like this:

I'm not sure what is going on here. @radical - any ideas? Is there a way we can diagnose this?

One idea is to try to get a binlog or add more tracing to the test to try to figure out what is the problem.

We could also try enabling the other MSBuild test and see if it fails for the same reason.

@MattKotsenas
Copy link
Member Author

A binlog is a good next step. Where / how do I determine the output path for the binlog so it gets packaged up along with the test results?

@radical
Copy link
Member

radical commented Jul 8, 2025

A binlog is a good next step. Where / how do I determine the output path for the binlog so it gets packaged up along with the test results?

artifacts/log/Debug should get picked up.

@MattKotsenas
Copy link
Member Author

I have a run with a binlog available here: https://github.com/dotnet/aspire/actions/runs/16152828326/job/45588227053?pr=9868

That makes the failure clear: we're running with a mix of the pre-installed SDK 9, and the local install of SDK 10:

image

However, it's unclear to me how these types of tests should work. I also see https://github.com/dotnet/aspire/blob/d554b8bf24bbb932fe752e41f96c2ff4043b1b23/tests/Shared/TemplatesTesting/BuildEnvironment.cs used elsewhere, but it's unclear if that's the right path for these tests as well. It might also be the case that the CI setup for the local SDK is incomplete, and rather than edit the test we should edit the pipeline?

@eerhardt, if you let me know how you'd like these types of tests set up, I'm happy to follow up.

@eerhardt
Copy link
Member

eerhardt commented Jul 8, 2025

Do you understand how we are getting 9.0 and 10.0 versions for the different MSBuild properties? This definitely shouldn't be happening. They should be consistent.

cc @radical

@radical
Copy link
Member

radical commented Jul 9, 2025

Do you understand how we are getting 9.0 and 10.0 versions for the different MSBuild properties? This definitely shouldn't be happening. They should be consistent.

cc @radical

DOTNET_ROOT is set to $repo/.dotnet, and system dotnet is being run based on the the PATH. I think that DOTNET_ROOT is confusing things. I'll add the .dotnet directory to PATH here. That should fix it.

@MattKotsenas
Copy link
Member Author

@radical , looks like even with your change, it's still using Process = "C:\Program Files\dotnet\dotnet.exe" to build, which results in the torn state

@MattKotsenas MattKotsenas requested a review from eerhardt July 10, 2025 14:48
@MattKotsenas
Copy link
Member Author

MattKotsenas commented Jul 10, 2025

@eerhardt , build is fixed now (thanks @radical!). Would you mind taking another look? Thanks!

@MattKotsenas MattKotsenas requested a review from eerhardt July 10, 2025 23:13
@MattKotsenas
Copy link
Member Author

@eerhardt , thanks for your feedback! I believe I've addressed all your comments. Would you mind taking another look? Thanks!

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution.

Will merge once CI passes.

@eerhardt eerhardt enabled auto-merge (squash) July 14, 2025 16:25
@eerhardt eerhardt merged commit 7c8b37c into dotnet:main Jul 14, 2025
275 of 276 checks passed
@MattKotsenas MattKotsenas deleted the bugfix/9197 branch July 15, 2025 03:13
radical added a commit to radical/aspire that referenced this pull request Jul 18, 2025
This was originally a part of dotnet#9868, and the fix stands on its own.

-----
Try setting the PATH in a os-aware way

(cherry picked from commit 08a4b70)
joperezr pushed a commit that referenced this pull request Jul 18, 2025
)

This was originally a part of #9868, and the fix stands on its own.

-----
Try setting the PATH in a os-aware way

(cherry picked from commit 08a4b70)
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated project metadata class produces CS1591 warnings if project generates documentation xml
4 participants