Add new arch input - #4
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a new architecture input to the setup-dotnet action, allowing users to install .NET SDK for different CPU architectures (e.g., cross-compilation scenarios). The implementation adds architecture validation, normalization, and passes the architecture parameter to the underlying dotnet-install scripts.
Changes:
- Added
architectureinput to action.yml with support for x64, x86, arm64, amd64, arm, s390x, ppc64le, and riscv64 - Implemented architecture input handling and validation in setup-dotnet.ts with error messages for unsupported values
- Modified DotnetCoreInstaller to accept and use architecture parameter, including custom install directory logic for cross-architecture scenarios
- Added normalizeArch utility function to normalize 'amd64' to 'x64'
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| action.yml | Adds architecture input parameter with description and supported values |
| src/setup-dotnet.ts | Implements getArchitectureInput validation function, passes architecture to installer, sets DOTNET_INSTALL_DIR for cross-arch scenarios |
| src/installer.ts | Adds useArchitecture method, normalizeArch function, modifies DotnetCoreInstaller to handle architecture parameter and custom install directories |
| dist/setup/index.js | Compiled/bundled version of TypeScript source changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const architectureArguments = | ||
| this.architecture && | ||
| normalizeArch(this.architecture) !== normalizeArch(os.arch()) | ||
| ? [ | ||
| IS_WINDOWS | ||
| ? '-InstallDir' | ||
| : '--install-dir', | ||
| IS_WINDOWS | ||
| ? `"${path.join(DotnetInstallDir.dirPath, this.architecture)}"` | ||
| : path.join(DotnetInstallDir.dirPath, this.architecture) | ||
| ] | ||
| : []; |
There was a problem hiding this comment.
The architectureArguments logic uses the unnormalized architecture value (this.architecture) when constructing the install directory path, while the condition checks normalized values. This inconsistency could cause issues. For example, if a user specifies 'amd64', it would be normalized to 'x64' for comparison but the directory would be named 'amd64'. This should use the normalized architecture value for consistency: normalizeArch(this.architecture).
| const architectureArguments = | |
| this.architecture && | |
| normalizeArch(this.architecture) !== normalizeArch(os.arch()) | |
| ? [ | |
| IS_WINDOWS | |
| ? '-InstallDir' | |
| : '--install-dir', | |
| IS_WINDOWS | |
| ? `"${path.join(DotnetInstallDir.dirPath, this.architecture)}"` | |
| : path.join(DotnetInstallDir.dirPath, this.architecture) | |
| ] | |
| : []; | |
| const normalizedArchitecture = | |
| this.architecture && normalizeArch(this.architecture); | |
| const architectureArguments = | |
| normalizedArchitecture && | |
| normalizedArchitecture !== normalizeArch(os.arch()) | |
| ? [ | |
| IS_WINDOWS ? '-InstallDir' : '--install-dir', | |
| IS_WINDOWS | |
| ? `"${path.join(DotnetInstallDir.dirPath, normalizedArchitecture)}"` | |
| : path.join(DotnetInstallDir.dirPath, normalizedArchitecture) | |
| ] | |
| : []; |
| ] | ||
| : []; | ||
| /** | ||
| * Install dotnet runitme first in order to get |
There was a problem hiding this comment.
Typo in comment: 'runitme' should be 'runtime'.
| * Install dotnet runitme first in order to get | |
| * Install dotnet runtime first in order to get |
| 'arm', | ||
| 's390x', | ||
| 'ppc64le', | ||
| 'riscv64' |
There was a problem hiding this comment.
The supportedArchitectures list includes 'riscv64' but the official dotnet-install.sh script does not accept riscv64 as a valid architecture parameter (it's only auto-detected). Additionally, 'loongarch64' is missing from the list but is supported by the official script. Consider removing 'riscv64' and adding 'loongarch64' to match the actual capabilities of the installation scripts.
| 'riscv64' | |
| 'loongarch64' |
| const architectureArguments = | ||
| this.architecture && | ||
| normalizeArch(this.architecture) !== normalizeArch(os.arch()) | ||
| ? [ | ||
| IS_WINDOWS | ||
| ? '-InstallDir' | ||
| : '--install-dir', | ||
| IS_WINDOWS | ||
| ? `"${path.join(DotnetInstallDir.dirPath, this.architecture)}"` | ||
| : path.join(DotnetInstallDir.dirPath, this.architecture) | ||
| ] | ||
| : []; |
There was a problem hiding this comment.
The indentation and formatting of the architectureArguments assignment is inconsistent with the rest of the codebase. The ternary operator should be properly indented. Consider reformatting this to improve readability.
| const architectureArguments = | |
| this.architecture && | |
| normalizeArch(this.architecture) !== normalizeArch(os.arch()) | |
| ? [ | |
| IS_WINDOWS | |
| ? '-InstallDir' | |
| : '--install-dir', | |
| IS_WINDOWS | |
| ? `"${path.join(DotnetInstallDir.dirPath, this.architecture)}"` | |
| : path.join(DotnetInstallDir.dirPath, this.architecture) | |
| ] | |
| : []; | |
| const architectureArguments = | |
| this.architecture && | |
| normalizeArch(this.architecture) !== normalizeArch(os.arch()) | |
| ? [ | |
| IS_WINDOWS ? '-InstallDir' : '--install-dir', | |
| IS_WINDOWS | |
| ? `"${path.join(DotnetInstallDir.dirPath, this.architecture)}"` | |
| : path.join(DotnetInstallDir.dirPath, this.architecture) | |
| ] | |
| : []; |
| if ( | ||
| architecture && | ||
| normalizeArch(architecture) !== normalizeArch(os.arch()) | ||
| ) { | ||
| process.env['DOTNET_INSTALL_DIR'] = path.join( | ||
| DotnetInstallDir.dirPath, | ||
| architecture |
There was a problem hiding this comment.
Similar to the issue in installer.ts, this uses the unnormalized architecture value when constructing the path but normalized values for comparison. This should use normalizeArch(architecture) for consistency with the condition check.
| if ( | |
| architecture && | |
| normalizeArch(architecture) !== normalizeArch(os.arch()) | |
| ) { | |
| process.env['DOTNET_INSTALL_DIR'] = path.join( | |
| DotnetInstallDir.dirPath, | |
| architecture | |
| const normalizedArchitecture = architecture | |
| ? normalizeArch(architecture) | |
| : undefined; | |
| if ( | |
| normalizedArchitecture && | |
| normalizedArchitecture !== normalizeArch(os.arch()) | |
| ) { | |
| process.env['DOTNET_INSTALL_DIR'] = path.join( | |
| DotnetInstallDir.dirPath, | |
| normalizedArchitecture |
| function getArchitectureInput(): SupportedArchitecture | '' { | ||
| const raw = (core.getInput('architecture') || '').trim(); | ||
| if (!raw) return ''; | ||
| const normalized = raw.toLowerCase(); | ||
| if ((supportedArchitectures as readonly string[]).includes(normalized)) { | ||
| return normalized as SupportedArchitecture; | ||
| } | ||
| throw new Error( | ||
| `Value '${raw}' is not supported for the 'architecture' option. Supported values are: ${supportedArchitectures.join( | ||
| ', ' | ||
| )}.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new architecture functionality lacks test coverage. The existing test files (tests/installer.test.ts and tests/setup-dotnet.test.ts) contain comprehensive tests for other features, but there are no tests for the architecture input, getArchitectureInput function, normalizeArch function, or the useArchitecture method. Tests should be added to verify architecture validation, normalization, and installation behavior for cross-architecture scenarios.
| description: 'Optional SDK workloads to install for additional platform support. Examples: wasm-tools, maui, aspire.' | ||
| required: false | ||
| architecture: | ||
| description: 'Optional architecture for the .NET install. Supported values: x64, x86, arm64, amd64, arm, s390x, ppc64le, riscv64. If not set, the installer auto-detects the current system architecture.' |
There was a problem hiding this comment.
The supportedArchitectures list includes 'riscv64' but the official dotnet-install.sh script does not accept riscv64 as a valid architecture parameter. Additionally, 'loongarch64' is missing from the list but is supported by the official script. The action.yml description should be updated to match the actually supported architectures.
| description: 'Optional architecture for the .NET install. Supported values: x64, x86, arm64, amd64, arm, s390x, ppc64le, riscv64. If not set, the installer auto-detects the current system architecture.' | |
| description: 'Optional architecture for the .NET install. Supported values: x64, x86, arm64, amd64, arm, s390x, ppc64le, loongarch64. If not set, the installer auto-detects the current system architecture.' |
Description:
Describe your changes.
Related issue:
Add link to the related issue.
Check list: