open-remote-ssh icon indicating copy to clipboard operation
open-remote-ssh copied to clipboard

support vscodium alpine reh

Open GitMensch opened this issue 1 year ago • 11 comments

internally override PLATFORM for download URL for $OS_RELEASE_ID = alpine

@daiyam this tackles your report on https://github.com/VSCodium/vscodium/issues/1634#issuecomment-1966541125 - please comment on this.

Fair warning:

  • the second commit is "brute force", but 'should work'
  • the changes are not tested, as I currently don't run insiders which is needed for reh alpine

GitMensch avatar Feb 27 '24 16:02 GitMensch

I've checked and can run the alpine-reh just fine without using insiders.

But the fact persists that without your changes the wrong package is downloaded. It might make sense to detect musl instead of alpine and get a package supporting musl flavor. (e.g. like so: https://stackoverflow.com/questions/58177815/how-to-actually-detect-musl-libc)

Also the README.md would need updating because the extra packages are not necessary if using the alpine (musl) reh. (But it would be sensible to check if AllowTcpForwarding is enabled)

Finally instead of trying ash in case bash fails it would make sense to replace bash with sh if the command still works in a POSIX shell.

Other than that I appreciate your effort and would like to see the issue fixed.

HoboDev avatar Jul 22 '24 07:07 HoboDev

Finally instead of trying ash in case bash fails it would make sense to replace bash with sh if the command still works in a POSIX shell.

That's a good idea, but the script uses several extensions - come to think, I don't know if they all work in ash either. Looking at the generated script, there are some portability issues; ChatGPT-4 seems to be able to solve most of these. @jeanp413: would you like me to include those in this PR as a second commit? Should I already change bash | ash to either $SHELL or sh in that commit as well?

GitMensch avatar Jul 22 '24 09:07 GitMensch

In case of several (bash) extensions I'd recommend to keep them as is for now. It takes some effort and testing to convert the script properly.

Additionally there is bash for wsl in the script and bash for linux. It might be a trivial change to just touch the latter.

I'd rather have the correct reh build automatically and can live with installing bash. It also narrows down the scope of the PR to one problem at a time.

A script migration could be something for later if the repo owner agrees to it.

HoboDev avatar Jul 22 '24 10:07 HoboDev

@GitMensch I've used your PR (but I reversed the line commandOutput = await conn.exec(...);) and with the correct AllowTcpForwarding in SSH, the working correctly.

I will agree with @HoboDev, we should keep bash as a dependency.

daiyam avatar Jul 22 '24 13:07 daiyam

Even with that current dependency $SHELL is better, isn't it?

GitMensch avatar Jul 22 '24 14:07 GitMensch

The script fails with ash so I would keep bash.

daiyam avatar Jul 22 '24 14:07 daiyam

$SHELL might fail with system with fishshell

daiyam avatar Jul 22 '24 14:07 daiyam

OK, I've dropped ash for this PR, If @jeanp413 is fine with that and has merged it I'll create another PR for some "more portable syntax" changes, then we can see if we may add "any shell" ash/ksh/fish to the supported shells

GitMensch avatar Jul 22 '24 14:07 GitMensch

LGTM

daiyam avatar Jul 22 '24 15:07 daiyam

I'd recommend adding changes to the README.md because with the reh version for musl I do not need to install the mentioned packages which are for libc compatibility though it does not hurt to mention bash as a requirement.

HoboDev avatar Jul 23 '24 03:07 HoboDev

OK, I've dropped ash for this PR, If @jeanp413 is fine with that and has merged it I'll create another PR for some "more portable syntax" changes, then we can see if we may add "any shell" ash/ksh/fish to the supported shells

Looking at the code at first glance in generateBashInstallScript we'll only have to port the if conditions from bash to sh and alter the range in the for loop from {1..5} to 1 2 3 4 5

After that it should work with plain sh

HoboDev avatar Jul 23 '24 03:07 HoboDev