Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make POSIX-compliant to increase portability and remove bash depend… #100145

Merged
merged 4 commits into from Jun 23, 2020
Merged

Make POSIX-compliant to increase portability and remove bash depend… #100145

merged 4 commits into from Jun 23, 2020

Conversation

nothingnesses
Copy link
Contributor

…ency on Linux. Fix causes of shellcheck warnings.

This PR fixes #100144 by removing bashisms and replacing them with POSIX-compliant alternatives.

…ency on Linux. Fix causes of `shellcheck` warnings.
@ghost
Copy link

ghost commented Jun 14, 2020

CLA assistant check
All CLA requirements met.

Copy link
Contributor Author

@nothingnesses nothingnesses left a comment

Choose a reason for hiding this comment

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

Should the VS Code on lines 8 and 33 be replaced with @@PRODNAME@@?

resources/linux/bin/code.sh Outdated Show resolved Hide resolved
resources/linux/bin/code.sh Outdated Show resolved Hide resolved
@joaomoreno
Copy link
Member

Does this fix an actual issue?

@nothingnesses
Copy link
Contributor Author

nothingnesses commented Jun 16, 2020

I'm unable to start VS Code on my system which doesn't have bash, and this fixes that.
For what it's worth, this would still allow VS Code to work on systems with bash. It simply also allows it to work on systems that don't have bash but do have other POSIX-compliant shells.

@joaomoreno
Copy link
Member

joaomoreno commented Jun 17, 2020

Can you give me an example of said system? I'm just looking for a specific practical example.

@nothingnesses
Copy link
Contributor Author

Sure thing. There are certain Linux distributions that don't enforce the usage of, and don't come pre-packaged with bash. For example, there's the minimal version of the Void Linux distribution which opts for dash and Kiss Linux which opts for busybox instead. On these systems, bash would arguably be considered redundant and could (in the case of Kiss where all packages must be compiled by the user) cause other software that depend on it an unnecessarily longer amount of time to build (when considering that bash also has to be built even though all it might be needed for is to run a start up script for VS Code). As such, reducing dependencies would be beneficial for the end user.

@joaomoreno
Copy link
Member

I'm actually surprised VS Code runs in said systems. 👌

joaomoreno and others added 2 commits June 21, 2020 17:11
Co-authored-by: VoidNoire <18732253+VoidNoire@users.noreply.github.com>
Co-authored-by: VoidNoire <18732253+VoidNoire@users.noreply.github.com>
@joaomoreno
Copy link
Member

@aeschli @bpasero Please review whether your areas of the script still work as designed.

resources/linux/bin/code.sh Outdated Show resolved Hide resolved
@joaomoreno joaomoreno added the workbench-os-integration Native OS integration issues label Jun 21, 2020
@joaomoreno joaomoreno added this to the June 2020 milestone Jun 22, 2020
@joaomoreno joaomoreno merged commit de12505 into microsoft:master Jun 23, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
workbench-os-integration Native OS integration issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make resources/linux/bin/code.sh POSIX-compliant to remove bash dependency
4 participants