Add module-scoped ALC bootstrapper generation#354
Conversation
Code Review: Add module-scoped ALC bootstrapper generationOverviewThis PR adds an opt-in Positives
Issues & Suggestions1.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af2f8451a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static string BuildAssemblyLoadContextProject(AssemblyLoadContextLoaderIdentity identity) | ||
| => $@"<Project Sdk=""Microsoft.NET.Sdk""> | ||
| <PropertyGroup> | ||
| <TargetFramework>net6.0</TargetFramework> |
There was a problem hiding this comment.
The temporary loader project is always emitted as net6.0, which forces dotnet build to resolve .NET 6 reference packs even when the host build is on newer SDKs. In CI/offline environments (or agents without cached/net6 targeting packs), this makes UseAssemblyLoadContext builds fail during restore/build despite the module itself targeting newer frameworks. Consider targeting the current build TFM (or a bundled/prebuilt loader) to remove this fragile dependency.
Useful? React with 👍 / 👎.
| "NETBinaryModuleDocumentation": { "type": ["boolean", "null"] }, | ||
| "HandleRuntimes": { "type": ["boolean", "null"] }, | ||
| "UseAssemblyLoadContext": { "type": ["boolean", "null"] }, | ||
| "NETAssemblyLoadContext": { "type": ["boolean", "null"] }, |
There was a problem hiding this comment.
The schema now advertises NETAssemblyLoadContext, but the typed JSON segment model only exposes UseAssemblyLoadContext (no matching property or alias mapping), so JSON configs that set NETAssemblyLoadContext are silently ignored during deserialization. This creates a contract mismatch where validated config does not actually enable ALC loading.
Useful? React with 👍 / 👎.
| private static string BuildAssemblyLoadContextProject(AssemblyLoadContextLoaderIdentity identity) | ||
| => $@"<Project Sdk=""Microsoft.NET.Sdk""> | ||
| <PropertyGroup> | ||
| <TargetFramework>net6.0</TargetFramework> |
There was a problem hiding this comment.
The temporary loader project is always emitted as net6.0, which forces a .NET 6 targeting-pack restore even when the build uses newer SDK/runtime stacks. In offline or minimal CI agents this makes UseAssemblyLoadContext builds fail solely because net6 reference packs are unavailable, despite the module itself targeting other TFMs.
Useful? React with 👍 / 👎.
| "dotnet", | ||
| buildRoot, | ||
| new[] { "build", projectPath, "-c", "Release", "-o", outputRoot, "-nologo", "-v:minimal" }, | ||
| TimeSpan.FromMinutes(2)); |
There was a problem hiding this comment.
The generated loader build is capped at a hard 2-minute timeout. First-run restores, cold package caches, or slower CI runners can easily exceed this and produce exit code 124, causing module builds to fail nondeterministically even though dotnet build would complete successfully with a normal timeout window.
Useful? React with 👍 / 👎.
| if (hasLib && useAssemblyLoadContext) | ||
| { | ||
| BuildAssemblyLoadContextLoader(root, moduleName); | ||
| } | ||
| else if (hasLib) |
There was a problem hiding this comment.
When useAssemblyLoadContext is enabled, this branch skips generating <Module>.Libraries.ps1 entirely. The new ALC bootstrapper still uses the normal Desktop import path, but without the old libraries script dot-source step, extra module DLLs are no longer preloaded on Windows PowerShell. Modules that relied on those additional assemblies being loaded can regress even though the option is intended to affect Core loading behavior.
Useful? React with 👍 / 👎.
| "NETBinaryModuleDocumentation": { "type": ["boolean", "null"] }, | ||
| "HandleRuntimes": { "type": ["boolean", "null"] }, | ||
| "UseAssemblyLoadContext": { "type": ["boolean", "null"] }, | ||
| "NETAssemblyLoadContext": { "type": ["boolean", "null"] }, |
There was a problem hiding this comment.
The schema now advertises NETAssemblyLoadContext, but the typed JSON segment model only exposes UseAssemblyLoadContext and doesn't map this alias during deserialization. As a result, JSON configs that pass schema validation with NETAssemblyLoadContext silently do nothing, which is a contract mismatch for users authoring pipeline JSON directly.
Useful? React with 👍 / 👎.
| private static string BuildAssemblyLoadContextProject(AssemblyLoadContextLoaderIdentity identity) | ||
| => $@"<Project Sdk=""Microsoft.NET.Sdk""> | ||
| <PropertyGroup> | ||
| <TargetFramework>net6.0</TargetFramework> |
There was a problem hiding this comment.
The temporary loader project is always emitted as net6.0, which forces a .NET 6 targeting-pack restore even when the build uses newer SDK/runtime stacks. In offline or minimal CI agents this can make UseAssemblyLoadContext builds fail solely because net6 reference packs aren't available, despite the module itself targeting other frameworks.
Useful? React with 👍 / 👎.
| "dotnet", | ||
| buildRoot, | ||
| new[] { "build", projectPath, "-c", "Release", "-o", outputRoot, "-nologo", "-v:minimal" }, | ||
| TimeSpan.FromMinutes(2)); |
There was a problem hiding this comment.
The generated loader build is capped at a hard 2-minute timeout. First-run restores, cold package caches, or slower CI runners can exceed this and return a timeout exit code, causing nondeterministic module build failures even when dotnet build would otherwise complete successfully.
Useful? React with 👍 / 👎.
Code Review: Add module-scoped ALC bootstrapper generation
OverviewThis PR adds an opt-in What the diff actually contains (correcting the record)A few things the earlier automated review got wrong — important to note for anyone reading that comment:
Genuine issues1.
|
Code Review — PR #354: Add module-scoped ALC bootstrapper generationOverviewThis PR adds an opt-in What's good
Issues1. Hardcoded
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0656e01b84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var targetDirectories = Directory.EnumerateDirectories(libRoot) | ||
| .Where(directory => !string.Equals(Path.GetFileName(directory), "Default", StringComparison.OrdinalIgnoreCase)) | ||
| .ToArray(); | ||
| if (targetDirectories.Length == 0) return; |
There was a problem hiding this comment.
When Lib contains only Default, this filter leaves targetDirectories empty and returns without creating any *.ModuleLoadContext.dll, but the generated ALC bootstrapper still tries to Add-Type that loader on Core import. In that layout, enabling UseAssemblyLoadContext produces a deterministic module import failure because the referenced loader DLL is never emitted.
Useful? React with 👍 / 👎.
| private static string BuildAssemblyLoadContextProject(AssemblyLoadContextLoaderIdentity identity) | ||
| => $@"<Project Sdk=""Microsoft.NET.Sdk""> | ||
| <PropertyGroup> | ||
| <TargetFramework>net8.0</TargetFramework> |
There was a problem hiding this comment.
The temporary loader project is hardcoded to net8.0, so ALC mode produces a loader assembly that cannot be loaded by older PowerShell Core runtimes (for example, PS 7.2/7.3 on .NET 6/7), even if the module itself is otherwise compatible there. Fresh evidence in this revision is the new fixed <TargetFramework>net8.0</TargetFramework> value, which introduces host-version-coupled import failures for UseAssemblyLoadContext.
Useful? React with 👍 / 👎.
| ExcludeLibraryFilter = excludeLibraryFilterFromSegments ?? spec.Build.ExcludeLibraryFilter ?? Array.Empty<string>(), | ||
| DoNotCopyLibrariesRecursively = doNotCopyLibrariesRecursivelyFromSegments ?? spec.Build.DoNotCopyLibrariesRecursively, | ||
| HandleRuntimes = handleRuntimesFromSegments ?? spec.Build.HandleRuntimes, | ||
| UseAssemblyLoadContext = useAssemblyLoadContextFromSegments ?? spec.Build.UseAssemblyLoadContext, |
There was a problem hiding this comment.
This commit forwards UseAssemblyLoadContext into ModuleBuildSpec, but the missing-csproj gate still does not consider that flag, so a config can explicitly request ALC loading while CsprojPath is unresolved and the pipeline will continue by reusing whatever Lib payload happens to be present. That bypasses the existing safeguard meant to fail fast on explicit binary-build settings and can silently ship stale binaries.
Useful? React with 👍 / 👎.
| if (hasLib && useAssemblyLoadContext) | ||
| BuildAssemblyLoadContextLoader(root, moduleName); | ||
|
|
||
| if (hasLib) | ||
| { | ||
| var librariesPath = Path.Combine(root, $"{moduleName}.Libraries.ps1"); |
There was a problem hiding this comment.
ALC mode now builds the loader DLL before generating <Module>.Libraries.ps1, and the libraries script includes every *.dll it finds under each Lib/<folder>. In layouts where Desktop resolves to Standard (for example, no Default folder), this causes the Desktop path to Add-Type the ALC loader assembly even though it is Core-only infrastructure, which can break import on Desktop hosts.
Useful? React with 👍 / 👎.
| var result = RunProcess( | ||
| "dotnet", | ||
| buildRoot, | ||
| new[] { "build", projectPath, "-c", "Release", "-o", outputRoot, "-nologo", "-v:minimal" }, | ||
| AssemblyLoadContextLoaderBuildTimeout); |
There was a problem hiding this comment.
Enabling UseAssemblyLoadContext now always shells out to dotnet build during bootstrapper generation. That introduces a new hard runtime dependency on a local .NET SDK even for module builds that intentionally use prebuilt Lib payloads with no CsprojPath, turning previously valid no-SDK packaging flows into hard failures.
Useful? React with 👍 / 👎.
Code Review: Add module-scoped ALC bootstrapper generationOverviewThis PR adds an opt-in
The approach is architecturally sound and follows existing project conventions well. The dual naming ( Issues & SuggestionsMedium Priority1. 2. Static private static readonly Dictionary<string, ModuleAssemblyLoadContext> Contexts = new(...);Entries are added but never removed. Since 3. Private reflection API risk in the template $AddExportedCmdlet = [System.Management.Automation.PSModuleInfo].GetMethod(
'AddExportedCmdlet',
[System.Reflection.BindingFlags]'Instance, NonPublic'
)This is the standard community pattern for ALC-based modules and the warning fallback handles the unavailable case gracefully. However, the template comment only explains what is being done; it would be worth also noting which PowerShell versions this is known to work on (tested range), so future maintainers know when to reassess. 4. Hardcoded return candidates.FirstOrDefault() ?? "net8.0";As net10.0 becomes the LTS baseline, targeting Minor5. 6. Silent no-op when private static void BuildAssemblyLoadContextLoader(...)
{
if (!Directory.Exists(libRoot)) return; // silentA misconfigured module (e.g., 7. private sealed class AssemblyLoadContextLoaderIdentity { ... }The codebase uses records elsewhere. This class is immutable with three string properties — a 8. Plan.cs: if (bl.UseAssemblyLoadContext.HasValue)
useAssemblyLoadContextFromSegments = bl.UseAssemblyLoadContext.Value;
else if (bl.NETAssemblyLoadContext.HasValue)
useAssemblyLoadContextFromSegments = bl.NETAssemblyLoadContext.Value;Could be simplified to match the existing useAssemblyLoadContextFromSegments = bl.UseAssemblyLoadContext ?? bl.NETAssemblyLoadContext ?? useAssemblyLoadContextFromSegments;Minor readability point. What's done well
Overall this is a well-considered feature. The main asks before merging are the |
Code Review: Add module-scoped ALC bootstrapper generationOverall this is a well-structured feature that follows the existing patterns in the codebase. The ALC isolation approach is the right solution for PowerShell module dependency conflicts. Here is my detailed feedback. What the PR Does
IssuesMissing
ModuleBootstrapperGenerator.Generate(
root,
moduleName,
exports,
exportAssemblies,
handleRuntimes,
plan?.BuildSpec.UseAssemblyLoadContext ?? false, // ✓
conditionalExportDependencies); // targetFrameworks not passed
Hardcoded build timeout private static readonly TimeSpan AssemblyLoadContextLoaderBuildTimeout = TimeSpan.FromMinutes(5);Five minutes may not be enough on a cold NuGet cache (first run, CI with no package cache). The main Static private static readonly Dictionary<string, ModuleAssemblyLoadContext> Contexts = new(...);The dictionary is keyed by the full assembly path and the ALC is SuggestionsLock scope in lock (Sync)
{
if (!Contexts.TryGetValue(fullPath, out var context))
{
context = new ModuleAssemblyLoadContext(...);
Contexts[fullPath] = context;
}
return context.LoadMainModule(); // ← inside the lock
}
Duplicate fallback logic in The private static bool? GetAssemblyLoadContext(IDictionary dict) =>
HasKey(dict, "UseAssemblyLoadContext") ? GetBool(dict, "UseAssemblyLoadContext")
: HasKey(dict, "NETAssemblyLoadContext") ? GetBool(dict, "NETAssemblyLoadContext")
: null;Integration test needs
Positive Notes
SummaryThe main actionable item is passing |
Code Review — PR #354: Add module-scoped ALC bootstrapper generationOverviewThis PR adds an opt-in
Architecture and layer placement are correct — core logic lives in Potential Issues1. In if (effectiveUseAssemblyLoadContext)
reasons.Add("NETAssemblyLoadContext");This means even when the user writes 2. $AddExportedCmdlet = [System.Management.Automation.PSModuleInfo].GetMethod(
'AddExportedCmdlet',
[System.Reflection.BindingFlags]'Instance, NonPublic'
)This relies on a private internal API that could be removed in a future PowerShell version. The template does have a 3. Blocking private static ProcessRunResult RunProcess(...) =>
new ProcessRunner().RunAsync(...).GetAwaiter().GetResult();Synchronously blocking on a 4. Hardcoded <AssemblyVersion>1.0.0.0</AssemblyVersion>
<FileVersion>1.0.0.0</FileVersion>
<InformationalVersion>1.0.0</InformationalVersion>The loader version is not linked to the parent module version. This is fine functionally (the loader is an opaque build artifact), but could be surprising when inspecting file properties in the installed module directory. Passing through the module's version would make tracing easier. 5. Missing if (hasLib && useAssemblyLoadContext && assemblyLoadContextLoaderIdentity is not null)
BuildAssemblyLoadContextLoader(root, assemblyLoadContextLoaderIdentity, ...);The 6. if (byName.TryGetValue("Standard", out var standard))
return new[] { standard };
if (byName.TryGetValue("Core", out var core))
return new[] { core };The single-directory result is intentional and matches the template's Minor Observations
Test CoverageTests are solid for the main paths:
Suggested additions:
SummaryThe implementation is well-structured, follows project layering rules, and solves a real problem (ALC isolation for binary modules). The core logic is correct and the test coverage is meaningful. The main actionable items are:
Everything else is minor or informational. Good PR overall. |
Code Review — PR #354: Add module-scoped ALC bootstrapper generationOverviewThis PR adds an opt-in Bugs / Risks1. Silent cmdlet-export failure when
|
Code Review – PR #354: Add module-scoped ALC bootstrapper generationOverall: A well-structured, opt-in feature that follows the project's layering guidelines closely. Business logic is correctly placed in What the PR does
Issues / Suggestions🔴 1 – Hard failure when
|
| Area | Covered |
|---|---|
ResolveAssemblyLoadContextTargetFramework |
✅ |
ResolveAssemblyLoadContextTargetDirectories |
✅ |
| Factory mapping of new flag | ✅ |
| Legacy alias deserialization in Plan | ✅ |
| Generator output with Core/Default layouts | ✅ |
EnsureDotNetSdkAvailable / actual dotnet build |
❌ (reasonable exclusion) |
SanitizeAssemblyName / ToCSharpIdentifier edge cases |
❌ (low risk) |
Summary
The architecture is solid and the layering follows AGENTS.md conventions correctly. The two items worth fixing before merge are:
- The hard-
throwfallback forAddExportedCmdlet(should warn + gracefully fall back), and - The
.GetAwaiter().GetResult()call site (deadlock risk in PS runspace).
The remaining items are improvements that can be addressed in follow-up PRs.
Code Review — PR #354: Add module-scoped ALC bootstrapper generationOverviewThis PR adds an opt-in The implementation is backward-compatible (opt-in, legacy alias handled in both the JSON adapter and the plan accumulator) and the test coverage is meaningfully expanded. Issues and Suggestions1. Sync-over-async in
|
1 participant
Summary
NETAssemblyLoadContext/UseAssemblyLoadContextbuild setting for binary modulesPSParseHTML.ModuleLoadContext.dll, instead of sharing a common loader assembly across modulesValidation
dotnet build .\PSPublishModule\PSPublishModule.csproj -c Debug -f net8.0dotnet build .\PowerForge.Tests\PowerForge.Tests.csproj -c Debugdotnet test .\PowerForge.Tests\PowerForge.Tests.csproj -c Debug --no-build --filter "FullyQualifiedName~BuildConfigurationFactoryTests|FullyQualifiedName~ModulePipelineExportAssemblyInferenceTests|FullyQualifiedName~ModuleBootstrapperGeneratorTests"git diff --check