Conversation
…existing packages, add csproj editing for enabling packaging with comments
There was a problem hiding this comment.
Pull request overview
Updates winapp init to be more non-destructive for existing .NET project package versions, and adds an opt-in flow for installing the WinApp “dotnet run packaged” integration with associated csproj tweaks.
Changes:
- Prompt user to optionally install
Microsoft.Windows.SDK.BuildTools.WinAppand gate packaged-app-related csproj edits behind that choice. - Preserve existing top-level package versions found in the project (except always updating the WinApp integration package).
- Add csproj helpers to ensure
EnableMsixTooling=true, removeWindowsPackageType=None, and annotate PackageReferences with explanatory comments.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/winapp-CLI/WinApp.Cli/Services/WorkspaceSetupService.cs | Adds WinApp package opt-in prompt; preserves existing package versions; conditionally updates csproj and annotates packages. |
| src/winapp-CLI/WinApp.Cli/Services/IDotNetService.cs | Extends interface with new csproj-editing helpers. |
| src/winapp-CLI/WinApp.Cli/Services/DotNetService.cs | Implements csproj regex-based edits for MSIX properties and PackageReference annotations. |
| src/winapp-CLI/WinApp.Cli.Tests/FakeDotNetService.cs | Updates test fake to delegate new csproj-editing helper methods to the real implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Build Metrics ReportBinary Sizes
Test Results✅ 722 passed out of 722 tests in 402.7s (+4 tests, -7.3s vs. baseline) Test Coverage❌ 20.6% line coverage, 34.6% branch coverage · ✅ +0.2% vs. baseline CLI Startup Time41ms median (x64, Updated 2026-04-15 22:20:43 UTC · commit |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/winappCli/sessions/bc7a49b1-9b1a-4b9d-93c7-5e8a9c8be379 Co-authored-by: zateutsch <88554871+zateutsch@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/winappCli/sessions/3f70e57d-31a0-467f-8080-6ac6147c4902 Co-authored-by: zateutsch <88554871+zateutsch@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…ng tag Agent-Logs-Url: https://github.com/microsoft/winappCli/sessions/4cbc7abe-b7d7-4d7f-9044-60a9a065e326 Co-authored-by: zateutsch <88554871+zateutsch@users.noreply.github.com>
Applied in commit 5ad4595. |
nmetulev
left a comment
There was a problem hiding this comment.
After testing, this is what I've found:
- We don't actually need the buildtools package to be installed, we can skip that and remove the code adding the package or checking for it
- The true is not added if the winapp package is not available - it should be added regardless as long as the winappsdk package is in the csproj (whether we add it or not)
- The EnabledMsixTooling doesn't seem to work when the manifest is called appxmanifest.xml, but it works for Package.appxmanifest - we should name the generated manifest Package.appxmanifest to align with the conventions used in winui projects
- We should only add comments in the csproj for things we have changed - if we add the EnableMsixTooling propoerty, we should comment why it was added.
Co-authored-by: Nikola Metulev <nmetulev@users.noreply.github.com>
…m/microsoft/winappCli into zt/395-csproj-version-overwriting
…m/microsoft/winappCli into zt/395-csproj-version-overwriting
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
src/winapp-CLI/WinApp.Cli/Services/DotNetService.cs:1
- In the
false -> trueupdate path, the replacement logic drops indentation on the<EnableMsixTooling>line (the comment is indented viaindent, but the element itself is not). Separately, the insert path double-indents<EnableMsixTooling>becauseelementalready includes 4 spaces on the second line and the caller prefixes another\" \". Consider building the replacement/insert text so both the comment and element lines use the same detected indent, and avoid embedding indentation insideelementif the caller is also adding indentation. This keeps.csprojformatting stable and makes repeated runs/idempotency easier to reason about.
src/winapp-CLI/WinApp.Cli/Services/DotNetService.cs:1 - In the
false -> trueupdate path, the replacement logic drops indentation on the<EnableMsixTooling>line (the comment is indented viaindent, but the element itself is not). Separately, the insert path double-indents<EnableMsixTooling>becauseelementalready includes 4 spaces on the second line and the caller prefixes another\" \". Consider building the replacement/insert text so both the comment and element lines use the same detected indent, and avoid embedding indentation insideelementif the caller is also adding indentation. This keeps.csprojformatting stable and makes repeated runs/idempotency easier to reason about.
src/winapp-CLI/WinApp.Cli/Services/DotNetService.cs:1 - In the
false -> trueupdate path, the replacement logic drops indentation on the<EnableMsixTooling>line (the comment is indented viaindent, but the element itself is not). Separately, the insert path double-indents<EnableMsixTooling>becauseelementalready includes 4 spaces on the second line and the caller prefixes another\" \". Consider building the replacement/insert text so both the comment and element lines use the same detected indent, and avoid embedding indentation insideelementif the caller is also adding indentation. This keeps.csprojformatting stable and makes repeated runs/idempotency easier to reason about.
src/winapp-npm/package.json:1 - This PR changes the npm package
versionfrom1.0.0to a prerelease build string and also rewrites the JSON formatting (extra spacing/indentation). Neither is mentioned in the PR description, and the formatting change makes future diffs noisier. If the version bump is intentional, it should align with the repo’s release/versioning policy (and ideally be called out in the PR description); otherwise, consider reverting to the expected semver version and restoring conventionalpackage.jsonformatting.
src/winapp-npm/package.json:1 - This PR changes the npm package
versionfrom1.0.0to a prerelease build string and also rewrites the JSON formatting (extra spacing/indentation). Neither is mentioned in the PR description, and the formatting change makes future diffs noisier. If the version bump is intentional, it should align with the repo’s release/versioning policy (and ideally be called out in the PR description); otherwise, consider reverting to the expected semver version and restoring conventionalpackage.jsonformatting.
src/winapp-CLI/WinApp.Cli/Services/DotNetService.cs:1 - New
.csprojmutation behaviors were added (EnableMsixTooling insertion/update, WindowsPackageType removal, PackageReference comment annotation), but there are no direct unit tests validating their exact modifications and idempotency. Since this repo already has comprehensiveDotNetServiceTests, please add targeted tests that assert: (1)EnsureEnableMsixToolingAsyncinserts the property in the intended location and updatesfalsetotruewithout mangling indentation, (2)RemoveWindowsPackageTypeNoneAsyncremoves the element and doesn’t remove otherWindowsPackageTypevalues, and (3)AnnotatePackageReferencesAsyncadds comments only once and respects existing comments.
src/winapp-CLI/WinApp.Cli/Services/DotNetService.cs:1 - New
.csprojmutation behaviors were added (EnableMsixTooling insertion/update, WindowsPackageType removal, PackageReference comment annotation), but there are no direct unit tests validating their exact modifications and idempotency. Since this repo already has comprehensiveDotNetServiceTests, please add targeted tests that assert: (1)EnsureEnableMsixToolingAsyncinserts the property in the intended location and updatesfalsetotruewithout mangling indentation, (2)RemoveWindowsPackageTypeNoneAsyncremoves the element and doesn’t remove otherWindowsPackageTypevalues, and (3)AnnotatePackageReferencesAsyncadds comments only once and respects existing comments.
src/winapp-CLI/WinApp.Cli/Services/DotNetService.cs:1 - New
.csprojmutation behaviors were added (EnableMsixTooling insertion/update, WindowsPackageType removal, PackageReference comment annotation), but there are no direct unit tests validating their exact modifications and idempotency. Since this repo already has comprehensiveDotNetServiceTests, please add targeted tests that assert: (1)EnsureEnableMsixToolingAsyncinserts the property in the intended location and updatesfalsetotruewithout mangling indentation, (2)RemoveWindowsPackageTypeNoneAsyncremoves the element and doesn’t remove otherWindowsPackageTypevalues, and (3)AnnotatePackageReferencesAsyncadds comments only once and respects existing comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…m/microsoft/winappCli into zt/395-csproj-version-overwriting
Co-authored-by: Nikola Metulev <nmetulev@users.noreply.github.com>
Co-authored-by: Nikola Metulev <nmetulev@users.noreply.github.com>
…m/microsoft/winappCli into zt/395-csproj-version-overwriting
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
fixes #395
Prevents --use-defaults flag from overwriting the existing dependency versions in a csproj.
fixes #399
Now prompts users to install nuget packages, unless --use-defaults is specified
fixes #401
If a user opts in to installing the nuget package, init will now make sure that the proper properties are added/removed in the csproj to enable running packaged.