Skip to content

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jul 11, 2025

Fixes #12058 via a global property opt-in (to avoid a breaking change).
To be used by file-based apps: dotnet/sdk#49745

@jjonescz jjonescz marked this pull request as ready for review July 12, 2025 15:53
@Copilot Copilot AI review requested due to automatic review settings July 12, 2025 15:53
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

This PR adds support for building in-memory or non-existent projects by opt-in via a new global property, avoiding a breaking change.

  • Introduces a check for the BuildNonexistentProjectsByDefault global property to default to Build behavior
  • Adds equivalent logic in the backend intrinsic MSBuild task
  • Adds unit tests covering in-memory project scenarios with explicit and default behavior

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/Tasks/MSBuild.cs Checks BuildNonexistentProjectsByDefault to set skip behavior to Build
src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs Mirrors the same property check in the request builder
src/Tasks.UnitTests/MSBuild_Tests.cs Adds tests for explicit Build, explicit Error, and default Build scenarios
Comments suppressed due to low confidence (3)

src/Tasks/MSBuild.cs:300

  • [nitpick] The property name uses 'Nonexistent' in one word, while the enum SkipNonExistentProjectsBehavior uses 'NonExistent'. Consider aligning the casing and spelling (e.g., 'BuildNonExistentProjectsByDefault') for consistency.
                        .TryGetValue("BuildNonexistentProjectsByDefault", out var buildNonexistentProjectsByDefault) &&

src/Tasks/MSBuild.cs:299

  • Add XML documentation or inline comments for the new BuildNonexistentProjectsByDefault global property in the MSBuild task, explaining its purpose and usage.
                    else if (BuildEngine is IBuildEngine6 buildEngine6 && buildEngine6.GetGlobalProperties()

src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs:333

  • Add unit tests for the backend MSBuild intrinsic task to verify that BuildNonexistentProjectsByDefault correctly triggers SkipNonExistProjectsBehavior.Build in the request builder scenario.
                    else if (BuildEngine is IBuildEngine6 buildEngine6 && buildEngine6.GetGlobalProperties()

Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

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

the property should be documented in src\MSBuild\MSBuild\Microsoft.Build.CommonTypes.xsd

please address comments, otherwise looks good

@jjonescz jjonescz requested a review from JanProvaznik July 15, 2025 09:06
@jjonescz
Copy link
Member Author

the property should be documented in src\MSBuild\MSBuild\Microsoft.Build.CommonTypes.xsd

missed this comment, looking now

@JanProvaznik JanProvaznik self-assigned this Jul 15, 2025
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Approach, implementation, and tests look pretty good.

Could you please write up a speclet describing the change in our documentation/specs folder, from the MSBuild perspective? Or link to a larger file-based apps spec that mentions it in the PR description.

@JanProvaznik JanProvaznik merged commit 5d215b0 into dotnet:main Jul 16, 2025
9 checks passed
@jjonescz jjonescz deleted the virtual-msbuild branch July 16, 2025 14:51

## Alternatives Considered

1. **Always allow building in-memory projects**: This would be a breaking change as it could mask legitimate errors when projects are missing.
Copy link
Member

Choose a reason for hiding this comment

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

@jjonescz Curious more about this particular case -- I'd imagine that an in-memory project is an explicit enough of a decision for the user of MSBuild to create that this woudln't be an accident.

(not that I'm disagreeing with this direction; since this is an internal-ish handshake nothing means we can't change it later.)

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, the first part ("This would be a breaking change") is true anyway, but the second part ("it could mask legitimate errors when projects are missing") doesn't sound correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

MSBuild task should work on virtual projects
4 participants