Skip to content

Update serverSetup.ts | Add shebang to Bash install script for compatibility#184

Open
vadrif-draco wants to merge 4 commits into
jeanp413:masterfrom
vadrif-draco:patch-1
Open

Update serverSetup.ts | Add shebang to Bash install script for compatibility#184
vadrif-draco wants to merge 4 commits into
jeanp413:masterfrom
vadrif-draco:patch-1

Conversation

@vadrif-draco

Copy link
Copy Markdown

This should extend support for linux servers whose default shell is not compatible with the embedded shell script

This should extend support for linux servers whose default shell is not compatible with the embedded shell script
Comment thread src/serverSetup.ts Outdated
Co-authored-by: Adam C. Stephens <2071575+adamcstephens@users.noreply.github.com>
@vadrif-draco vadrif-draco changed the title Update serverSetup.ts | Add #!/bin/sh to script Update serverSetup.ts | Add shebang to Bash install script for compatibility Dec 19, 2024
@vadrif-draco

Copy link
Copy Markdown
Author

Any updates on this? I believe it's a very simple fix for a relatively important issue for anyone whose remote machine logon shell isn't compatible with the script...

@GitMensch GitMensch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - but.. is the "env" binary guaranteed to be available in this place everywhere?

@pomponne

Copy link
Copy Markdown

Simple fix for #182 ?

@pomponne

Copy link
Copy Markdown

Just installed a packaged version of the current patch-1 branch in codium, and I still have the same error when reading the install script on the remote machine...

@daiyam

daiyam commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator

@vadrif-draco Does the shebang fix the issue? I wonder because the script is executed with the command bash -c <script> so bash should execute it. Do I miss something?
Is bash symlinked to another shell on some systems?

@lex-ibm

lex-ibm commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

I've seen issues with the install script where users use shells like tcsh/csh. So adding my +1

@daiyam

daiyam commented Apr 3, 2026

Copy link
Copy Markdown
Collaborator

I've seen issues with the install script where users use shells like tcsh/csh. So adding my +1

I see no issue to add the shebang but I just wonder if that will fix the issue...

@vadrif-draco Can you change from sh to bash? I will merge it when done.

@GitMensch

GitMensch commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

As noted I'm not sure if the env binary is always there... and actually Termux (which comes with env) for example has no /usr in the first place.
The shebang should not fix anything, other than possibly syntax highlighting when the script is opened...

I think the fix is to change

commandOutput = await conn.exec(`bash -c '${installServerScript.replace(/'/g, `'\\''`)}'`);

to use sh -c - because all systems not having a bash binary will fail otherwise... of course that only works if the generated script is posix-compatible - is it?
And if it isn't then it would be useful to use no features that aren't available in the supported shells (the ones that have a shellintegration https://github.com/microsoft/vscode/tree/main/src/vs/workbench/contrib/terminal/common/scripts ).

@daiyam

daiyam commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

to use sh -c - because all systems not having a bash binary will fail otherwise... of course that only works if the generated script is posix-compatible - is it?

shellcheck --shell=sh install.sh does fail. [[]] and (()) aren't POSIX...

@GitMensch

Copy link
Copy Markdown
Contributor

I kind of feel it would be more useful to work on POSIX compatibility and replace bash by sh... (but have no clue when I find time working on that).

@daiyam

daiyam commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

This is why I find that #!/usr/bin/env sh to be puzzling since it doesn't change the fact that the script isn't POSIX or that bash is called to execute it...

I will try to exfiltrate the install scripts outside the code so they can be linted more easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants