Migrate to Uno.Sdk - #305
MartinZikmund wants to merge 30 commits into
Conversation
b7fd380 to
5290b8f
Compare
25e6b65 to
e673abd
Compare
041c863 to
4fcc0c4
Compare
4fcc0c4 to
3d1928d
Compare
^ Conflicts: ^ .github/workflows/build.yml
There was a problem hiding this comment.
Pull request overview
This PR migrates the tooling “project heads” to use Uno.Sdk (primarily for WinUI 3 / Uno 6.x), updating build scripts and CI to generate/build the new Uno head alongside existing multi-target heads.
Changes:
- Adds new Uno.Sdk-based heads for AllComponents and SingleComponent (desktop/wasm/android/ios scaffolding).
- Updates Uno package/version management to support Uno 5.x for WinUI 2 and Uno 6.x for WinUI 3 via conditional props.
- Updates generation scripts and GitHub Actions workflows to include/build the Uno.Sdk head and install required workloads.
Reviewed changes
Copilot reviewed 54 out of 63 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ProjectHeads/SingleComponent/Wasm/Program.cs | Updates WinAppSDK WASM entrypoint to use UnoPlatformHostBuilder. |
| ProjectHeads/SingleComponent/Uno/ProjectTemplate.Uno.csproj | Adds new SingleComponent Uno.Sdk head project. |
| ProjectHeads/SingleComponent/Uno/Platforms/WebAssembly/WasmScripts/AppManifest.js | Adds Uno WASM app manifest (display name). |
| ProjectHeads/SingleComponent/Uno/Platforms/WebAssembly/Program.cs | Adds Uno WASM entrypoint for SingleComponent head. |
| ProjectHeads/SingleComponent/Uno/Platforms/WebAssembly/manifest.webmanifest | Adds web manifest for WASM PWA metadata. |
| ProjectHeads/SingleComponent/Uno/Platforms/WebAssembly/LinkerConfig.xml | Adds linker descriptor for WASM trimming behavior. |
| ProjectHeads/SingleComponent/Uno/Platforms/iOS/PrivacyInfo.xcprivacy | Adds iOS privacy manifest. |
| ProjectHeads/SingleComponent/Uno/Platforms/iOS/Media.xcassets/LaunchImages.launchimage/Contents.json | Adds iOS launch image asset metadata. |
| ProjectHeads/SingleComponent/Uno/Platforms/iOS/Main.iOS.cs | Adds iOS entry point using UnoPlatformHostBuilder. |
| ProjectHeads/SingleComponent/Uno/Platforms/iOS/Info.plist | Adds iOS app plist. |
| ProjectHeads/SingleComponent/Uno/Platforms/iOS/Entitlements.plist | Adds iOS entitlements placeholder. |
| ProjectHeads/SingleComponent/Uno/Platforms/Desktop/Program.cs | Adds desktop entrypoint enabling platform hosts (X11/Win32/etc.). |
| ProjectHeads/SingleComponent/Uno/Platforms/Android/Resources/values/Styles.xml | Adds Android styles scaffolding. |
| ProjectHeads/SingleComponent/Uno/Platforms/Android/Resources/values/Strings.xml | Adds Android strings scaffolding. |
| ProjectHeads/SingleComponent/Uno/Platforms/Android/MainActivity.Android.cs | Adds Android activity scaffold for Uno. |
| ProjectHeads/SingleComponent/Uno/Platforms/Android/Main.Android.cs | Adds Android application entry scaffold for Uno. |
| ProjectHeads/SingleComponent/Uno/Platforms/Android/environment.conf | Adds Mono GC tuning for Android. |
| ProjectHeads/SingleComponent/Uno/Platforms/Android/Assets/AboutAssets.txt | Adds Android assets readme. |
| ProjectHeads/SingleComponent/Uno/Platforms/Android/AndroidManifest.xml | Adds Android manifest scaffold. |
| ProjectHeads/SingleComponent/Uno/Assets/Splash/splash_screen.svg | Adds splash asset for Uno head. |
| ProjectHeads/SingleComponent/Uno/Assets/SharedAssets.md | Adds shared assets documentation. |
| ProjectHeads/SingleComponent/Uno/Assets/Icons/icon.svg | Adds app icon background asset. |
| ProjectHeads/SingleComponent/Uno/Assets/Icons/icon_foreground.svg | Adds app icon foreground asset. |
| ProjectHeads/GenerateSingleSampleHeads.ps1 | Adds -IncludeUnoSdkHead support for generation. |
| ProjectHeads/App.Head.Wasm.props | Adjusts wasm head output path behavior; aligns Uno adapter version selection. |
| ProjectHeads/App.Head.Uno.WinUI.Dependencies.props | Updates Uno.WinUI Lottie dependency to 6.x line. |
| ProjectHeads/App.Head.Uno.UI.Dependencies.props | Removes trailing blank lines. |
| ProjectHeads/App.Head.Uno.props | Makes dependency variant and dependency imports conditional on WinUI major version. |
| ProjectHeads/AllComponents/Wasm/Program.cs | Updates WinAppSDK WASM entrypoint to use UnoPlatformHostBuilder. |
| ProjectHeads/AllComponents/Uno/Platforms/WebAssembly/WasmScripts/AppManifest.js | Adds Uno WASM app manifest (display name). |
| ProjectHeads/AllComponents/Uno/Platforms/WebAssembly/Program.cs | Adds Uno WASM entrypoint for AllComponents head. |
| ProjectHeads/AllComponents/Uno/Platforms/WebAssembly/manifest.webmanifest | Adds web manifest for WASM PWA metadata. |
| ProjectHeads/AllComponents/Uno/Platforms/WebAssembly/LinkerConfig.xml | Adds linker descriptor for WASM trimming behavior. |
| ProjectHeads/AllComponents/Uno/Platforms/iOS/PrivacyInfo.xcprivacy | Adds iOS privacy manifest. |
| ProjectHeads/AllComponents/Uno/Platforms/iOS/Media.xcassets/LaunchImages.launchimage/Contents.json | Adds iOS launch image asset metadata. |
| ProjectHeads/AllComponents/Uno/Platforms/iOS/Main.iOS.cs | Adds iOS entry point using UnoPlatformHostBuilder. |
| ProjectHeads/AllComponents/Uno/Platforms/iOS/Info.plist | Adds iOS app plist. |
| ProjectHeads/AllComponents/Uno/Platforms/iOS/Entitlements.plist | Adds iOS entitlements placeholder. |
| ProjectHeads/AllComponents/Uno/Platforms/Desktop/Program.cs | Adds desktop entrypoint enabling platform hosts (X11/Win32/etc.). |
| ProjectHeads/AllComponents/Uno/Platforms/Android/Resources/values/Styles.xml | Adds Android styles scaffolding. |
| ProjectHeads/AllComponents/Uno/Platforms/Android/Resources/values/Strings.xml | Adds Android strings scaffolding. |
| ProjectHeads/AllComponents/Uno/Platforms/Android/MainActivity.Android.cs | Adds Android activity scaffold for Uno. |
| ProjectHeads/AllComponents/Uno/Platforms/Android/Main.Android.cs | Adds Android application entry scaffold for Uno. |
| ProjectHeads/AllComponents/Uno/Platforms/Android/environment.conf | Adds Mono GC tuning for Android. |
| ProjectHeads/AllComponents/Uno/Platforms/Android/Assets/AboutAssets.txt | Adds Android assets readme. |
| ProjectHeads/AllComponents/Uno/Platforms/Android/AndroidManifest.xml | Adds Android manifest scaffold. |
| ProjectHeads/AllComponents/Uno/CommunityToolkit.App.Uno.csproj | Adds new AllComponents Uno.Sdk head project. |
| ProjectHeads/AllComponents/Uno/Assets/Splash/splash_screen.svg | Adds splash asset for Uno head. |
| ProjectHeads/AllComponents/Uno/Assets/SharedAssets.md | Adds shared assets documentation. |
| ProjectHeads/AllComponents/Uno/Assets/Icons/icon.svg | Adds app icon background asset. |
| ProjectHeads/AllComponents/Uno/Assets/Icons/icon_foreground.svg | Adds app icon foreground asset. |
| MultiTarget/UseUnoWinUI.ps1 | Simplifies WinUI swap behavior (stops rewriting package IDs by string replace). |
| MultiTarget/Test-Component-Support.ps1 | Updates WinUI 3 multi-target support list. |
| MultiTarget/PackageReferences/Uno.props | Splits Uno.UI vs Uno.WinUI versions and conditions; adjusts wasm/devserver references. |
| MultiTarget/AvailableTargetFrameworks.props | Whitespace cleanup. |
| global.json | Registers Uno.Sdk as an MSBuild SDK reference. |
| GenerateVSCodeLaunchConfig.ps1 | Adds a separate VS Code launch config entry for Uno heads. |
| GenerateSingleSolution.ps1 | Adds -IncludeUnoSdkHead and skips classic WASM head when Uno.Sdk head is included. |
| GenerateAllSolution.ps1 | Adds -IncludeUnoSdkHead and includes Uno.Sdk head for all-components solution generation. |
| CommunityToolkit.App.Shared/Renderers/Markdown/MarkdownTextBlock.cs | Alters Uno WASM polyfill compilation conditions for Markdown rendering. |
| Build-Toolkit-Gallery.ps1 | Whitespace cleanup. |
| Build-Toolkit-Components.ps1 | Whitespace cleanup. |
| .github/workflows/build.yml | Adds workload installs, copies global.json to root, adds Uno.Sdk CI coverage (Windows matrix + Linux Uno builds). |
Comments suppressed due to low confidence (1)
CommunityToolkit.App.Shared/Renderers/Markdown/MarkdownTextBlock.cs:23
- The Uno WASM MarkdownTextBlock polyfill is now excluded for WINUI3 builds. For Uno.WinUI WebAssembly (the new Uno.Sdk head), this will fall back to the non-WASM implementation (a plain TextBlock), which prevents markdown from being rendered.
#if HAS_UNO_WASM && !WINUI3
using Markdig;
using Uno.Foundation.Interop;
using Uno.UI.Runtime.WebAssembly;
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if HAS_UNO_WASM && !WINUI3 | ||
| [HtmlElement("div")] | ||
| public partial class MarkdownTextBlock : TextBlock | ||
| { |
There was a problem hiding this comment.
WINUI3 in the WCT is defined for all multitargets that support it, whether it's Uno or native.
Although, a potential bigger issue here-- this change would disable the MarkdownTextBlock WASM polyfill for WinUI 3 on WASM.
This is fine if there's an actionable reason why, but there's no comment explaining this strange choice.
There was a problem hiding this comment.
We can go ahead and apply the suggested change here, but we need to make sure it's applied consistently everywhere this change was made, not just this one code location.
| `"args`": [ | ||
| `"run`", | ||
| `"build`", | ||
| `"/r`", | ||
| `"/p:UnoSourceGeneratorUseGenerationHost=true`", | ||
| `"/p:UnoSourceGeneratorUseGenerationController=false`", | ||
| `"/p:UnoRemoteControlPort=443`", | ||
| `"--project=`$`{workspaceFolder`}/components/$projectName/heads/Uno/$projectName.Uno.csproj`" | ||
| ], |
| - name: Install .NET workloads | ||
| run: dotnet workload install wasm-tools | ||
|
|
There was a problem hiding this comment.
Could this be done by uno-check instead of manually installing the workload? We usually rely on that one tool to define and install workloads to keep things clean and easy to manage.
| - name: Install .NET workloads | ||
| if: ${{ matrix.multitarget == 'wasm' || matrix.multitarget == 'unosdk' }} | ||
| run: dotnet workload install wasm-tools ${{ matrix.multitarget == 'unosdk' && 'android ios maui' || '' }} | ||
|
|
| <linker> | ||
| <assembly fullname="CommunityToolkit.App.Uno" /> | ||
| </linker> |
| <IsPackable>false</IsPackable> | ||
| <UnoSingleProject>true</UnoSingleProject> | ||
|
|
||
| <!-- Platform flags for project reference conditions --> | ||
| <IsDeployableHead>true</IsDeployableHead> | ||
| <IsUno>true</IsUno> | ||
| <IsWasm>true</IsWasm> | ||
| <HasWinUI>true</HasWinUI> | ||
| <WinUIMajorVersion>3</WinUIMajorVersion> | ||
| <IsSingleExperimentHead>true</IsSingleExperimentHead> | ||
| <DependencyVariant>WinUI</DependencyVariant> | ||
|
|
||
| <DefineConstants>$(DefineConstants);WINUI3</DefineConstants> |
Previously CommunityToolkit.App.Uno.csproj (and the SingleComponent template) hardcoded TargetFrameworks to all four platforms (desktop/browserwasm/android/ios) regardless of what -MultiTargets was requested at solution-gen time, and forced every component ProjectReference to resolve at bare net9.0 regardless of which of the head's own TargetFrameworks was building. Requesting e.g. -MultiTargets android still produced wasm/desktop deploy options pointing at component builds that were never produced, and the head's own android/ios builds silently referenced components' shared net9.0 output instead of their net9.0-android/net9.0-ios build. - TargetFrameworks is now computed from MultiTarget/EnabledMultiTargets.props (the same file UseTargetFrameworks.ps1 already writes for every other head), so an unrequested platform never gets an orphaned entry. net9.0-desktop rides along whenever wasm/wpf/linuxgtk is requested, since it needs the same shared net9.0 component build. - SetTargetFramework on the component ProjectReference now maps to the head's own $(TargetFramework) for android/ios instead of a blanket net9.0. - The head's IsWasm/IsDroid/IsiOS platform flags (which gate whether Generated/*.Samples.props and *.Source.props even reference a given component) were hardcoded IsWasm=true unconditionally and never set IsDroid/IsiOS at all, so android/ios builds referenced zero components regardless of the two fixes above. These are now computed per the head's own TargetFramework. Verified: default (-MultiTargets wasm) still builds net9.0-desktop and net9.0-browserwasm clean; -MultiTargets android now builds net9.0-android end-to-end (previously failed with CS0103 on ToolkitSampleRegistry, since zero components were ever referenced); -MultiTargets ios correctly narrows to net9.0-ios only, excluding desktop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017mBuXiEpiN6B1xFD7gYUAM
Previously the Uno.Sdk head's net9.0-desktop TFM rode along with the
net-common group (wasm/wpf/linuxgtk) and borrowed IsWasm=true to pick up
components. That meant desktop couldn't be requested independently -- asking
for e.g. -MultiTargets wasm always dragged the desktop head along, and there
was no way to ask for only the desktop head.
'desktop' is now a first-class MultiTarget:
- Selecting it auto-selects wpf + linuxgtk (the shared net9.0 desktop
component surface) so the head has components to reference. This happens at
the generation funnels (UseTargetFrameworks, GenerateAllProjectReferences,
Test-Component-Support), so NO component MultiTarget.props changes are
needed -- every desktop-capable component already declares wpf;linuxgtk.
- The head keys its net9.0-desktop TFM off EnabledMultiTargets.Contains('desktop')
and presents that TFM as IsWpf (not IsWasm), so reference gating, sample-doc
filtering, and MultiTargetIdentifier all reuse the existing wpf surface.
IsWpf's constants (HAS_UNO_SKIA/__SKIA__) are also more accurate for a Skia
desktop head than the previous __WASM__.
- AddMultiTargetCompatibleSampleDocs.targets gains wpf/linuxgtk
MultiTargetIdentifier cases so IsWpf yields a valid identifier.
- Test-Component-Support restores wpf/linuxgtk to the WinUI-3 supported set
(Uno 6 dropped the Skia WPF/GTK *packages*, but the desktop *surface* they
represented is now served by the unified Uno.Sdk desktop head). Verified
no-op for the current component set: every wpf/linuxgtk component also
declares wasm, so no package surface changes.
- CI uno-linux and GenerateAllSolution's default now request desktop explicitly.
Verified: -MultiTargets desktop -> only net9.0-desktop, builds end-to-end with
real component references (previously would have referenced zero components);
-MultiTargets wasm -> only net9.0-browserwasm (no desktop ride-along);
-MultiTargets wasm,desktop -> both; -MultiTargets android still narrows to
net9.0-android; default gallery builds both browserwasm + desktop clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017mBuXiEpiN6B1xFD7gYUAM
'unosdk' was never a MultiTarget - it existed only to flip -IncludeUnoSdkHead for the winui:3, multitarget:wasm combination, which meant matrix.multitarget held a value that no other part of the tooling recognises. The Uno.Sdk head is now a flag on the wasm/winui3 job instead: the fake entry and its two exclusions are gone, and an include: entry merges unoSdkHead: true into the existing combination. Steps gate on matrix.unoSdkHead rather than comparing matrix.multitarget against a value the MultiTarget system would reject. Same four jobs as before (uwp/2, wasm/2, wasdk/3, wasm/3+UnoSdk), and -MultiTargets no longer needs the unosdk -> wasm translation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111vdg6YNSNuoVkGdHyr2eX
'desktop' was a single bulk MultiTarget standing in for every desktop OS at once, so a consumer could not ask for (or a component declare) just one of them - the level of control the MultiTarget system exists to provide. The name also said nothing about which TFMs or packages it selected. It is replaced by three targets named after the platforms they run on: - macos - already existed. - linux - renamed from 'linuxgtk'. GTK is a WinUI 2 implementation detail, not the platform, and it disappears entirely under Uno 6. - win32 - new, complementing 'wpf' the same way linux complements gtk. On WinUI 3 all three map to Uno 6's single cross-platform net9.0-desktop head, which is why they can share one head while staying separately requestable. On WinUI 2 they keep the Uno.UI heads they always had. Consequences: - Test-Component-Support's WinUI sets now reflect reality: 'wpf' is WinUI 2 only again (Uno 6 dropped Uno.UI.Skia.Wpf), and win32/linux/macos are the WinUI 3 desktop surface. The previous commit had put wpf/linuxgtk in the WinUI 3 set to feed the 'desktop' head. - The auto-select shims that 'desktop' needed in UseTargetFrameworks, GenerateAllProjectReferences and Test-Component-Support are gone; the three targets stand on their own. - The Uno.Sdk head declares net9.0-desktop when any of win32/linux/macos is requested, and flags that TFM as IsDesktop (Uno.Sdk's own property) rather than borrowing IsWpf, which was also defining WINDOWS_WPF on a build that runs on Linux and macOS. - Project reference generation gained CanTargetWin32 plus an IsDesktop clause covering the trio, so the desktop head picks up a component that declares any one of them and still skips components that declare none. - CheckMultiTarget accepts a ';' separated MultiTargetIdentifier, so the one desktop head can match sample docs for all three. - macos joins the shared net9.0 group: without it, requesting only macos on WinUI 3 produces a desktop head whose components were built solely for maccatalyst. No-op in practice, since macos is always declared alongside wasm. - MultiTarget/ReadMe.md documents what every MultiTarget maps to per WinUI version. Components and the default template now declare win32 and linux. The matching MultiTarget.props updates in Labs and stable land alongside this. Verified: UseTargetFrameworks over 6 request sets; Test-Component-Support over 9 WinUI/target combinations; MSBuild evaluation of TargetFrameworks (win32 alone -> net9.0, macos alone -> net9.0 + net9.0-maccatalyst, wasdk alone -> windows only); and generated reference props, where -mt uwp excludes a component from the desktop head and -mt linux includes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0111vdg6YNSNuoVkGdHyr2eX
Uno Platform 7.0 drops Mac Catalyst support, so net{x}-maccatalyst has no
future on the Uno 6.x/WinUI 3 side. Carrying it through this migration
would mean removing a TFM from a published package later, on its own,
without the context that explains it.
macos now means what the review agreed it should: Mac Catalyst on WinUI 2
(Uno 5.x), and the unified Skia desktop head on WinUI 3, which consumes
the shared net{x} component build that macos already contributes to. This
is why macos joined the net-common group in the previous commit.
Consumers targeting Mac Catalyst against a WinUI 3 package keep working:
the package still ships a net{x} asset, which a net{x}-maccatalyst project
resolves, and no component has Catalyst-specific code (nothing in stable
or Labs references __MACOS__ or MACCATALYST), so the two assets were
behaviourally identical anyway.
Deciding a TFM per WinUI version means EnabledTargetFrameworks.props needs
WinUIMajorVersion, so WinUI.TargetVersion.props is imported ahead of it in
Library.props. That file only defines PackageIdVariant and
WinUIMajorVersion, so nothing else is affected by the move. The condition
is written as != '3' so an undefined WinUIMajorVersion keeps the previous
behaviour rather than silently dropping the TFM.
Verified by MSBuild evaluation through Library.props' import order:
macos on WinUI 3 yields net9.0 alone, macos on WinUI 2 yields net9.0 plus
net9.0-maccatalyst, and 'all' behaves the same way on each side.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0111vdg6YNSNuoVkGdHyr2eX
| - name: Add project heads to ProjectTemplate | ||
| working-directory: ./${{ env.PROJECT_DIRECTORY }} | ||
| run: ${{ github.workspace }}/${{ env.HEADS_DIRECTORY }}/GenerateSingleSampleHeads.ps1 -componentPath ${{ github.workspace }}/${{ env.PROJECT_DIRECTORY }}${{ env.ENABLE_DIAGNOSTICS == 'true' && ' -UseDiagnostics' || '' }} | ||
| run: ${{ github.workspace }}/${{ env.HEADS_DIRECTORY }}/GenerateSingleSampleHeads.ps1 -IncludeUnoSdkHead -componentPath ${{ github.workspace }}/${{ env.PROJECT_DIRECTORY }}${{ env.ENABLE_DIAGNOSTICS == 'true' && ' -UseDiagnostics' || '' }} |
There was a problem hiding this comment.
MultiTargets imply the heads that need to be included when generating the solution, we don't need a separate switch parameter for it.
As example, if you request components that have UnoSdk-compatible MultiTargets supported and you also request to enable those same MultiTargets when generating the solution, then the UnoSdk head should be included implicitly without needing an explicit param flag/switch.
We do this on main in:
Tooling-Windows-Submodule/GenerateAllSolution.ps1
Lines 125 to 169 in ebb5ee2
and
Tooling-Windows-Submodule/GenerateSingleSolution.ps1
Lines 189 to 209 in ebb5ee2
We'll want to do something similar here, having a dedicated parameter for this is an excess requirement that we can simplify by extending the method we already use to enable heads based on requested/supported multitargets.
| multitarget: uwp | ||
| include: | ||
| # The Uno.Sdk head replaces the classic Wasm head on WinUI 3, so it rides on the | ||
| # existing wasm/winui3 job as a flag rather than a fake entry in the multitarget axis. |
There was a problem hiding this comment.
I'm very confused by the rationale for this comment, why would we need to explicitly define the behavior of switching between the classic wasm/winui2 and the modern wasm/winui3?
The multitarget system exists to abstract away this complexity and reduce the platform choice (native or uno) down to their actual deployment targets from the developer perspective.
Neither the consuming developer nor the CI matrix should have to manually activate Uno or switch around Uno complexity under the hood, our internals should handle this transparently.
E.g. -- If WinUI 2 + wasm is requested, it uses the classic WASM head. If WinUI 3 + wasm is asked for, it uses the newer Uno SDK head. These implementation details should not leak into parameters that make it to the CLI or CI, they're implied by the requested WinUI version and the user-requested + component-supported MultiTargets.
|
|
||
| - name: Install .NET workloads | ||
| if: ${{ matrix.multitarget == 'wasm' }} | ||
| run: dotnet workload install wasm-tools ${{ matrix.unoSdkHead && 'android ios maui' || '' }} |
There was a problem hiding this comment.
Same question as https://github.com/CommunityToolkit/Tooling-Windows-Submodule/pull/305/changes#r3997101887, wondering if we can have uno-check do this for us like before, or if we really must do this manually.
| -MultiTargets ${{ matrix.multitarget }} | ||
| -winui ${{ matrix.winui }} | ||
| -componentPath "${{ github.workspace }}/${{ env.TEST_PROJECT_DIRECTORY }}" | ||
| ${{ matrix.unoSdkHead && ' -IncludeUnoSdkHead' || '' }} |
There was a problem hiding this comment.
Same comment regarding -IncludeUnoSdkHead as https://github.com/CommunityToolkit/Tooling-Windows-Submodule/pull/305/changes#r3997136522
| ${{ env.ENABLE_DIAGNOSTICS == 'true' && ' -UseDiagnostics' || '' }} | ||
|
|
||
| - name: MSBuild | ||
| if: ${{ !matrix.unoSdkHead }} |
There was a problem hiding this comment.
If we clean up the unoSdkHead property per this comment, presumably we'll still need to build the Uno head with dotnet build instead of msbuild-- but I'm skeptical of that, because msbuild is what Visual Studio uses.
We need to double check that we're not making an excess/unnecessary change here, if we can build the Uno head in VS then it should also work via CLI with msbuild instead of dotnet and we won't need this line or L246-L250 below.
| shell: pwsh | ||
| working-directory: ./ | ||
| run: ./tooling/GenerateAllSolution.ps1${{ env.ENABLE_DIAGNOSTICS == 'true' && ' -UseDiagnostics' || '' }} -MultiTargets wasm | ||
| run: ./tooling/GenerateAllSolution.ps1 -IncludeUnoSdkHead${{ env.ENABLE_DIAGNOSTICS == 'true' && ' -UseDiagnostics' || '' }} -MultiTargets wasm,linux |
| # TODO: Do we want to run tests here? Can we do that on linux easily? | ||
| run: ./tooling/Build-Toolkit-Components.ps1 -Release -MultiTargets wasm,linux ${{ env.ENABLE_DIAGNOSTICS == 'true' && ' -BinlogOutput ./ -EnableBinLogs' || '' }} | ||
|
|
||
| - name: Build Uno head (browserwasm) |
There was a problem hiding this comment.
It looks like you're running our custom scaffold + build script and then... building again?
Build-Toolkit-Components.ps1 above builds all components individually, but then your invocations below to build the Uno gallery heads are building all components a second then third time.
This looks like an excess unexpected change. Presumably it's here so that we can make sure the Uno heads build on Linux and not just the components, but it's building the components multiple times and building every component at least one more time than it strictly needs to (looks like twice is minimum, currently 3x).
Since the existing Build-Toolkit-Components.ps1 script is only being invoked here to make sure project references are generated and such so that the heads can build, I recommend that instead we make sure that our Build-Toolkit-Gallery.ps1 script is capable of building the uno-specific multitargets on Linux and that we use that here instead.
This script generates project references and builds the head you ask for based solely on the requested WinUI version and the requested MultiTargets in the same manner described in my comment above.
Using Build-Toolkit-Gallery.ps1 instead of Build-Toolkit-Components.ps1 + manual csproj builds will:
- Reduce unnecessary component rebuilds
- Ensure that anyone (newcomers, CI, etc) can build the gallery head correctly knowing only the components, multitargets and winui version they want.
| <PackageReference Include="Uno.UI.WebAssembly" Version="$(CommonUnoPackageVersion)" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(IsWasmHead)' == 'true' AND '$(WinUIMajorVersion)' != '2'"> |
There was a problem hiding this comment.
We need to be very careful about using '$(WinUIMajorVersion)' != '2'" anywhere, because we don't only have WinUI 2 and 3 to choose from.
For non-WinUI projects that use the netstandard MultiTarget, the value 0 for WinUIMajorVersion is what activates it, and '$(WinUIMajorVersion)' != '2'" would accidentally activate for WinUI 0 (no-winui) projects.
These currently only exist in WCT Labs, but we need to accommodate it up front so we aren't spending another review round on it.
| <IsNetCommonMultiTargetEnabled Condition="$(EnabledMultiTargets.Contains('wasm')) == 'true' OR $(EnabledMultiTargets.Contains('wpf')) == 'true' OR $(EnabledMultiTargets.Contains('linuxgtk')) == 'true'">true</IsNetCommonMultiTargetEnabled> | ||
| <IsNetCommonMultiTargetAvailable Condition="$(AvailableMultiTargets.Contains('wasm')) == 'true' OR $(AvailableMultiTargets.Contains('wpf')) == 'true' OR $(AvailableMultiTargets.Contains('linuxgtk')) == 'true'">true</IsNetCommonMultiTargetAvailable> | ||
| <IsNetCommonMultiTargetEnabled Condition="$(EnabledMultiTargets.Contains('wasm')) == 'true' OR $(EnabledMultiTargets.Contains('wpf')) == 'true' OR $(EnabledMultiTargets.Contains('win32')) == 'true' OR $(EnabledMultiTargets.Contains('linux')) == 'true' OR $(EnabledMultiTargets.Contains('macos')) == 'true'">true</IsNetCommonMultiTargetEnabled> | ||
| <IsNetCommonMultiTargetAvailable Condition="$(AvailableMultiTargets.Contains('wasm')) == 'true' OR $(AvailableMultiTargets.Contains('wpf')) == 'true' OR $(AvailableMultiTargets.Contains('win32')) == 'true' OR $(AvailableMultiTargets.Contains('linux')) == 'true' OR $(AvailableMultiTargets.Contains('macos')) == 'true'">true</IsNetCommonMultiTargetAvailable> |
There was a problem hiding this comment.
Quick scope check both for present unnecessary changes and for missing necessary changes, does this need Android, iOS or other targets to be in this condition as well?
Originally, these conditions were there so that the non-WinUI netstandard2.0 TFM and netstandard MT (or netcore/netx in the future) wouldn't interfere with the (what were then) identical TFMs in the Uno targets, back when Uno used netstandard2.0 and back when netx.0 was used for some targets.
| which consumes the shared net9.0 build above. Uno 7 removes Mac Catalyst support entirely, so | ||
| the Catalyst TFM is deliberately not carried into the WinUI 3 package surface. | ||
| --> | ||
| <TargetFrameworks Condition="$(EnabledMultiTargets.Contains('macos')) == 'true' AND $(AvailableMultiTargets.Contains('macos')) == 'true' AND '$(WinUIMajorVersion)' != '3'">$(TargetFrameworks);$(MacOSLibTargetFrameworks)</TargetFrameworks> |
There was a problem hiding this comment.
Same concern as this comment regarding '$(WinUIMajorVersion)' != '2'"
| <IsDeployableHead>true</IsDeployableHead> | ||
| <IsUno>true</IsUno> | ||
| <IsWasm Condition="'$(TargetFramework)' == 'net9.0-browserwasm'">true</IsWasm> | ||
| <IsDesktop Condition="'$(TargetFramework)' == 'net9.0-desktop'">true</IsDesktop> |
There was a problem hiding this comment.
The IsDesktop variable defined here and by the Uno.Sdk seems to be slightly conflicting with the Is* multitarget variables we use to abstract over the TFMs.
IsDesktop is more than fine to consume for defining skia-specific compilation constants, but this variable has lingering usages throughout the codebase where it should be declaring the individual multitargets that the desktop TFM supports. It's the multitargets that imply the TFMs the user wants to load/use, not the other way around.
The issue here is that we're defining IsDesktopMultiTargetEnabled and subsequently the desktop TFM by checking EnabledMultiTargets against the ones we know the desktop TFM supports, but then we're using that TFM to define whether IsDesktop is enabled, and then we're using IsDesktop to tell other code that it could be one of the macos, linux or the win32 multitargets.
Instead, we need to clean up IsDesktop everywhere that it reaches into the MultiTarget system and replace it with either IsMacOS, IsLinux, or IsWin32.
Letting it define constants makes sense, letting it define MultiTarget internals does not make sense-- the MT system uses the declared support in the heads to light up the supported+requested TFMs in the libraries, never the other way around (in libraries) where TFMs deciding the MTs that are enabled.
For heads, they usually define TFMs inline in the csproj, and for heads that have multiple possible TFMs the Conditions here make sense for IsDroid and IsiOS etc., but declaring IsDesktop to tunnel all three possible targets into a variable that represents the TFM, which then itself represents the mt variables, to use directly in the MT system rather than the separate IsMacOS, IsLinux and IsWin32, could get messy in the future.
It carries the additional risk of making it easier to enable e.g. linux when you only wanted macos or win32, etc.-- not all components will support all three when it supports one, reflected more by compilation conditionals guarding platform-specific code-behind than TFM compatibility or universality of Skia rendering is.
| | MultiTarget | Platform | WinUI 2 (Uno 5.x) | WinUI 3 (Uno 6.x) | | ||
| | --- | --- | --- | --- | | ||
| | `uwp` | Windows, UWP XAML | `uap10.0.17763`, `net{x}-windows10.0.26100` | n/a | | ||
| | `wasdk` | Windows, Windows App SDK | n/a | `net{x}-windows10.0.19041` | |
There was a problem hiding this comment.
We've bumped the WASDK TFM from 19041 to 26100 over in #316
| OutputItemType="Analyzer" ReferenceOutputAssembly="true" /> | ||
| </ItemGroup> | ||
|
|
||
| <!-- Gallery dependencies --> |
There was a problem hiding this comment.
Gallery dependencies on project heads shouldn't be directly referenced with a PackageReference, because these exact assemblies are present as ProjectReferences when you generate the solution from the stable mainline wct repo.
You'll need these links to do this right:
- https://github.com/CommunityToolkit/Windows/blob/413892f3e929beae3fbcf9863b8385e570407a41/Directory.Build.props#L10-L21
- https://github.com/CommunityToolkit/Labs-Windows/blob/13a9bf4ebfec272bfe2ba9875b21a5e09829ad74/Directory.Build.props#L1-L37
Tooling-Windows-Submodule/ProjectHeads/App.Head.props
Lines 31 to 84 in ebb5ee2
Notice how Labs doesn't define the properties pointing to the component path on disk, because it doesn't have those components on disk. Instead, seeing that the properties are missing, the code in App.Head.props includes the PackageReferences. This is what you want to lean on instead of doing it manually.
|
|
||
| <!-- Uno app dependencies --> | ||
| <ItemGroup> | ||
| <PackageReference Include="Uno.Microsoft.Xaml.Behaviors.Interactivity.WinUI" Version="3.0.3" /> |
There was a problem hiding this comment.
App head dependencies that are Uno.WinUI-specific should go in ProjectHeads/App.Head.Uno.WinUI.Dependencies.props
| </ItemGroup> | ||
|
|
||
| <!-- Sample doc content inclusion --> | ||
| <ItemGroup> |
There was a problem hiding this comment.
This whole code block looks like it was duplicated from our existing App.Head.props. We shouldn't be copy-pasting code when the code being copied is designed to be used and reused modularly.
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <NoWarn>$(NoWarn);IL2026;Uno0006</NoWarn> |
There was a problem hiding this comment.
Both of these NoWarn declarations already exist in App.Head.Uno.props, which wasn't imported properly in this file. Duplicated code.
|
|
||
| <!-- Disable AOT on Android - not needed for test app, avoids issues with source generator assemblies in runtime output --> | ||
| <PropertyGroup Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'android'"> | ||
| <RunAOTCompilation>false</RunAOTCompilation> |
There was a problem hiding this comment.
We have an existing App.Head.Wasm.props where we set WASM-specific properties, including AoT configuration, then import directly on the Wasm head.
This AoT flag should be moved into a similar App.Head.Android.props and imported similarly so that it can be reused across both single-component and all-component solution variants.
|
|
||
| IsSupported = multiTargetIdentifier.Split(';').Contains(MultiTargetIdentifier); | ||
| // MultiTargetIdentifier may be a ';' separated list: the unified Uno desktop head covers | ||
| // several MultiTargets with a single build, so a doc matching any one of them is supported. |
There was a problem hiding this comment.
Identical concern to this comment.
the unified Uno desktop head covers several MultiTargets with a single build,
Yes, but also no, not quite-- it's largely the other way around, and it's a distinction we must be careful about, so we don't mix up the direction that data is meant to flows between TFMs and MTs and make a mess.
Instead:
Several combinations of MultiTarget and WinUI version, namely WinUI 3 under Linux, MacOS and Win32, imply that the unified Uno desktop head and custom
-desktopTFM is used because it's the only thing that can run them. The TFM does not define what MultiTarget(s) a component supports, rather the MultiTargets define what TFMs that a component supports.
This is subtly different. The WinUI version and requested/supported (per component) MultiTargets decide what head and TFM to use for that specific configuration.
For WinUI 3 under Linux, MacOS and Win32, we back it with the -desktop TFM, but the MT internals hide that detail and simply say linux, macos and win32 or IsLinux IsMacOS and IsWin32, all without TFM knowledge UNTIL it computes the enabled TFMs by the matrix of:
- MTs that are available + requested/enabled (at slngen) + supported (per component)
- WinUI major versions that are requested/enabled (explicit at slngen) + supported (implicit via MTs)
The correct TFMs the end user wants (and can want, per component support) are the end result/output of invoking the MT system, never used in input or in interim code.
These docs made the issue obvious but actual code fix here stretches beyond this file. If enabling any of the MTs that give you the -desktop TFM also enable all other MTs that use the -desktop TFM, that's a bug, and I see it mirrored elsewhere e.g. here.
Yes, the -desktop TFM which supports all of them gets used if any are supported and requested, the problem is the other non-TFM things we do when the user doesn't request one of those targets.
| <Project> | ||
| <PropertyGroup> | ||
| <DependencyVariant>WinUI</DependencyVariant> | ||
| <DependencyVariant Condition="'$(WinUIMajorVersion)' != '2'">WinUI</DependencyVariant> |
There was a problem hiding this comment.
Same concern as this comment regarding '$(WinUIMajorVersion)' != '2'"
| Changes to this file are suppressed by git when switching to avoid changing the default for all users. | ||
| --> | ||
| <Import Project="$(MSBuildThisFileDirectory)\App.Head.Uno.WinUI.Dependencies.props" /> | ||
| <Import Project="$(MSBuildThisFileDirectory)\App.Head.Uno.WinUI.Dependencies.props" Condition="'$(WinUIMajorVersion)' != '2'" /> |
There was a problem hiding this comment.
Same concern as this comment regarding '$(WinUIMajorVersion)' != '2'"
| # Use & and a separate script path variable to avoid issues with parameter passing | ||
| $scriptPath = "$PSScriptRoot/../GenerateSingleSolution.ps1" | ||
| & $scriptPath -MultiTargets $MultiTargets -ExcludeMultiTargets $ExcludeMultiTargets -WinUIMajorVersion $WinUIMajorVersion -UseDiagnostics:$UseDiagnostics -componentPath $componentPath No newline at end of file | ||
| & $scriptPath -MultiTargets $MultiTargets -ExcludeMultiTargets $ExcludeMultiTargets -WinUIMajorVersion $WinUIMajorVersion -UseDiagnostics:$UseDiagnostics -IncludeUnoSdkHead:$IncludeUnoSdkHead -componentPath $componentPath No newline at end of file |
There was a problem hiding this comment.
Same comment regarding -IncludeUnoSdkHead as https://github.com/CommunityToolkit/Tooling-Windows-Submodule/pull/305/changes#r3997136522
|
|
||
| [bool]$Launch = $true, | ||
|
|
||
| [switch]$IncludeUnoSdkHead = $false |
There was a problem hiding this comment.
Same comment regarding -IncludeUnoSdkHead as https://github.com/CommunityToolkit/Tooling-Windows-Submodule/pull/305/changes#r3997136522
| [switch]$UseDiagnostics = $false | ||
| [switch]$UseDiagnostics = $false, | ||
|
|
||
| [switch]$IncludeUnoSdkHead = $false |
There was a problem hiding this comment.
Same comment regarding -IncludeUnoSdkHead as https://github.com/CommunityToolkit/Tooling-Windows-Submodule/pull/305/changes#r3997136522
Arlodotexe
left a comment
Arlodotexe
left a comment
There was a problem hiding this comment.
Got through review of every changed file this time! Left several comments:
- A few minor residual issues from the last review round (particularly IsDesktop still mixed into MTs)
- Several misc gotchas that were in your blind spot
- Several things to clean up or fix, both small and mid-sized finds.
Otherwise, this code is looking really good! If we can resolve the flagged concerns here all within the next review round, I can get this tested locally and I'll happily approve to merge and close off.
Depends on #304