fix(core): Fix "Invoke-ExternalCommand" quoting rules

[niheaven]

Description

Quoting rules...

There's a bug in PS5 if extract_dir has spaces when using 7z to extract the archive, and this PR tries to fix it.

Motivation and Context

• Closes https://github.com/ScoopInstaller/Extras/issues/13174
• Closes https://github.com/ScoopInstaller/Extras/issues/13221
• Closes https://github.com/ScoopInstaller/Extras/issues/13227
• Closes https://github.com/ScoopInstaller/Extras/issues/13255

There's a known bug that NSIS's /D param doesn't work if the path has spaces in PS7, if we provide /D=xxx xxx in ArgumentList, the NSIS installer doesn't recognize it, don't know why.

How Has This Been Tested?

Local install affected apps in PS5 and add some test cases.

Checklist:

• [x] I have read the Contributing Guide.
• [x] I have ensured that I am targeting the develop branch.
• [ ] I have updated the documentation accordingly.
• [x] I have updated the tests accordingly.
• [x] I have added an entry in the CHANGELOG.

[chawyehsu]

Could you please add unit tests for this type of situation to the Decompress testsuite?

---

The NSIS Document says that there are two constrains of the /D switch.

• NO quotes (even if the path contains spaces)
• It MUST be the last parameter used in the command line

There's a known bug that NSIS's /D param doesn't work if the path has spaces in PS7, if we provide /D=xxx xxx in ArgumentList, the NSIS installer doesn't recognize it, don't know why.

Probably the second constrain is the cause in this case?

[niheaven]

Alright, I will implement some test cases and refactor the decompression test structure accordingly.

NSIS in PS7:

[niheaven]

Test cases added, all done.

[chawyehsu]

I think I found out the cause of the /D switch getting ignored in PS7 @niheaven

$Process = New-Object System.Diagnostics.Process
$Process.StartInfo.FileName = '...exe'
$Process.StartInfo.ArgumentList.Add('/S')
$Process.StartInfo.ArgumentList.Add('/D=C:\temp\miniconda space space')

If an arguement that is added to ProcessStartInfo.ArgumentList contains whitespaces, it will be wrapped with quotes before being passed to the native command, this is an expected behavoir for most cases but not for the NSIS installer.

Probably we have to use the legacy argument escaping necessarily for NSIS installers, whether in PS5 or PS7, i.e. there needs to be one more condition of testing NSIS installers in https://github.com/ScoopInstaller/Scoop/blob/36026f1804090b1726fe8a7df699ca739fdb9cbc/lib/core.ps1#L782

[chawyehsu]

Regarding to the TestCases, perhaps there should also be test nsis installers to validate such a situation.

[niheaven]

Probably we have to use the legacy argument escaping necessarily for NSIS installers

Indeed, that was my initial thought, but I was unable to find an efficient method to verify it. PEiD or Exeinfo should be able to accomplish the task, yet I was unable to discern their methodology. (Using a hex editor, I could find Nullsoft in PE, but couldn't with PS)

[chawyehsu]

Reading hex to identify NSIS installers requires extra IO. What about picking the inactive #3502 up? That may take much work though, and a workaround (by just checking if /D=<path> exists?) should be released to mitigate existing issues.

[niheaven]

I am currently addressing issue #3502 by dividing it into multiple individual PRs, with some already merged, the most recent being #5951. I will also review any existing manifest that utilizes /D=, as I recall that other installers may use this as well.

[niheaven]

Check NSIS installer by its arguments (should have /S and /D=xxx), and add test cases.

[chawyehsu]

Check NSIS installer by its arguments (should have /S and /D=xxx)

That's the workaround I was thinking of.

[RaphaelTarita]

also closes https://github.com/ScoopInstaller/Extras/issues/13160. When is this fix expected to be released?

[niheaven]

In a few days.

[chawyehsu]

I would like to cherry-pick this and publish a patch release instead of waiting for days. And I'm seeing the sqlite cache feature needs some more tweaks to be available for master channel. WDYT? @niheaven

[niheaven]

Although I believe the SQLite cache feature is stable enough for the end user, cherry-picking some fixes to the main branch is a decision that requires careful consideration. I will proceed with it now and create a pull request.