Closed davidpfarrell closed 3 years ago
From what I've read, seems like using #!/usr/bin/env bash
is the most flexible & system agnostic. And since bash-it is used in various systems and environments, maybe the best thing would be to have env
find it for us. I'll go ahead and update my PR - thanks for this!!
Thanks for reviewing this - it would be great to get this in a consistent style - please go ahead with a PR.
Having said that, for the Bash-it plugins/completions/aliases/themes, it's less relevant, since they're all loaded using the source
command anyway, which means that the shebang line in the individual files is pretty much ignored anyway... Still nice if we can get a uniform way of doing this.
There's a lot more cleanup work that could be done, I have a list of areas that should be looked at. Maybe it's time to start tackling that.
If the shebang line isn't necessary, my vote would be to remove it from all of them. Why maintain it if it isn't needed? Perhaps replace it with a comment along the lines of:
# loaded by bash-it via source
Good point. Since it does not seem to be in all files, we probably should remove it from the other ones. The ones where we need it are bash_it.sh
, install.sh
and uninstall.sh
.
The ones where we need it are
bash_it.sh
,install.sh
anduninstall.sh
.
@nwinkler Are you saying that, throughout the entire repository, these are the only files that actually need a #!
? i.e. every .bash
file is sourced as needed?
Assuming Yes:
That's great ! With that I lean even more toward removing leading #!
lines from all .bash
files.
Additionally, if we have any CI checks in place, we could add a check to enforce this rule.
Moreover, we could also add a check to enforce that .bash
files do not have their executable flags set.
@davidpfarrell Yes, that should be right. If you take a look at the bash_it.sh
file, it source
s all of the other files.
I'm not entirely sure about the bats
tests, they use the bats
load
command, but I assume that that uses source
as well for loading files. The test/run
script needs to be executable and requires a shebang, in addition to the ones listed above.
I don't think that we have any CI checks in place.
Adding a comment instead of a shebang sounds like a pretty good idea.
You might even use the shellcheck ignore directive instead...
# shellcheck shell=bash disable=SC2148
I think the best approach is to keep the shebang and rename all to /usr/bin/env bash
, this is more formalized and indicative for new users IMO
@NoahGorny I would agree if it weren't for our filename scheme that includes .bash
. So, here is my proposal for a tie breaker, plus it would get something done that I've wanted for a while: base the decision on impact to load time - Let's benchmark it!
We can use a minimal alpine + bash container and setup a minimal environment where we could do something simple, like start 100 shells and calculate an average load time. It would be best to structure the test in a way that we could run it multiple times, allowing for the enabling of plugins and any associated software to locally benchmark a new plugin.
@NoahGorny I would agree if it weren't for our filename scheme that includes
.bash
. So, here is my proposal for a tie breaker, plus it would get something done that I've wanted for a while: base the decision on impact to load time - Let's benchmark it!We can use a minimal alpine + bash container and setup a minimal environment where we could do something simple, like start 100 shells and calculate an average load time. It would be best to structure the test in a way that we could run it multiple times, allowing for the enabling of plugins and any associated software to locally benchmark a new plugin.
Benchmarking is a good idea, but how it is connected to the shebang?
@NoahGorny The more I dig into bash's internal mechanics, the more I learn that there are very small ways that increase computation cost, and they add up. Admittedly, the number of files we end up loading is extremely small compared the this benchmark, but it will illustrate what I was considering in my reply.
#!/usr/bin/env bash
cat > shebang.bash <<-EOF
#!/usr/bin/env bash
:
EOF
cat > nobang.bash <<-EOF
:
EOF
trap 'rm -f shebang.bash nobang.bash' EXIT
time for i in {1..100000}; do source shebang.bash; done
time for i in {1..100000}; do source nobang.bash; done
$ ./benchmark.sh
real 0m2.083s
user 0m1.259s
sys 0m0.811s
real 0m1.953s
user 0m1.158s
sys 0m0.776s
Hi Team!
Looks a benchmarking discussion broke out in my bash-headers discussion :)
You might want to start a new issue/discussion and move the comments there.
On a side note, though, seeing the notifications here reminded me to follow-up on this bash-header discussion here.
I have created PR #1765 which I hope addresses the concerns discussed here.
Cheers!
cornfeedhobo wrote:
Sorry, I don't see benchmarking as unrelated, nor do I see a consensus about how to move forward, but I guess we can continue the discussion in your pr...
@cornfeedhobo Sorry I think I may of misunderstood your benchmarking.
Were you saying that the presence of the #!
line in a sourced file has a non-0 impact on its load/processing time?
If so, then I think that makes a strong case to remove #! headers from .bash files.
@davidpfarrell Yeah, that is what I was trying to convey, but I think your use of echo run from bash: .
has greater value.
While looking at current PR #1490 I saw the the author switched the completion script:
This seemed like an oversight so I called it out on the PR, but it got me wondering how the rest of the repo is doing in this regard :
grep
results (formatted)
@nwinkler interested in your thoughts here?
If we consider these wrong, I'm happy to put together a PR to set them all to
#!/usr/bin/env bash
-DF