Add a way to force the architecture by kevingosse · Pull Request #338 · actions/setup-dotnet

Add a way to force the architecture by kevingosse · Pull Request #338 · actions/setup-dotnet

Conversation

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.

… (x86, x64, arm64, ...).

		//
		const versions = core.getMultilineInput('dotnet-version');
		const installedDotnetVersions: string[] = [];
		let architecture = core.getInput('architecture');

Choose a reason for hiding this comment

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

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

Comment on lines +33 to +35

		if (!architecture) {
		architecture = '';
		}

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 = ''

Choose a reason for hiding this comment

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

	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.

		this.setQuality(dotnetVersion, scriptArguments);
		}

		if (this.architecture != '') {

Choose a reason for hiding this comment

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

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

Comment on lines +239 to +242


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

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

Choose a reason for hiding this comment

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

	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.'

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

Review to be implementation

IvanZosimov linked an issue

that may be closed by this pull request

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.

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.

Why is that still open? We need setting architecture and DOTNET_ROOT(x86) environment variable, please.