Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to force the architecture #338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kevingosse
Copy link

@kevingosse kevingosse commented Oct 21, 2022

Description:
Add an optional architecture parameter to set the target architecture (x86, x64, arm64, ...).

Related issue:
#72

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@@ -28,6 +28,11 @@ export async function run() {
//
const versions = core.getMultilineInput('dotnet-version');
const installedDotnetVersions: string[] = [];
let architecture = core.getInput('architecture');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let architecture = core.getInput('architecture');
const architecture = core.getInput('architecture');

Comment on lines +33 to +35
if (!architecture) {
architecture = '';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we need this check here. If the architecture input is not supplied function core.getInput() will return an empty string by default.

constructor(
version: string,
quality: QualityOptions,
architecture: string = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
architecture: string = ''
architecture: string

Do we need a default value here? As I wrote in another comment, the default value for the empty input will be an empty string.

@@ -230,6 +236,10 @@ export class DotnetCoreInstaller {
if (this.quality) {
this.setQuality(dotnetVersion, scriptArguments);
}

if (this.architecture != '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (this.architecture != '') {
if (this.architecture) {

Comment on lines +239 to +242

if (this.architecture != '') {
scriptArguments.push('--architecture', this.architecture);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding the same code for Windows.

Comment on lines +20 to +22
architecture:
description: 'Optional architecture to use. If not provided, will default to the OS architecture.'
required: False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
architecture:
description: 'Optional architecture to use. If not provided, will default to the OS architecture.'
required: False
architecture:
description: 'Optional architecture of the .NET binaries to install. Possible values: amd64, x64, x86, arm64, arm, and s390x, if not provided, defaults to the OS architecture.'

@IvanZosimov
Copy link
Contributor

Hi, @kevingosse 👋 ! Thank you for the PR, I left some comments below and I'd also suggest making changes to the documentation.

@IvanZosimov
Copy link
Contributor

Just a gentle ping, @kevingosse

@lwcross
Copy link

lwcross commented Mar 28, 2023

Review to be implementation

@IvanZosimov IvanZosimov linked an issue Jul 3, 2023 that may be closed by this pull request
@actions actions deleted a comment from Tico7793 Jul 27, 2023
@IvanZosimov
Copy link
Contributor

Hi, @kevingosse, just a gentle ping 📟

@kevingosse
Copy link
Author

Hey. Sorry I don't have time to work on this anymore. Happy to leave somebody else take over the work. I can close the PR if it creates noise.

@IvanZosimov
Copy link
Contributor

Hi, @kevingosse 👋 Thanks for the information! No worries, your PR doesn't create any noise. Let it be here, until we can proceed with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for target runtimes.
5 participants