Skip to content

[wasm] Align the runtime-test call-helper generator override - #133419

Merged
radekdoulik merged 3 commits into
dotnet:mainfrom
radekdoulik:radekdoulik-verbose-goggles
Sep 9, 2026
Merged

radekdoulik merged 3 commits into
dotnet:mainfrom
radekdoulik:radekdoulik-verbose-goggles

Conversation

@radekdoulik

Copy link
Copy Markdown
Member

Addresses review feedback on #131877.

Use PortableCallHelpersGeneratorPath for runtime-test corerun generation and diagnostics, matching browser/WASI app builds. Keep the existing in-repo generator fallback.

Validation

Seven target-level cases covered override precedence, fallback resolution, and diagnostics. Built a test-specific corerun and ran the native NestedStruct test on browser-wasm.

Note

This PR description was drafted with GitHub Copilot.

Use PortableCallHelpersGeneratorPath for test corerun generation and
diagnostics. Keep the existing in-repo generator fallback.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f6d6e5a4-5b25-4198-b42d-d5b2dc781f47
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is localized and consistent with existing browser/WASI targets; only a minor diagnostic ordering nit was found.

Pull request overview

Updates the WASM runtime-test corerun call-helper generation to use the shared PortableCallHelpersGeneratorPath MSBuild property (consistent with browser/WASI CoreCLR app builds), while retaining the in-repo Crossgen2InBuildDir fallback.

Changes:

  • Switch generator path override from a private _WasmCorerunGeneratorPath to $(PortableCallHelpersGeneratorPath).
  • Update diagnostics and execution (<Exec>) to reference PortableCallHelpersGeneratorPath.
  • Keep the in-repo resolution fallback via $(Crossgen2InBuildDir).
File summaries
File Description
src/tests/Common/CLRTest.WasmCorerun.targets Use PortableCallHelpersGeneratorPath for generating portable call helpers during test-specific corerun generation, with updated error messages and Exec invocation.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/tests/Common/CLRTest.WasmCorerun.targets Outdated
@radekdoulik radekdoulik added the arch-wasm WebAssembly architecture label Sep 8, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@radekdoulik radekdoulik added this to the 12.0.0 milestone Sep 8, 2026
Comment thread src/tests/Common/CLRTest.WasmCorerun.targets Outdated
Rename the portable call-helper generator override to Crossgen2Path across runtime tests and browser/WASI app builds. Update diagnostics while preserving existing fallbacks and ReadyToRun compiler-selection policy.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 8, 2026 21:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Renaming the MSBuild override property from $(PortableCallHelpersGeneratorPath) to $(Crossgen2Path) risks breaking existing out-of-repo builds unless a compatibility alias is retained.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/mono/browser/build/BrowserWasmApp.CoreCLR.targets
Comment thread src/mono/wasi/build/WasiApp.CoreCLR.targets
Comment thread src/mono/browser/build/BrowserWasmApp.CoreCLR.targets Outdated
@lewing lewing mentioned this pull request Sep 9, 2026
6 tasks
Use _Crossgen2ExeSuffix, _Crossgen2GeneratorArg, and
_Crossgen2GeneratorRsp consistently in browser, WASI, and runtime-test
call-helper targets.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 9, 2026 13:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The property rename removes prior override entry points (e.g., PortableCallHelpersGeneratorPath / _WasmCorerunGeneratorPath) without a compatibility alias, which can break existing build/test customization.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/tests/Common/CLRTest.WasmCorerun.targets:327

  • This removes the _WasmCorerunGeneratorPath override point and switches to $(Crossgen2Path). If any existing infra is setting _WasmCorerunGeneratorPath (the previous error text suggested it was meant to be configurable), this will stop working. Consider treating _WasmCorerunGeneratorPath as a legacy alias when Crossgen2Path is not set.

src/mono/browser/build/BrowserWasmApp.CoreCLR.targets:703

  • This change removes the previously supported $(PortableCallHelpersGeneratorPath) override and only honors $(Crossgen2Path). If any consumers (or existing build scripts) still set PortableCallHelpersGeneratorPath, the override will silently stop working. Consider mapping PortableCallHelpersGeneratorPath into Crossgen2Path as a backwards-compatible alias (with Crossgen2Path taking precedence).
      <_Crossgen2ExeSuffix Condition="'$(OS)' == 'Windows_NT'">.exe</_Crossgen2ExeSuffix>
      <Crossgen2Path Condition="'$(Crossgen2Path)' == '' and '$(Crossgen2InBuildDir)' != ''">$([MSBuild]::NormalizePath('$(Crossgen2InBuildDir)', 'crossgen2$(_Crossgen2ExeSuffix)'))</Crossgen2Path>
      <Crossgen2Path Condition="'$(Crossgen2Path)' == ''">$(Crossgen2ToolPath)</Crossgen2Path>

src/mono/wasi/build/WasiApp.CoreCLR.targets:163

  • This renames the crossgen2 override property from $(PortableCallHelpersGeneratorPath) to $(Crossgen2Path) (and removes the former). If PortableCallHelpersGeneratorPath has existing consumers, this is a breaking change. Consider honoring PortableCallHelpersGeneratorPath as an alias when Crossgen2Path is not set.
      <_Crossgen2ExeSuffix Condition="'$(OS)' == 'Windows_NT'">.exe</_Crossgen2ExeSuffix>
      <Crossgen2Path Condition="'$(Crossgen2Path)' == '' and '$(Crossgen2InBuildDir)' != ''">$([MSBuild]::NormalizePath('$(Crossgen2InBuildDir)', 'crossgen2$(_Crossgen2ExeSuffix)'))</Crossgen2Path>
      <Crossgen2Path Condition="'$(Crossgen2Path)' == ''">$(Crossgen2ToolPath)</Crossgen2Path>
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@radekdoulik
radekdoulik merged commit bd707a4 into dotnet:main Sep 9, 2026
103 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-Infrastructure

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants