Skip to content

Update and create documentation in build.cmd and build.sh for CoreCLR test builds#126951

Open
Copilot wants to merge 4 commits intomainfrom
copilot/update-documentation-scripts
Open

Update and create documentation in build.cmd and build.sh for CoreCLR test builds#126951
Copilot wants to merge 4 commits intomainfrom
copilot/update-documentation-scripts

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 15, 2026

Summary

Updates the help text/documentation in src/tests/build.cmd and src/tests/build.sh for the CoreCLR test build scripts.

Changes

src/tests/build.cmd

  • Added wasm to the list of valid build architectures in the usage section
  • Added an OS targeting section documenting os <value>, browser (shorthand for os browser), and wasi (shorthand for os wasi) options
  • Added -CoreCLR to the usage section (the option was implemented in the code but missing from the help text)

src/tests/build.sh

  • Added an OS targeting section to usage_list documenting the -os <value> option (handled by the common build framework), listing common OS values, and noting that native test components are automatically skipped for mobile/device targets (android, ios, iossimulator, tvos, tvossimulator)
  • Added -coreclr to usage_list (the option was implemented in the code but missing from the help text)
  • Fixed escaped characters (^(, ^)) and Windows-style backslashes in path examples that were incorrectly copied from the Windows batch script

Command sync check

Option build.cmd build.sh
OS targeting os <value>, browser, wasi -os <value> (via common framework)
Architecture x64, x86, arm64, wasm Same (via common framework)
-CoreCLR / -coreclr ✅ (now documented) ✅ (now documented)
-Mono / -ExcludeMonoFailures
-mono_aot, -mono_fullaot ❌ (Linux/macOS only)
-runtests ❌ (Windows: run manually via run.cmd)
-MSBuild Use -ninja false via common framework
-PDB ✅ (Windows-only PDB format) ❌ (not applicable)

Note

This PR was generated by GitHub Copilot.

…ommand sync

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/0ff1f33d-17b4-43db-aa5f-b488e6f5f1a5

Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Copy link
Copy Markdown
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Add test build script docs for os specification.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the user-facing help text in the CoreCLR test build scripts to better document supported options (especially OS targeting, WASM architecture, and CoreCLR-vs-Mono selection), and fixes a few shell-usage formatting issues.

Changes:

  • Documented OS targeting options in src/tests/build.cmd and src/tests/build.sh (including browser/WASI shorthands on Windows, and -os via the common framework on Unix).
  • Added missing -CoreCLR / -coreclr help entries (option already existed in argument parsing).
  • Corrected path/escaping examples in build.sh that were copied from the Windows batch script.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/tests/build.sh Adds OS-targeting and -coreclr help text; fixes path/escaping in usage examples.
src/tests/build.cmd Expands usage docs to include wasm, OS targeting options, and -CoreCLR.

Comment thread src/tests/build.sh Outdated
davidwrighton and others added 2 commits April 16, 2026 07:59
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 16, 2026 15:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread src/tests/build.sh Outdated
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126951

Note

This review was generated by GitHub Copilot using multi-model analysis (Claude Opus 4.6 primary, Claude Sonnet 4 and GPT-5.3-Codex sub-agents).

Holistic Assessment

Motivation: The PR documents already-implemented but undocumented options (-os, -coreclr/-CoreCLR, wasm architecture) in src/tests/build.cmd and src/tests/build.sh. It also fixes incorrect Windows batch escape sequences (^(, ^)) and backslash paths (src\tests) that were present in the bash script's help text. The motivation is clear and justified — these are genuine gaps in the user-facing help output.

Approach: Documentation-only changes to help text strings. The approach is appropriate and low-risk.

Summary: ⚠️ Needs Human Review. The changes are correct and all documented options match their implementations in code. One cross-script inconsistency (maccatalyst listed in build.cmd but omitted from build.sh) was flagged by a prior review and by both sub-agent models but remains unaddressed. A human reviewer should decide whether this is worth fixing in-PR or is acceptable as-is given the "Common values" qualifier.


Detailed Findings

✅ Correctness — All documented options verified against implementation

All newly documented options match the actual parsing code:

  • -coreclr/-CoreCLR: Parsed at build.sh:297 and build.cmd:115 — sets __Mono=0
  • -os <value>: Handled via common framework at eng/native/build-commons.sh:498
  • wasm architecture: Parsed at build.cmd:70
  • browser/wasi shorthands: Parsed at build.cmd:73-74
  • Mobile target auto-skip: Implemented at build.sh:61-62 for android, ios, iossimulator, tvos, tvossimulator — matches documentation exactly

Flagged by: all 3 models

✅ Shell syntax fix — Correct removal of Windows artifacts from bash script

The three usage_list entries for -test/-dir/-tree previously contained:

  1. Windows batch escape sequences (^(, ^)) — meaningless in bash
  2. Windows-style backslash paths (src\tests) — should be src/tests in Unix
  3. Trailing semicolons (;) after the closing quote — unnecessary in bash and inconsistent with all other entries

All three issues are correctly fixed. The semicolon fix was addressed in the second commit after a prior review flagged it.

Flagged by: all 3 models

⚠️ Inconsistency — maccatalyst omitted from build.sh OS list (advisory, not merge-blocking)

build.cmd (line 363) lists maccatalyst as a common OS value, but build.sh (line 141) omits it. The common framework fully supports maccatalyst (eng/native/build-commons.sh:570, eng/build.sh:285-286). This was flagged in a prior review comment and remains unaddressed.

Mitigating factor: The list is prefaced with "Common values" (not "all values"), so the omission isn't technically incorrect. However, consistency between the two scripts' documentation would be ideal.

Suggested fix (if addressing): Change line 141 of build.sh to:

usage_list+=("    ios, iossimulator, tvos, tvossimulator, maccatalyst, browser, wasi.")

Flagged by: all 3 models

💡 Observation — Orphaned text in build.cmd (pre-existing, out of scope)

Line 385 of build.cmd (Set to "" to disable default exclusion file.) was originally a continuation of the -ExcludeMonoFailures entry but is now visually separated from it by the new -CoreCLR line (383). This makes it slightly confusing, but it's a pre-existing issue and not something this PR needs to fix.

Generated by Code Review for issue #126951 ·

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 17, 2026 10:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread src/tests/build.cmd
Comment on lines +364 to +365
echo browser: Shorthand for "os browser" ^(targets WebAssembly in the browser^).
echo wasi: Shorthand for "os wasi" ^(targets WebAssembly System Interface^).
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

The new OS targeting help for browser/wasi reads like those options alone “target WebAssembly”, but this script’s default architecture is still x64 unless the user also passes wasm. Consider clarifying in the help text that browser/wasi typically need to be combined with the wasm architecture (e.g., wasm browser / wasm wasi) to actually build the WebAssembly target.

Suggested change
echo browser: Shorthand for "os browser" ^(targets WebAssembly in the browser^).
echo wasi: Shorthand for "os wasi" ^(targets WebAssembly System Interface^).
echo browser: Shorthand for "os browser" ^(typically combine with "wasm", for example "wasm browser"^).
echo wasi: Shorthand for "os wasi" ^(typically combine with "wasm", for example "wasm wasi"^).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants