Test AOT publish #3937
Test AOT publish #3937
Conversation
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughAdds AOT publish configuration and IL compiler/linker packages, and replaces runtime JSON deserialization with a source-generated JsonSerializerContext for Windows settings deserialization. No removals or other functional API changes are present. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Code invoking deserialization
participant JsonLib as System.Text.Json
rect rgb(240,248,255)
Note right of Caller: Previous flow (runtime options)
Caller->>JsonLib: JsonSerializer.Deserialize(text, typeof(IEnumerable<WindowsSetting>), options)
JsonLib-->>Caller: Uses reflection/runtime metadata to produce IEnumerable<WindowsSetting>
end
rect rgb(245,255,240)
Note right of Caller: New flow (source-generated)
Caller->>JsonLib: JsonSerializer.Deserialize(text, MyJsonContext.Default.IEnumerableWindowsSetting)
JsonLib-->>Caller: Uses compile-time generated metadata (no runtime options)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Directory.Build.props(1 hunks)Flow.Launcher.Core/packages.lock.json(1 hunks)Flow.Launcher.Infrastructure/packages.lock.json(1 hunks)Flow.Launcher.Plugin/packages.lock.json(1 hunks)Flow.Launcher/packages.lock.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T15:52:58.573Z
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Applied to files:
Flow.Launcher.Infrastructure/packages.lock.jsonFlow.Launcher.Core/packages.lock.jsonFlow.Launcher.Plugin/packages.lock.jsonFlow.Launcher/packages.lock.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
Flow.Launcher.Plugin/packages.lock.json (2)
17-22: AOT dependencies correctly added.The Microsoft.DotNet.ILCompiler package enables native AOT compilation for better performance and smaller deployment size. The version 9.0.8 aligns with .NET 9 requirements.
23-28: ILLink.Tasks package supports tree-shaking.Microsoft.NET.ILLink.Tasks provides IL trimming capabilities that work alongside AOT compilation to remove unused code and reduce application size.
Flow.Launcher.Infrastructure/packages.lock.json (2)
48-53: Consistent AOT toolchain dependency added.The Microsoft.DotNet.ILCompiler package is properly configured across all project modules to enable AOT compilation.
54-59: ILLink.Tasks enhances AOT optimization.This package provides IL linking and trimming capabilities that complement AOT compilation for optimized binary generation.
Flow.Launcher/packages.lock.json (2)
72-77: Main application AOT support enabled.Microsoft.DotNet.ILCompiler 9.0.8 is correctly added to enable native AOT compilation for the main Flow.Launcher executable, which should improve startup performance and reduce memory usage.
117-122: IL linking tasks for comprehensive optimization.Microsoft.NET.ILLink.Tasks provides essential IL trimming and linking functionality that works with AOT to produce optimized binaries by removing unused code paths.
Flow.Launcher.Core/packages.lock.json (2)
26-31: Core module AOT enablement.The Microsoft.DotNet.ILCompiler dependency is properly added to the core module to support AOT compilation across the application's foundational components.
38-43: IL optimization toolkit integration.Microsoft.NET.ILLink.Tasks complements the AOT compiler by providing advanced IL trimming and optimization capabilities for reduced binary size.
| <AccelerateBuildsInVisualStudio>true</AccelerateBuildsInVisualStudio> | ||
| <!-- Work around https://github.com/dotnet/runtime/issues/109682 --> | ||
| <CETCompat>false</CETCompat> | ||
| <PublishAot>true</PublishAot> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t enable AOT globally; gate it (WPF and plugin-style apps are incompatible with trimming/AOT).
NativeAOT requires trimming, and WPF trimming is disabled in the SDK; dynamic assembly loading (common in plugin systems) is also trim-incompatible. This unconditional setting will fail publish or break at runtime. Gate AOT behind an opt-in switch and exclude WPF/WinForms EXEs. (learn.microsoft.com)
Apply this minimal fix:
<CETCompat>false</CETCompat>
- <PublishAot>true</PublishAot>
+ <!-- Opt-in from CLI/CI: -p:EnableNativeAot=true -->
+ <EnableNativeAot>false</EnableNativeAot>
+ <PublishAot Condition="'$(EnableNativeAot)' == 'true'
+ and '$(IsPublishable)' == 'true'
+ and '$(OutputType)' == 'Exe'
+ and $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))
+ and '$(RuntimeIdentifier)' != ''
+ and '$(UseWPF)' != 'true'
+ and '$(UseWindowsForms)' != 'true'">true</PublishAot>Example publish: dotnet publish src/Flow.Launcher/Flow.Launcher.csproj -c Release -r win-x64 -p:EnableNativeAot=true
Note: Windows AOT also requires the “Desktop development with C++” workload on build agents/dev machines. (learn.microsoft.com)
🏁 Script executed:
#!/bin/bash
# Verify risk hotspots for AOT in this repo
set -euo pipefail
echo "Projects using WPF/WinForms:"
rg -n -g "**/*.csproj" -C1 -P '<UseWPF>\s*true|<UseWindowsForms>\s*true' || true
echo -e "\nExecutable projects and TFMs:"
rg -n -g "**/*.csproj" -C1 -P '<OutputType>\s*(WinExe|Exe)\s*</OutputType>|<TargetFrameworks?>' || true
echo -e "\nDynamic loading patterns (trim/AOT incompatible):"
rg -n -g "!**/bin/**" -g "!**/obj/**" -P '\b(Assembly\.Load|AssemblyLoadContext\.Default\.Load|LoadFrom\(|LoadFile\()' || trueLength of output: 12529
Gate Native AOT behind an opt-in switch and only for compatible executables
NativeAOT (PublishAot) must not be enabled globally—trimming is incompatible with WPF/WinForms and plugin-style assemblies. Add an EnableNativeAot flag (default false) and conditionally set <PublishAot> only when publishing an EXE targeting .NET 9.0+ with a RID and without WPF/WinForms:
<CETCompat>false</CETCompat>
- <PublishAot>true</PublishAot>
+ <!-- Opt-in via CLI/CI: -p:EnableNativeAot=true -->
+ <EnableNativeAot>false</EnableNativeAot>
+ <PublishAot Condition="
+ '$(EnableNativeAot)' == 'true'
+ and '$(IsPublishable)' == 'true'
+ and '$(OutputType)' == 'Exe'
+ and $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0'))
+ and '$(RuntimeIdentifier)' != ''
+ and '$(UseWPF)' != 'true'
+ and '$(UseWindowsForms)' != 'true'">
+ true
+ </PublishAot>Example publish command:
dotnet publish Flow.Launcher/Flow.Launcher.csproj -c Release -r win-x64 -p:EnableNativeAot=true
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <PublishAot>true</PublishAot> | |
| <PropertyGroup> | |
| <CETCompat>false</CETCompat> | |
| <!-- Opt-in via CLI/CI: -p:EnableNativeAot=true --> | |
| <EnableNativeAot>false</EnableNativeAot> | |
| <PublishAot Condition=" | |
| '$(EnableNativeAot)' == 'true' | |
| and '$(IsPublishable)' == 'true' | |
| and '$(OutputType)' == 'Exe' | |
| and $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0')) | |
| and '$(RuntimeIdentifier)' != '' | |
| and '$(UseWPF)' != 'true' | |
| and '$(UseWindowsForms)' != 'true'"> | |
| true | |
| </PublishAot> | |
| </PropertyGroup> |
🤖 Prompt for AI Agents
In Directory.Build.props around line 6, PublishAot is currently enabled
globally; add a new property <EnableNativeAot>false</EnableNativeAot> and change
the PublishAot setting to only evaluate true when publishing an executable for
.NET 9+ with a RID and explicit opt-in: check $(EnableNativeAot) == 'true',
$(OutputType) == 'Exe' (or $(IsExe) if used),
TargetFrameworkMoniker/TargetFramework starts with net9 or
TargetFrameworkVersion >= 9, and that a RuntimeIdentifier/RuntimeIdentifiers is
present, and exclude when $(UseWPF) == 'true' or $(UseWindowsForms) == 'true'
(or when WPF/WinForms SDK is detected); implement this as a conditional
PropertyGroup so the default remains false and developers can opt in via
-p:EnableNativeAot=true when publishing an EXE with a RID.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.WindowsSettings/Helper/JsonSettingsListHelper.cs (4)
45-45: Good switch to source-generated metadata; deserialize directly from the stream to avoid extra allocation.You can skip the intermediate string and keep it UTF-8 by deserializing from the resource stream.
- using var reader = new StreamReader(stream); - var text = reader.ReadToEnd(); - - settingsList = JsonSerializer.Deserialize(text, MyJsonContext.Default.IEnumerableWindowsSetting); + settingsList = JsonSerializer.Deserialize(stream, MyJsonContext.Default.IEnumerableWindowsSetting);
56-63: Context config looks correct for AOT; consider clearer naming and trimming code size.
- Rename the context to reflect scope (optional).
- If you only need metadata-based APIs, set GenerationMode = Metadata to reduce generated code.
- [JsonSourceGenerationOptions( - Converters = new[] { typeof(JsonStringEnumConverter) }, - PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase - )] - [JsonSerializable(typeof(IEnumerable<WindowsSetting>))] - internal partial class MyJsonContext : JsonSerializerContext + [JsonSourceGenerationOptions( + Converters = new[] { typeof(JsonStringEnumConverter) }, + PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, + GenerationMode = JsonSourceGenerationMode.Metadata + )] + [JsonSerializable(typeof(IEnumerable<WindowsSetting>))] + internal partial class WindowsSettingsJsonContext : JsonSerializerContext { }And update the call site:
- settingsList = JsonSerializer.Deserialize(stream, MyJsonContext.Default.IEnumerableWindowsSetting); + settingsList = JsonSerializer.Deserialize(stream, WindowsSettingsJsonContext.Default.IEnumerableWindowsSetting);
28-36: Reflection here is brittle under trimming; prefer direct type access.AOT+linker can make Assembly.GetTypes() unreliable and costly. Use typeof(Main) to get the assembly/namespace without scanning.
- var assembly = Assembly.GetExecutingAssembly(); - var type = assembly.GetTypes().FirstOrDefault(x => x.Name == nameof(Main)); + var assembly = typeof(Main).Assembly; + var type = typeof(Main); @@ - var resourceName = $"{type?.Namespace}.{_settingsFile}"; + var resourceName = $"{type.Namespace}.{_settingsFile}";
39-41: Improve the exception for easier diagnostics.Include the computed resource name in the message to quickly spot namespace mismatches.
- throw new Exception("stream is null"); + throw new Exception($"Embedded resource not found: {resourceName}");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.WindowsSettings/Helper/JsonSettingsListHelper.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
Plugins/Flow.Launcher.Plugin.WindowsSettings/Helper/JsonSettingsListHelper.cs (1)
31-53: Embedded JSON resource and AOT deserialization verified. WindowsSettings.json is present and marked as an EmbeddedResource in the .csproj, and JsonSerializer.Deserialize uses the source-generated MyJsonContext (IEnumerableWindowsSetting), ensuring AOT compatibility.
|
@Jack251970 out of interest, how far did you get with this? |
It seems WPF cannot work with AOT at all |
No description provided.