[Bug] installer.args, Invoke-ExternalCommand unexpected bug

[HUMORCE]

Bug Report

manifest: extras/ida-free

A:

...
    "installer": {
        "args": [
            "--mode",
            "unattended",
            "--prefix",
            "$dir"
        ]
    },
...

B:

...
    "installer": {
        "args": [
            "--mode unattended",
            "--prefix $dir",
        ]
    },
...

when I put some debug code around L788 of install.ps1 ,the debug output of both mafenist are same, but A works, B fails.

Scoop/lib/install.ps1

Lines 778 to 798 in 8aee6f9

	function install_prog($fname, $dir, $installer, $global) {
	$prog = "$dir\$(coalesce $installer.file "$fname")"
	if(!(is_in_dir $dir $prog)) {
	abort "Error in manifest: Installer $prog is outside the app directory."
	}
	$arg = @(args $installer.args $dir $global)
	if($prog.endswith('.ps1')) {
	& $prog @arg
	} else {
	$installed = Invoke-ExternalCommand $prog $arg -Activity "Running installer..."
	if(!$installed) {
	abort "Installation aborted. You might need to run 'scoop uninstall $app' before trying again."
	}
	# Don't remove installer if "keep" flag is set to true
	if(!($installer.keep -eq "true")) {
	Remove-Item $prog
	}
	}
	}

related: ScoopInstaller/Extras#9603

Current Behavior

the installer of app cannot be executed properly with the correct args.

Expected Behavior

pass args normally, installation success.

Additional context/output

reproduced when install an older version of extras/ida-free too. e.g. v7.7

Possible Solution

cd (scoop prefix scoop)
git revert 288aee9 # then resolve conflicts

System details

Windows version: 11

OS architecture: 64bit

PowerShell version: pwsh core 7.2.6 and windows powershell 5.1

Additional software: [(optional) e.g. ConEmu, Git]

Scoop Configuration

{
    "debug":  true,
    "last_update":  "2022-10-29T03:36:12.4060230+00:00",
    "scoop_branch":  "develop"
}

[HUMORCE]

caused by #5065(288aee9)

Scoop/lib/core.ps1

Lines 531 to 535 in 8aee6f9

	} elseif ($Process.StartInfo.ArgumentList.Add) {
	# ArgumentList is supported in PowerShell 6.1 and later (built on .NET Core 2.1+)
	# ref-1: https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.processstartinfo.argumentlist?view=net-6.0
	# ref-2: https://docs.microsoft.com/en-us/powershell/scripting/whats-new/differences-from-windows-powershell?view=powershell-7.2#net-framework-vs-net-core
	$ArgumentList | ForEach-Object { $Process.StartInfo.ArgumentList.Add($_) }

ArgumentList is supported in PowerShell 6.1 and later (built on .NET Core 2.1+)

Windows Powershell 5.1 was not considered in it, related: ScoopInstaller/Extras#9583

Scoop uses rolling updates and has a large number of buckets (offical, 3rd-party, private), not sure this feature affects how many manifests, if changes is not compatible with the existing manifest, what is the point of the change?

[niheaven]

In WindowsPowerShell it should be fallback to

Scoop/lib/core.ps1

Lines 536 to 547 in 8aee6f9

	} else {
	# escape arguments manually in lower versions, refer to https://docs.microsoft.com/en-us/previous-versions/17w5ykft(v=vs.85)
	$escapedArgs = $ArgumentList | ForEach-Object {
	# escape N consecutive backslash(es), which are followed by a double quote, to 2N consecutive ones
	$s = $_ -replace '(\\+)"', '$1$1"'
	# escape N consecutive backslash(es), which are at the end of the string, to 2N consecutive ones
	$s = $s -replace '(\\+)$', '$1$1'
	# escape double quotes
	$s = $s -replace '"', '\"'
	# quote the argument
	"`"$s`""
	}

So does the if ($Process.StartInfo.ArgumentList.Add) work incorrectly?

If the two argument lists act differently, then it may be a bug of L538.

[HUMORCE]

no further review yet. but it is incompatible with the existing manifest in fact.

edit:

the arguments are differ between 7.1 and 5.1

[niheaven]

Yes, it is the quote here cause these bugs... Will try to resolve it.

Simply change ""$s"" to "$s" fixes the bug, but I'm not sure if this could introduce other bugs.

Scoop/lib/core.ps1

Line 546 in 8aee6f9

"`"$s`""

@lewis-yeung

[HUMORCE]

if you replace L546 with $s or "$s" will fix installation of extras/7tt, but extras/ida-free(before ScoopInstaller/Extras@29b27cd) still fail.

[niheaven]

ida-free should work if $escapedArgs = $ArgumentList | ForEach-Object { is replaced with $escapedArgs = $ArgumentList -split ' ' | ForEach-Object { I thought, but haven't tested.

[lewis-yeung]

Simply change "`"$s`"" to "$s" fixes the bug, but I'm not sure if this could introduce other bugs.

If you do this, a single argument containing any space characters will be incorrectly split.

[niheaven]

I've seen many manifests drop installer.args and use Invoke-ExternalCommand or Start-Process directly, and the arguments handler may be rewritten or redesigned.

[HUMORCE]

I've seen many manifests drop installer.args and use Invoke-ExternalCommand or Start-Process directly

...the car is reversing, because it was impassable ahead.

[HUMORCE]

how about formatting the args before calling Invoke-ExternalCommand? brain loss 😂

[chawyehsu]

chawyehsu/dorado#684

So basically #5065 & #5066 not only breaks #5066 (comment) but also all manifests using installer.args? Then sadly this refactoring is a big regression I believe. Look at those reference commits by @issaclin32 in #5065...

[rashil2000]

That's what I was saying all along too - it was a massive breaking change...

[HUMORCE]

The similar bug is happening with Invoke-ExternalCommand. ScoopInstaller/Extras#10322