Skip to content

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