Add AOT support for dotnet sln list, migrate, and remove - #54384
Conversation
Enable 'sln list' and 'sln migrate' commands in the AOT-compiled CLI (dotnet-aot). These commands are self-contained — they parse solution files using Microsoft.VisualStudio.SolutionPersistence without any MSBuild or NuGet dependencies. Changes: - Parser.cs: Add sln command with list/migrate subcommands under #if CLI_AOT, with inline action handlers that avoid CommandBase and its ParseResultExtensions dependency chain - dotnet-aot.csproj: Link SlnFileFactory.cs and SlnfFileHelper.cs, add CliStrings.resx as embedded resource, add Microsoft.VisualStudio.SolutionPersistence package reference Design decisions: - SLN_FILE argument is on each subcommand (not parent) so that unsupported commands like 'sln add/remove' produce parse errors and correctly fall back to the managed CLI - Hardcoded English strings for AOT-specific messages (matching the existing --info AOT pattern) to avoid CliCommandStrings.resx dep - GracefulException handling wraps all action handlers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implement sln remove subcommand in the CLI_AOT parser section, including project removal from both .sln and .slnf files, empty solution folder cleanup, and directory-to-project resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… validation
- sln migrate: Use same error/success messages as managed implementation
('Only .sln files can be migrated' and '.slnx file {0} generated.')
- sln remove: Add validation to detect misplaced .sln/.slnx files in
project arguments with 'Did you mean' suggestion
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move SLN_FILE argument from individual subcommands to the parent sln command, matching the managed CLI structure. This fixes a parsing ambiguity where 'dotnet sln remove proj1.csproj proj2.csproj' could misinterpret the first project as the solution file. Also: - Remove parent sln action handler so bare 'dotnet sln' falls back to managed CLI (which shows complete help including 'add') - Use .GetAwaiter().GetResult() instead of .Wait() in migrate Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@NikolaMilosavljevic as we start lighting up these commands, a before/after comparison of time spent would be very useful data to have! |
There was a problem hiding this comment.
Pull request overview
Adds Native AOT support for dotnet sln list, sln migrate, and sln remove by wiring inline handlers into the CLI_AOT Parser, linking the SlnFileFactory/SlnfFileHelper source files into the AOT project, and providing a hand-written CliStrings shim with the subset of strings these flows need. Unknown sln subcommands and bare dotnet sln deliberately fail to parse so NativeEntryPoint falls back to the managed CLI.
Changes:
- Add
sln list/migrate/removehandlers inParser.csunder#if CLI_AOT, including misplaced-sln-file detection, directory-to-project resolution, and empty-solution-folder cleanup. - Link
SlnFileFactory.cs/SlnfFileHelper.csand add theMicrosoft.VisualStudio.SolutionPersistencepackage reference to the AOT csproj. - Introduce
src/Cli/dotnet-aot/CliStrings.csas a string shim with hard-coded English values for the resources used by the linked sources.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Cli/dotnet/Parser.cs | New ConfigureSolutionCommand, inline list/migrate/remove handlers, and helpers (GetProjectFileFromDirectory, RemoveProjectsFromSolution, RemoveProjectsFromSolutionFilter) duplicated from the managed CLI with hard-coded English strings. |
| src/Cli/dotnet-aot/dotnet-aot.csproj | Links SlnFileFactory.cs/SlnfFileHelper.cs, adds Microsoft.VisualStudio.SolutionPersistence package, includes the new CliStrings.cs shim. |
| src/Cli/dotnet-aot/CliStrings.cs | New shim providing the subset of CliStrings properties (matching the values in CliStrings.resx) consumed by the linked AOT sources. |
Replace duplicated inline command definitions and action handlers with shared code from Microsoft.DotNet.Cli.Definitions project: - Use SolutionCommandDefinition from Definitions project instead of creating ad-hoc Command instances in Parser.cs AOT section - Use SolutionCommandParser to wire actions for list/migrate/remove - Link command implementation files (SolutionListCommand, SolutionMigrateCommand, SolutionRemoveCommand) from managed project - Link CommandBase, CommandParsingException, SolutionArgumentValidator, and MsbuildProject with #if CLI_AOT guards - Replace hand-written CliStrings.cs shim with proper .resx inclusion (CliStrings.resx + CliCommandStrings.resx with GenerateSource) - Add Definitions project reference to dotnet-aot.csproj - Add #if CLI_AOT guards in CommandBase.cs (skip ShowHelpOrErrorIfAppropriate) - Add #if CLI_AOT guards in MsbuildProject.cs (inline file-I/O methods, exclude MSBuild-dependent code) - Add #if CLI_AOT guards in SolutionCommandParser.cs (skip add command and parent action wiring) - Remove AddCommand from AOT parser to trigger managed fallback - Add localized strings for project directory lookup errors Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refactoring summary (commit 908be28)This commit addresses the review feedback by refactoring the AOT sln commands to share definitions and implementations with the managed CLI instead of duplicating them: Key changes:
Net effect on Parser.cs AOT section:
All 157 sln tests pass (list: 54, migrate: 2, remove: 101). |
baronfel
left a comment
There was a problem hiding this comment.
Love the new approach - it makes a lot of sense to me!
|
One additional note on the refactoring commit: the |
Address review feedback: replace the sketchy Data dictionary check with a direct catch of GracefulException, which is the specific exception type used to signal user-displayable errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ption Instead of removing AddCommand from the definition tree in Parser.cs, wire it (and the parent sln command) to throw a dedicated exception in AOT mode. NativeEntryPoint catches CommandNotAvailableInAotException and falls back to managed CLI. This keeps AOT customization local to SolutionCommandParser.cs and the definition tree intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On macOS, NativeAOT unconditionally statically links libSystem.Security.Cryptography.Native.Apple.a into all outputs, including NativeLib=Shared. This embeds Swift binding classes (HashBox, X25519KeyBox) that duplicate the same classes in the shared framework, causing ObjC runtime warnings to stderr. These warnings break tests that assert stderr is empty. Since the AOT CLI does not use cryptographic APIs, remove the native library and DirectPInvoke entry after SetupOSSpecificProps. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On macOS, NativeAOT statically links libSystem.Security.Cryptography.Native.Apple.a into every shared library output. The Swift binding classes in this archive conflict with the host process's copy, causing duplicate ObjC class warnings on stderr. Since the AOT CLI does not use cryptographic APIs, safely remove this library. Port of fix from PR dotnet#54384. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…props The merge with main kept both the AotSourceFiles.props import and a full inline Compile list in dotnet-aot.csproj, causing duplicate compile items. It also left the dotnet-aot.Tests project unable to build, because it imports the shared props (which includes Parser.cs) but Parser.cs now references the sln command tree that wasn't shared. Consolidate everything the shared Parser.cs needs into AotSourceFiles.props: the sln command sources, CommandBase, the exception types, the CliStrings/CliCommandStrings embedded resources those sources reference, and the Definitions project + SolutionPersistence package dependencies. Use MSBuildThisFileDirectory-relative paths so they resolve from both the AOT project and the test project. Remove the duplicated inline entries from dotnet-aot.csproj, keeping only project-specific items. Restore the 'Usage: dn [options]' output in the AOT Parser root action that the merge dropped (required by AotParserTests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Split shared AOT scaffolding from sln-specific items into clearly-named groups so future commands reuse the common group instead of re-adding it, reducing merge conflicts with sibling AOT PRs. Pure reorganization; no items added or removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This is now passing all checks. @baronfel @JeremyKuhne @marcpopMSFT - any more comments before we can get this approved and merged? |
baronfel
left a comment
There was a problem hiding this comment.
One question about how the macOS crypto library might bite us in the butt :)
| (from the shared framework's copy of the same library), macOS emits duplicate class warnings to stderr, | ||
| which breaks tests that assert stderr is empty. | ||
|
|
||
| Since the AOT CLI does not use any cryptographic APIs, we can safely remove this native library from linking. |
There was a problem hiding this comment.
how long will this invariant be true? as we move more commands into place, surely we'll eventually need crypto APIs. What's the runtime experience for the AOT slice if we need this and it no longer exists?
There was a problem hiding this comment.
I don't understand why this is happening. Where exactly is the duplicate coming from? We should poke @agocke on how to properly handle the problem.
There was a problem hiding this comment.
Will investigate this more closely - thanks.
There was a problem hiding this comment.
@JeremyKuhne I dug into this — here's the root cause. /cc @agocke
The duplicate is the Apple crypto native library ending up in the same process twice — once dynamically (the running .NET host) and once statically baked into our AOT dylib.
-
Host already has it (dynamically): the normal
dotnethost loads the shared framework, which includeslibSystem.Security.Cryptography.Native.Apple.dylib. That dylib containspal_swiftbindings.o, whose Swift interop registers ObjC classes (HashBox,X25519KeyBox) with the process-wide ObjC runtime at load time. -
NativeAOT bakes a second copy in (statically): in
Microsoft.NETCore.Native.Unix.targets(ILCompiler11.0.0-preview.5.26261.101), line ~150:<NetCoreAppNativeLibrary Include="System.Security.Cryptography.Native.Apple" Condition="'$(_IsApplePlatform)' == 'true'" />
The only condition is "is this an Apple platform" — there is no
'$(NativeLib)' != 'Shared'guard. So ourNativeLib=Sharedoutput (libdotnet-aot.dylib) statically links the.aversion of the same crypto library, including the samepal_swiftbindings.oand itsHashBox/X25519KeyBoxclasses. -
ObjC class names are process-global → collision: our dylib is loaded into the host process that already registered those classes from the framework copy. The dynamic loader then prints to stderr:
objc[...]: Class HashBox is implemented in both .../libSystem.Security.Cryptography.Native.Apple.dylib and .../libdotnet-aot.dylib. One of the two will be used. Which one is undefined.That stderr line is what trips the
StdErr.Should().BeEmpty()assertions in our tests.
Root flaw: the NativeAOT targets assume the output is a self-contained app that needs crypto statically baked in. For a NativeLib=Shared library loaded into an already-running .NET process, that static copy is redundant and collides with the framework's dynamic copy. The proper upstream fix looks like guarding line ~150 (and the matching DirectPInvoke) with '$(NativeLib)' != 'Shared' so shared-library AOT outputs don't re-embed framework-provided native libs.
Our RemoveAppleCryptoFromNativeAotLink target is essentially doing that guard manually from the csproj after the fact. @agocke — does guarding these NetCoreAppNativeLibrary/DirectPInvoke items on NativeLib != Shared sound like the right runtime-side fix, or is there a preferred way to handle native-lib linking for shared AOT outputs that load into an existing runtime?
There was a problem hiding this comment.
Good question. To close the loop: @MichalStrehovsky extracted the root cause into a runtime tracking issue — dotnet/runtime#128867 ("Two CLR-based apps cannot coexist on Apple platforms"). So the underlying problem (NativeAOT statically baking framework-provided native libs into a NativeLib=Shared output that's loaded into an already-running CLR host) is now owned by the runtime/NativeAOT team.
On the durability of the "AOT CLI uses no crypto" invariant: you're right that it won't hold forever as more commands move into the AOT slice. Two points:
-
This
RemoveAppleCryptoFromNativeAotLinktarget is explicitly an interim workaround, and I've added a comment linking Two CLR-based apps cannot coexist on Apple platforms runtime#128867 so it's clear it should be removed once the runtime fix lands. The proper fix (guarding the native-lib linking onNativeLib != Shared, or reusing the already-loadedlibSystem.*.Nativedylibs as Michal suggests in the issue) makes the invariant irrelevant — crypto would resolve to the host's existing copy rather than a statically-baked duplicate. -
If we needed crypto in the AOT slice before the runtime fix lands, the failure mode isn't a missing-symbol crash — the crypto P/Invokes resolve against the framework's already-loaded
libSystem.Security.Cryptography.Native.Apple.dylibin the host process. The only observable artifact today is the duplicate-class stderr warning (which is exactly what this workaround suppresses by not baking in the second static copy). So removing the static copy is the correct behavior for a shared-lib-in-host scenario, not just a no-crypto convenience.
Net: the workaround is safe to keep short-term, is now tracked upstream, and won't silently break crypto-using AOT commands — worst case pre-fix is the stderr warning returning, which we'd catch immediately via the StdErr.Should().BeEmpty() assertions.
There was a problem hiding this comment.
Net: the workaround is safe to keep short-term
Unfortunately the workaround looks unsound. System.Security.Cryptography.Native.Apple holds onto global state, at minimum this one:
The workaround (native AOT and JIT-based runtime using the same libSystem.Security.Cryptography.Native.Apple.dylib within the process) will mean whichever comes last will win and erase callbacks set by the other one.
I think we'll need some refactoring on the libSystem.Security.Cryptography.Native.Apple.dylib side to allow reusing the library by multiple runtime instances within the same process, or explore other avenues.
There was a problem hiding this comment.
What the workaround I'm commenting on does is that it changes DllImport to the System.Security.Cryptography.Native.Apple library from direct p/invoke to "normal p/invoke".
Normal p/invoke will resolve the p/invoke same way how JIT-based CoreCLR does - inspecting DllImportSearchPaths, prefixing "lib", etc. I expect that it will resolve to the .dylib next to the library that will be used by the CoreCLR-based runtime too.
The DirectPInvoke on the other hand makes resolving this a problem for the linker to solve. Linker will see the symbols defined in the libSystem.Security.Cryptography.Native.Apple.a and will end up statically linking the library into the executable (leading the the runtime stderr spew). If we're seeing the stderr spew, libdotnet-aot.dylib probably already depends on the crypto somewhere, otherwise linker wouldn't have included libSystem.Security.Cryptography.Native.Apple.a.
|
Feel like we could use this as the basis of an 'aot a dotnet cli command' skill or something. |
JeremyKuhne
left a comment
There was a problem hiding this comment.
Should avoid copying any existing code if at all possible. Worst case we include individual files, but ultimately we want to include all of the sources and dependencies with a few targeted excludes.
| { | ||
| public static string GetProjectFileFromDirectory(string projectDirectory) | ||
| { | ||
| DirectoryInfo dir; |
There was a problem hiding this comment.
We should avoid duplicating code here. The idea is that we always use defines to handle excluding or tweaking code. You should be able to include the whole shared project in this case.
There was a problem hiding this comment.
Good call — fixed in 4e1c7dc.
Rather than reimplementing the directory project-lookup logic inline, I restored the original thin delegating wrappers (GetProjectFileFromDirectory/TryGetProjectFileFromDirectory now just call ProjectLocator.TryGetProjectFileFromDirectory, exactly as on main) and linked the shared, AOT-safe ProjectLocator.cs (pure file I/O — only System.IO/System.Linq + FileBasedProgramsResources, no MSBuild/NuGet deps) into the AOT project via AotSourceFiles.props. The using Microsoft.DotNet.ProjectTools; moved out of the #if !CLI_AOT block so it's available in AOT too.
I also reverted 3 redundant resource strings (CouldNotFindProjectOrDirectory, CouldNotFindAnyProjectInDirectory, MoreThanOneProjectInDirectory) my earlier commit had added to CliStrings.resx + the 13 xlf files. Those shadowed the existing CliStringsExtensions delegation to FileBasedProgramsResources and caused English-vs-localized drift; with ProjectLocator linked in, the strings now come (localized) from FileBasedProgramsResources.
Net result is +7/−261 — the inline duplication and the duplicate resx/xlf entries are gone. Builds clean (dotnet-aot, dotnet-aot.Tests, managed dotnet) and AOT (44) + sln-remove (101) tests pass.
I interpreted "include the whole shared project" as linking just the ProjectLocator source it needs rather than importing the entire Microsoft.DotNet.FileBasedPrograms projitems — the latter would pull in the file-based-app code (Roslyn/System.Text.Json) that isn't AOT-friendly and isn't needed by dotnet sln. Happy to revisit if you'd prefer a dedicated shared item split for ProjectLocator.
| (from the shared framework's copy of the same library), macOS emits duplicate class warnings to stderr, | ||
| which breaks tests that assert stderr is empty. | ||
|
|
||
| Since the AOT CLI does not use any cryptographic APIs, we can safely remove this native library from linking. |
There was a problem hiding this comment.
I don't understand why this is happening. Where exactly is the duplicate coming from? We should poke @agocke on how to properly handle the problem.
Address Jeremy's review feedback to avoid duplicating code. Instead of reimplementing the directory project-lookup logic inline in MsbuildProject, restore the thin delegating wrappers and link the shared, AOT-safe ProjectLocator (pure file I/O) into the AOT project. - Restore MsbuildProject.GetProjectFileFromDirectory / TryGetProjectFileFromDirectory as wrappers delegating to ProjectLocator.TryGetProjectFileFromDirectory (matches main). - Move `using Microsoft.DotNet.ProjectTools;` out of the !CLI_AOT block so it is available in AOT too. - Link ProjectLocator.cs and FileBasedProgramsResources.resx into AotSourceFiles.props (shared by dotnet-aot and dotnet-aot.Tests). - Revert the redundant CouldNotFindProjectOrDirectory / CouldNotFindAnyProjectInDirectory / MoreThanOneProjectInDirectory entries previously added to CliStrings.resx and the xlf files. These shadowed the existing CliStringsExtensions delegation to FileBasedProgramsResources and caused English-vs-localized drift; ProjectLocator now sources them from FileBasedProgramsResources (localized). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/ba-g blazorwasm test, unrelated to this CLI-only change |
|
/ba-g unrelated test failure |
Reconcile the aot3 branch merge with main's PR dotnet#54384 (sln AOT support): - AotSourceFiles.props: remove duplicate CommandBase.cs and duplicate CliStrings/CliCommandStrings EmbeddedResource items left by the merge; adopt main's grouped layout with a dedicated 'sdk check' command group. - NativeEntryPoint.cs: fix garbled ExecuteCore (out-of-scope parseResult) by using main's CommandNotAvailableInAotException fall-through structure plus the DotnetRoot assignment needed by sdk check. - Resolve CS0433 EnvironmentProvider ambiguity (now that the sln command references Microsoft.DotNet.Cli.Definitions, which also compiles EnvironmentProvider.cs): add Aliases=DotNetNativeWrapper,global to the NativeWrapper references in dotnet-aot.csproj and dotnet-aot.Tests.csproj, and make SdkCheckCommand.cs use the extern alias unconditionally. All three projects build clean (0 warnings/0 errors); 44 AOT tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Add Native AOT support for three
dotnet slnsubcommands:list,migrate, andremove. These commands operate on solution files using pure file I/O and theMicrosoft.VisualStudio.SolutionPersistencepackage — no MSBuild or NuGet dependencies required.Changes
New file:
src/Cli/dotnet-aot/CliStrings.csString shim providing the subset of
CliStringsproperties needed by linked source files (SlnFileFactory.cs,Parser.cs) in the AOT project context, where the full.resxauto-generation doesn't work for linked resources.Modified:
src/Cli/dotnet-aot/dotnet-aot.csprojSlnFileFactory.csandSlnfFileHelper.csfrom the main dotnet projectMicrosoft.VisualStudio.SolutionPersistencepackage referenceCliStrings.csshimModified:
src/Cli/dotnet/Parser.cs(#if CLI_AOTsection)slncommand withlist,migrate, andremovesubcommandsSLN_FILEargument on parentslncommand (matching managed CLI structure)sln remove: includes project removal from.slnand.slnffiles, empty folder cleanup, directory-to-project resolution, and misplaced-argument validationdotnet sln(no subcommand) falls back to managed CLI for complete helpDesign decisions
SLN_FILEis on the parentslncommand, not subcommands, matching managed CLI. This prevents parsing ambiguity withsln removewhere an optional first arg + required variadic arg could misparse.sln add) produce parse errors, causingNativeEntryPoint.csto fall back to manageddotnet.dll. Baredotnet slnalso falls back for complete help.--versionand--info.GetProjectFileFromDirectory: The managed version delegates throughMsbuildProjecttoProjectLocator, but the actual logic is pure file I/O (dir.GetFiles("*proj")). Inlined to avoid pulling inMsbuildProjectdependencies.Testing
dotnet-aotand maindotnetprojects build with 0 warnings, 0 errorsbuild.cmdcompletes (only pre-existing ApiCompat file-locking errors unrelated to this change)