Make Microsoft.Windows.SDK.BuildTools.WinApp safe for transitive consumption#513
Open
nmetulev wants to merge 4 commits into
Open
Make Microsoft.Windows.SDK.BuildTools.WinApp safe for transitive consumption#513nmetulev wants to merge 4 commits into
nmetulev wants to merge 4 commits into
Conversation
…umption Enables a library (e.g. a MAUI library wrapper) to re-export the package to its consumers via PrivateAssets="none", matching the dual-pack pattern used by Microsoft.WindowsAppSDK (build/ + buildTransitive/ mirrored 1:1). Hardening (build/Microsoft.Windows.SDK.BuildTools.WinApp.targets): * Introduce single computed gate _WinAppRunSupportActive based on: EnableWinAppRunSupport, WindowsPackageType != None, OutputType != Library, Windows TargetPlatformIdentifier (TargetPlatformIdentifier preferred over GetTargetPlatformIdentifier(TargetFramework) when set), and either a manifest in the project directory, an explicit WindowsPackageType=MSIX, or a custom WinAppManifestPath that exists. * OutputType=Exe is intentionally supported (samples/dotnet-app is a packaged console app via execution alias) - not gated to WinExe. * Replace per-target conditions with the single gate. Move the gate from the inner PropertyGroup to the outer Target Condition for _WinAppPrepareRunArguments so it no longer fires BeforeTargets= ComputeRunArguments in unrelated projects. * Apply the gate to the AppxManifest promotion ItemGroup, the design-time Compile inclusion, and the appxmanifest Content inclusion. Packaging (Microsoft.Windows.SDK.BuildTools.WinApp.csproj): * Pack build/ contents to both build/ and buildTransitive/ so direct and transitive consumers see the same MSBuild logic. * Remove DevelopmentDependency=true (it forces PrivateAssets=all on consumers, blocking transitive flow). Comment explains why. Tests (src/winapp-NuGet/tests/NuGet.Tests.ps1): * 12-case gate matrix using dotnet msbuild -getProperty: against synthesized csproj files, covering active scenarios (WinUI, packaged console, explicit MSIX, custom manifest path, MAUI Windows TFM) and inactive scenarios (libraries, no-manifest console, opt-outs, MAUI non-Windows TFMs, plain net8.0). * Package-shape parity test asserting build/ ↔ buildTransitive/ contain the same files. * All 14 cases pass. Docs: * README: split usage into direct (PrivateAssets=all recommended) vs transitive (PrivateAssets=none + IncludeAssets=build;buildTransitive) consumption with explanations of the gating guarantees. * docs/dotnet-run-support.md: detection logic section updated to describe the full _WinAppRunSupportActive gate; pipeline diagram label updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enables Microsoft.Windows.SDK.BuildTools.WinApp to flow MSBuild props/targets transitively (via buildTransitive/) while keeping them inert unless the consuming project is a packaged Windows app.
Changes:
- Introduces a single master gate property (
_WinAppRunSupportActive) and applies it across targets/item groups to prevent activation in non-applicable projects. - Packs identical
build/andbuildTransitive/contents and removesDevelopmentDependencyto allow transitive propagation. - Adds Pester coverage for gate behavior and nupkg layout parity; updates docs/README guidance for direct vs transitive consumption.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/winapp-NuGet/build/Microsoft.Windows.SDK.BuildTools.WinApp.targets | Adds _WinAppRunSupportActive and re-gates targets/items to be safe under buildTransitive/. |
| src/winapp-NuGet/Microsoft.Windows.SDK.BuildTools.WinApp.csproj | Packs build/ into buildTransitive/ and removes DevelopmentDependency to allow transitive flow. |
| src/winapp-NuGet/README.md | Documents direct vs transitive referencing patterns and expected gating behavior. |
| docs/dotnet-run-support.md | Updates detection logic description to reflect the new master gate. |
| src/winapp-NuGet/tests/NuGet.Tests.ps1 | Adds Pester tests for gate matrix and nupkg layout parity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 923 passed, 1 skipped out of 924 tests in 427.4s (-34.2s vs. baseline) Test Coverage❌ 23.2% line coverage, 38.9% branch coverage · ✅ no change vs. baseline CLI Startup Time44ms median (x64, Updated 2026-05-01 19:47:30 UTC · commit |
Code review fixes: - Tighten gate (Copilot #1, L1): Drop the standalone WindowsPackageType=='MSIX' activation clause and capture _WinAppManifestPathUserSet *before* the manifest auto-detection runs. The gate now only counts an explicitly user-set WinAppManifestPath, never an auto-detected output-dir path. This eliminates a class of subtle false-positives where a stale bin/AppxManifest.xml from a prior build would activate the gate on rebuild. - WinAppRunSupportInfo (L2): Print TargetPlatformIdentifier (raw), _WinAppEffectiveTargetPlatformIdentifier (used by gate), and _WinAppManifestPathUserSet so diagnostics mirror the actual gate inputs. - Doc/gate semantics alignment (Copilot #5): docs/dotnet-run-support.md now says "EnableWinAppRunSupport is true (the default)" instead of "is not set to false", matching the actual gate condition (== 'true'). Tests (Copilot #3, #4, M4): - Skip on non-Windows hosts via $IsWindows. - Add Get-GateValue parameters: -ProjectDirManifestName (named manifest filename), -TargetPlatformIdentifier (explicit TPI override). - Add new cases: project-dir Package.appxmanifest, project-dir AppxManifest.xml, explicit TargetPlatformIdentifier=windows on a non-windows TFM, explicit TargetPlatformIdentifier=android overriding a windows TFM. - Add regression case: WindowsPackageType=MSIX with no manifest must be inactive (replaces the previous active case). - Suite is now 18 tests, all passing. CI integration (M3): - scripts/build-cli.ps1 now invokes Invoke-Pester against src/winapp-NuGet/tests/NuGet.Tests.ps1 immediately after package-nuget.ps1 succeeds (skipped if -SkipTests is passed). Honors $FailOnTestFailure. Docs (M1, L3, M2): - README: replace stale Version="0.1.10" pins with <latest> placeholder + link to nuget.org. - README: new "Behavior changes" section explaining the DevelopmentDependency removal so direct consumers know to add PrivateAssets="all" if they don't want the package to flow into their own .nupkg output. - README: new troubleshooting entry "dotnet run is not intercepted" pointing users at `dotnet msbuild -t:WinAppRunSupportInfo` and listing the five gate discriminators. CI failures on PR #513 (build-and-package, electron) were pre-existing on main (E2E_DotNetApp_PackageShouldIncludeRuntimeDependency) or flaky/unrelated (electron C# native addon build) and are not caused by this PR. The four .NET-using sample jobs (dotnet-app, wpf-app, cpp-app, packaging-cli) all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
chiaramooney
approved these changes
May 1, 2026
mattleibow
reviewed
May 12, 2026
| Exists('$(MSBuildProjectDirectory)\Package.appxmanifest') | ||
| Or Exists('$(MSBuildProjectDirectory)\AppxManifest.xml') | ||
| Or Exists('$(MSBuildProjectDirectory)\appxmanifest.xml') | ||
| Or ('$(_WinAppManifestPathUserSet)' == 'true' And Exists('$(WinAppManifestPath)')) |
Member
There was a problem hiding this comment.
there are 6 conditional sets of WinAppManifestPath to fist check $(OutputPath) then $(MSBuildProjectDirectory). However, in the condition in _WinAppRunSupportActive there is just the latter 3.
In a MAUI app, we generate the manifest based on the platform and various msbuild props, so it only exists in the output. This condition is skipped here. If I just check Exists('$(WinAppManifestPath)') then it works. But I am not sure what else I break.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes
Microsoft.Windows.SDK.BuildTools.WinAppsafe for transitive consumption so a library author (e.g., the MAUI Windows wrapper) can take aPackageReferenceon it and have the targets flow through to consumer head apps — without having to ask every consumer to add their ownPackageReference.What changes
Targets hardening (
build/Microsoft.Windows.SDK.BuildTools.WinApp.targets)Introduces a single computed master gate
_WinAppRunSupportActivethat all gated targets/items now reference instead of repeating long conditions. The gate requires all of:EnableWinAppRunSupport != false(existing opt-out preserved)_WinAppEffectiveTargetPlatformIdentifier == Windows(skips iOS/Android/Mac TFMs in MAUI multi-target)OutputType != Library(head apps only —Exe,WinExe, etc.)WindowsPackageType != None(skip unpackaged WinUI apps)WinAppManifestPathis set and the file exists, or auto-detection finds one_WinAppEffectiveTargetPlatformIdentifierprefers$(TargetPlatformIdentifier)if explicitly set, falls back to[MSBuild]::GetTargetPlatformIdentifier($(TargetFramework)).The gate is also applied to the
<AppxManifest>ItemGroup, the design-time<Compile>inclusion, and the<Content Include="appxmanifest.xml">ItemGroup so they no longer fire in unrelated projects.Packaging (
Microsoft.Windows.SDK.BuildTools.WinApp.csproj)<None Include="build\**\*" PackagePath="buildTransitive" />so the targets are mirrored intobuildTransitive/(verified in the produced nupkg: 2 files inbuild/, identical 2 inbuildTransitive/).<DevelopmentDependency>true</DevelopmentDependency>— this was forcing consumers to default toPrivateAssets=all, which blocks transitive flow entirely.Docs
src/winapp-NuGet/README.md: split Usage into "Direct consumption (head app)" withPrivateAssets="all"recommended, and "Transitive consumption (library author)" withPrivateAssets="none" IncludeAssets="build;buildTransitive".docs/dotnet-run-support.md: full 5-condition gate explanation in the Detection Logic section.Tests (
src/winapp-NuGet/tests/NuGet.Tests.ps1)New 14-case Pester suite (12 gate matrix + 2 nupkg layout). Runs
dotnet msbuild -getProperty:_WinAppRunSupportActiveagainst synthetic csprojs (Library / Exe / WinExe × manifest yes/no ×EnableWinAppRunSupport=false× non-Windows TFM ×WindowsPackageType=None× custom manifest path missing). All 14 pass.Validation
End-to-end smoke tests against the locally produced
0.3.1-nm-winapp-nuget-buildtransitive.10.nupkg:samples/dotnet-appExetrue✅samples/wpf-appWinExe(UseWPF)true✅MyLibLibraryfalse✅HeadExe(no manifest)Exefalse✅HeadExe(with manifest)Exetrue✅ConsumerLibLibraryfalse✅Inspected
obj/HeadExe.csproj.nuget.g.targetsand confirmed NuGet auto-importsbuildTransitive/Microsoft.Windows.SDK.BuildTools.WinApp.targetsinto the consumer head app even though it never directly references the package — proves the MAUI scenario works.samples/dotnet-app(OutputType=Exe) was the specific reason the gate usesOutputType != Libraryrather thanOutputType == WinExe— gating onWinExewould have silently broken our own packaged-console-app sample.Risks / behavior changes
DevelopmentDependencyremoval is a behavior change for direct consumers using defaultPrivateAssets. They will now see the package flow transitively unless they setPrivateAssets="all"themselves. README updated to recommendPrivateAssets="all"for direct head-app consumption.build/andbuildTransitive/contain the same files; for direct consumers NuGet importsbuild/(withbuildTransitive/as fallback), so no double-import. This matches WindowsAppSDK's pattern at scale.Not covered by automated tests yet
_WinAppEffectiveTargetPlatformIdentifier != 'Windows', which is straightforward and tested at the property-evaluation level, but a workload-installed MAUI build would be the strongest signal. Recommend coordinating with Matthew for a real-world try.