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

PSA: bash != sh ! #408

Closed
Kreyren opened this issue Feb 14, 2020 · 10 comments · Fixed by nuttyartist/notes#421
Closed

PSA: bash != sh ! #408

Kreyren opened this issue Feb 14, 2020 · 10 comments · Fixed by nuttyartist/notes#421
Assignees
Labels
question Further information is requested

Comments

@Kreyren
Copy link

Kreyren commented Feb 14, 2020

image

Why is this being used instead of proper shebang based on provided extension?

@al-cheb
Copy link
Contributor

al-cheb commented Feb 15, 2020

Hello, @Kreyren

Very good explanation - https://stackoverflow.com/questions/27813563/what-is-the-bash-file-extension

man bash
Bash is an sh-compatible command language interpreter that executes commands read from the standard input or from a file. Bash also incorporates useful features from the Korn and C shells (ksh and csh).

Google Shell Style Guide - https://google.github.io/styleguide/shell.xml

Bash is the only shell scripting language permitted for executables.
Executables must start with #!/bin/bash and a minimum number of flags. Use set to set shell options so that calling your script as bash <script_name> does not break its functionality.

Executables should have no extension (strongly preferred) or a .sh extension. Libraries must have a .sh extension and should not be executable.
It is not necessary to know what language a program is written in when executing it and shell doesn't require an extension so we prefer not to use one for executables.

However, for libraries it's important to know what language it is and sometimes there's a need to have similar libraries in different languages. This allows library files with identical purposes but different languages to be identically named except for the language-specific suffix.

@Kreyren
Copy link
Author

Kreyren commented Feb 15, 2020

Very good explanation - https://stackoverflow.com/questions/27813563/what-is-the-bash-file-extension

Based on my experience i believe this being false since bash recognizes .bash extension

Bash is an sh-compatible command language interpreter that executes commands read from the standard input or from a file. Bash also incorporates useful features from the Korn and C shells (ksh and csh).

Correct, but that does not make it POSIX compatible on system that doesn't have shebang module compiled in nor system that does not support dynamic linking..

Also POSIX shell is generally faster then bash.

Executables should have no extension (strongly preferred) or a .sh extension. Libraries must have a .sh extension and should not be executable.

for executable permission it depends on the script usecase

@al-cheb
Copy link
Contributor

al-cheb commented Feb 17, 2020

cc @alepauly any thoughts?

@thejoebourneidentity thejoebourneidentity added question Further information is requested and removed needs triage labels Feb 18, 2020
@ethomson
Copy link

I'm not sure that I understand the question fully. Are you saying that the first line should be #!/bin/sh? Or that the extension should be .bash?

@Kreyren
Copy link
Author

Kreyren commented Feb 20, 2020

@ethomson if a minor optimization is worth the effort then #!/bin/sh should be considered otherwise if #!/bin/bash is used then extension .bash should be used in case target system does not support shebangs.

@ethomson
Copy link

What I don't understand is: what's the problem here that you're trying to solve with this?

We want to use bash because we want to be able to use bash-isms. Despite specifying bash, .sh is the recommended extension, based on several style guides. We control the target systems - they will always support #! syntax.

@Kreyren
Copy link
Author

Kreyren commented Feb 20, 2020

What I don't understand is: what's the problem here that you're trying to solve with this?

I believe this being code quality issue that may cause the mentioned issues in the future where I am considering refactoring the project since there are more issues assuming that GitHub Actions are used on my projects to know how upstream handles the code-quality to know if it's worth the time and welcomed.

@ethomson
Copy link

Yes, you're very much right that code quality is very important. I agree with you 100%. We take code quality seriously, and would welcome pull requests that address code quality issues.

But "quality" is often subjective. Using bash is our standard, and using .sh extensions are our standard. We're following standards that are widely believe to be best practices, and without compelling reasons to change them, we should not.

Do recall that these scripts exist only to create the environments that our runners support, and that they need only to be executed in our sandboxed environment, only to create that environment, and are then deleted. They will never operate in any environment where bash doesn't exist, since we have total control over that environment.

This is independent from using GitHub Actions, even on the environments where these scripts were created.

So this particular issue is stylistic and - while I understand that style is subjective - this is the one that we've chosen and want to remain consistent with.

If you have other code quality issues - eg, sloppy quoting, using things like set -e - those are a lot less stylistic and would be welcome indeed.

@Kreyren
Copy link
Author

Kreyren commented Feb 20, 2020

Yes, you're very much right that code quality is very important. I agree with you 100%. We take code quality seriously, and would welcome pull requests that address code quality issues.

Submitted, context and method of testing required for proper implementation

But "quality" is often subjective. Using bash is our standard, and using .sh extensions are our standard.

Then it is a terrible standard, notice the changes in #438 where we can do all the functions using POSIX shell, also standard without any linting you can't be serious to say that it is a best practice (ConaLisas added in #438).

So this particular issue is stylistic and - while I understand that style is subjective - this is the one that we've chosen and want to remain consistent with.

It is a concern of sanitization and optimization not only stylistic.

@ethomson
Copy link

Then it is a terrible standard

I'm sorry that you don't like it but again, bash will always be installed on our environments, so there's no reason not to use bashisms for convenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants