How to declare multiple args for a shim in a manifest?

[stkb]

I was working on a patch for fixing bugs relating to shims, that I've discovered after getting involved with #1951. But I can't really go further until I know the answer to this. The wiki isn't very clear: it just gives the example of [ "program.exe", "alias", "--arguments" ].

There are currently 3 ways of adding multiple arguments to a shim that are kinda supported by the code. (Depending on the shim type/target, they may work or not)

1. [ "shim_target", "shim_name", "arg1 arg2" ]
2. [ "shim_target", "shim_name", "arg1", "arg2" ]
3. [ "shim_target", "shim_name", ["arg1", "arg2"] ]

That (3) works is fairly incidental, and it's not supported by the json schema.

The question is are both (1) and (2) supported? And if (1) is supported, is it here two args or a single arg with a space?

[r15ch13]

It only supports (1)
https://github.com/lukesampson/scoop/blob/e80308022c82ed9cd4238dfb428d08aa909479b8/lib/install.ps1#L670-L691

It copies the third element to $arg and passes it to shim()

[stkb]

No in that method, on line 679, $arg becomes either:

• a string, if only 1 element is remaining, or
• an array, if 2 or more elements are remaining.

However that array is (inadvertently?) converted to a string when passed to the substitute function, which returns a string, which is passed to shim().

The reason I wan't sure is because the code that creates the .ps1 shim...
https://github.com/lukesampson/scoop/blob/e80308022c82ed9cd4238dfb428d08aa909479b8/lib/core.ps1#L220-L222
...treats $arg as is if it could be an array of args, trying to -join them separated with ', '. This means, for .ps1 shims, multiple args don't currently work (they get joined into 1 arg).

Anyway if only (1) is supported I can continue with that knowledge.

[r15ch13]

Oh, the substitute call was added with #1986
So I pretty much removed the array feature by merging this... 😞

[stkb]

No worries, I can add some tests too (once I figure out how that works 😃). So should the array feature be supported? I don't know if any manifests use it (haven't found any that use multiple arg shims; why the bug hasn't really been a problem)

[r15ch13]

There is not a single manifest that uses the array declaration, only 2 or 3 that use a string. So maybe we can scrap this feature.

[excitoon]

I apologize, I haven't thought arg can be an array. I'll prepare a fix for that soon.

[stkb]

@excitoon Hold on, not sure if it was yet decided which way to go on this.

However as far as I can tell, there are only two manifests in the main bucket (and none in any of the other known buckets) that use multiple args for a shim, and for each it's just one shim:

• busybox.json uses two strings: [ "busybox.exe", "bash", "bash", "-l" ]
• cygwin.json puts both args in one string [ "root\\bin\\bash.exe", "cygwin", "--login -i" ]

It perhaps seems silly to be spending too much time on something that impacts <1% of the manifests, but I think it needs to be one way or the other. (It also means though it's easy to fix the (one) that doesn't comply :) )

If it's the multiple strings way, I would fix the documentation to make it more clear.

[excitoon]

Well, actually it was me who broke that functionality by #1986. Personally, I find it is very clear to have arguments in the array like in (3), but I previously had no idea that this is possible.
I've finished with the fix, I hope @r15ch13 will take a look at it soon.

[r15ch13]

Now it's in. The change to the substitute() function may be handy for other use cases.

[stkb]

FYI multiple arguments in a single string (used by cygwin) are still broken for .ps1 shims.

Also there's now an inconsistency between shim definitions (args declared in separate strings) and shortcut definitions (all args in one string).

[excitoon]

I'll check this out.

[excitoon]

#1986 is irrelevant to this problem. Though I will fix that anyway.

[excitoon]

@stkb @r15ch13
Currently all three shim mechanisms for ps1 behave differently:

• sh shim works correctly for (1), but incorrectly for (2);
• ps1 shim joins arguments into one, it is correct for (2), but incorrect for (1);
• cmd shim skips arguments (bad for all notations).

Also, using arguments in single string (1) is technically mutually exclusive with (2) because there is no way to distinguish one argument from many. (Or everyone will have to escape arguments in all cases which is ugly.) Example:

['app', 'cmd', 'word1 word2'] # (two arguments in notation 1)
['app', 'cmd', 'word1 word2'] # (one argument in notation 2)

Long story short, we have to make a choice between (1,3) and (2,3). Personally, I like (2,3) because its more consistent and straightforward.

[excitoon]

Please someone reopen this.

[excitoon]

And final thing, exe shim works just like ps1 one, splitting (1) into different arguments.

[excitoon]

It's pretty strange that even though we have arrays in PowerShell we have to escape arguments by ourselves.

[excitoon]

(1,2,3) can also be possible, but in that case we require package maintainers to escape spaces manually:
[ "1 2", "2 \"2", "3\" 3" ] will become 'cmd 1 2 2 "2 3" 3'.
[ "long dir path" ] would have been written as [ "\"long dir path\"" ].
That will be truly chaotic.

[stkb]

@excitoon Yeah, I noticed the inconsistencies and bugs in all the creation shims, but I couldn't fix it without knowing what the spec was supposed to be (why I opened this thread).

As you say, we really cannot support (1) & (2) at the same time.
(3) is a bit of a red herring (while I know you like it 😄); it was incidentally supported by the code but that was never intended (it's not supported by the json schema).

In my opinion, only (1) should be supported, because:

1. [The most important] It's how the declarations for start menu shortcuts work, and this can't be changed (3rd string is all args, 4th string is icon). Having one rule for shims and another for shortcuts is confusing.
   • Given that shortcuts work in this way, I can only assume that this is also how shims were supposed to work too.
2. If all args are given in one string, we only need to paste this string in when writing the shim file (after doing variable substitution and possibly some sanitizing). IMO splitting the args up into an array just to recombine them again is unnecessarily complicated.
3. IMO, "--login -i" in a manifest is more readable than "--login", "-i" (again, I've always felt breaking args up is unnecessarily complicated).
4. While (1) "\"arg1 with spaces\" \"arg2 with spaces\"" is definitely uglier than (2) "arg1 with spaces", "arg2 with spaces", the number number of cases where you'd have this is very low (0 in all known buckets).

---

Hmm, thinking about it a little more, I guess as a compromise there's little reason why we couldn't also support (3) (for both shims and shortcuts) if you really do want to write the arguments out separately. Actually I'm warming to that idea.

[excitoon]

Well, let's keep it as it is - (1,2,3) with manual escaping. I've already fixed ps->cmd according to that in #2118. The only thing left is ps1->ps1 shim format (I wonder if anybody uses it).