open-remote-ssh
open-remote-ssh copied to clipboard
support vscodium alpine reh
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
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.
Finally instead of trying
ashin case bash fails it would make sense to replace bash withshif 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?
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.
@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.
Even with that current dependency $SHELL is better, isn't it?
The script fails with ash so I would keep bash.
$SHELL might fail with system with fishshell
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
LGTM
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.
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