Skip to content

refactor: add bash shebang for shellcheck #2286

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

Merged
merged 1 commit into from
Sep 25, 2024
Merged

refactor: add bash shebang for shellcheck #2286

merged 1 commit into from
Sep 25, 2024

Conversation

mjgallag
Copy link
Contributor

https://www.shellcheck.net/wiki/SC2148 (tips depend on target shell)
https://direnv.net/#how-it-works (.envrc is loaded into a bash sub-shell)

https://www.shellcheck.net/wiki/SC2148 (tips depend on target shell)
https://direnv.net/#how-it-works (.envrc is loaded into a bash sub-shell)

Signed-off-by: Michael Gallagher <[email protected]>
@mikeland73
Copy link
Contributor

@mjgallag this LGTM!

One caveat is that this file is not replaced automatically (because it would require re-allow from direnv).

Folks that already have .envrc would need to do: devbox gen direnv --force and direnv allow

(We've talked about versioning this file in the past, but that's outside the scope of this change)

@gcurtis
Copy link
Collaborator

gcurtis commented Sep 24, 2024

Not as familiar with the envrc stuff so this might not matter, but is the file generated from this template ever executed directly (e.g., ./envrc)? Although it's unlikely, that would fail if there's no bash in /bin/bash. Another option is to add a shellcheck directive with # shellcheck shell=bash.

@mjgallag
Copy link
Contributor Author

@gcurtis direnv uses the bash path that is set at build time:
https://github.com/direnv/direnv/blob/ae5a61848a06da4251b91fd084bffa9a5732fb89/internal/cmd/rc.go#L271
https://github.com/NixOS/nixpkgs/blob/aaa7fb5840f278da8010fe7466c3d952f7e536da/pkgs/by-name/di/direnv/package.nix#L17-L19
https://github.com/Homebrew/homebrew-core/blob/fb3c5a4fcbd4d4974499ed9b46304d8d0881cbc3/Formula/d/direnv.rb#L24

I could envision maybe running . ./envrc or ./envrc directly as a convenience when testing changes to it, but the shebang shouldn't have any effect as far as direnv is concerned. That being said I don't have a strong preference here between #!/bin/bash, #!/usr/bin/env bash or # shellcheck shell=bash.

@savil savil merged commit 3818683 into jetify-com:main Sep 25, 2024
23 of 24 checks passed
@mjgallag mjgallag deleted the refactor-add-bash-shebang-for-shellcheck branch September 25, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants