Restore runtime bootstrapper handling for binary modules#309
Restore runtime bootstrapper handling for binary modules#309PrzemyslawKlys merged 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95e4ca589f
ℹ️ 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".
| string moduleName, | ||
| ExportSet exports, | ||
| IReadOnlyList<string>? exportAssemblies, | ||
| bool handleRuntimes = false) |
There was a problem hiding this comment.
Pass handleRuntimes in every bootstrapper regeneration path
The new optional parameter defaults handleRuntimes to false, which lets older call sites silently compile without propagating runtime handling. ModulePipelineRunner.TryRegenerateBootstrapperFromManifest still calls ModuleBootstrapperGenerator.Generate(buildResult.StagingPath, moduleName, exports, exportAssemblies) (without the new argument), so in runs where that manifest-phase regeneration executes (for example when merge is disabled), it overwrites the earlier bootstrapper and drops the runtime PATH block even when NETHandleRuntimes was requested.
Useful? React with 👍 / 👎.
| new[] | ||
| { | ||
| "# Ensure native runtime libraries are discoverable on Windows", | ||
| "if ($IsWindows -and $LibFolder) {", |
There was a problem hiding this comment.
Replace
$IsWindows gate in runtime bootstrap block
The generated runtime handler is guarded by if ($IsWindows -and $LibFolder), but $IsWindows is only built-in on PowerShell Core. In Windows PowerShell 5.1 this evaluates as undefined/false (and can error under strict mode), so NETHandleRuntimes does not actually prepend the native runtime folder to PATH on Desktop hosts, which is exactly where net472 binary modules may still need native dependency resolution.
Useful? React with 👍 / 👎.
Clarifying ReviewA prior automated review (also attributed to "claude") was posted with some inaccurate observations. I want to correct the record before those points cause unnecessary churn. Corrections to the previous reviewPoint 1 (PATH substring match) — incorrect. The actual code at $PathEntries = if ([string]::IsNullOrWhiteSpace($env:PATH)) { @() } else { @($env:PATH -split [IO.Path]::PathSeparator) }
if ((Test-Path -LiteralPath $NativePath) -and ($PathEntries -notcontains $NativePath)) {This is already the robust approach the previous review recommended adding. Point 4 (silent fallback architecture) — incorrect. The Default {
Write-Warning -Message ("Unknown Windows architecture. Falling back to win-x64 native runtime probing.")
'win-x64'
}This is also verified by the test: Actual assessmentThe implementation is solid. A few genuine observations: 1. LibFolder empty-string guard works correctly. When only a Core-layout exists and the module loads on Desktop edition, 2. BuildRuntimeHandlerBlock is inline C# code, not a template file. All other bootstrapper scripts use embedded .ps1 template files (BinaryLoader.Template.ps1, ScriptLoader.Template.ps1). This block is hardcoded in a C# string array. Functional, but slightly inconsistent with the pattern. Could be extracted to a RuntimeHandler.Template.ps1 if the block grows. 3. Process-wide PATH modification is expected. Prepending to 4. Test coverage is thorough. The four generator tests and SummaryThe flag propagation chain is consistent with DoNotCopyLibrariesRecursively and other existing flags. The generated PowerShell is correct and well-tested. No blocking issues found. |
Summary
Testing
ModuleBootstrapperGeneratorTests|FullyQualifiedNameModulePipelineExportAssemblyInferenceTests"