simplify required version checks and bring back support for bash 3

[kingarrrt]

The =~ operator is bash 3, the only bash 4 feature used was named references introduced in 6997988. This changes that to use variable inheritance.

Tested on NixOS and macOS with direnv via both homebrew and nix.

Refs #423.

[bbenne10]

sigh

I am happy to merge this as it is simpler and easier to grok, but I don't want to promise continued support for macOS's ancient bash version moving forward. Honestly, I think it's an implementation detail that macOS even ships bash at this point and I wouldn't be surprised to see it go away in a (relatively soon to come) future release.

I am hesitant to encode a bash revision in the software minimum version check, as I can't promise that we're going to check everything we write against that version. I think we'll have to figure out a way to ensure this runs on bash 3 during test runs if we decide to extend support down that far.

[kingarrrt]

The existing code (excepting 6997988) is de facto bash 3 compatible so you're not doing anything specific to support macOS bash right now.

I put in the minimum version check so that a too-old bash is caught early before it raises a syntax error. Per #423 the end user experience is of course smoother if a homebrew-installed direnv with system bash works out of the box, but maybe its best to just drop the second commit and leave BASH_MIN_VERSION at 4? At least there is now a clear error message.

For reference, this lists bash features and versions: https://mywiki.wooledge.org/BashFAQ/061.

[kingarrrt]

Actually if the second commit is dropped BASH_MIN_VERSION could be 5.2 which is what's current in both nixpkgs and homebrew.

[bbenne10]

I've given this some thought and I think that "whatever version of bash is in nixpkgs stable" is the version we support. This seems the only reasonable way to ensure that everyone has the same version and reduces the burden on us to only check "does this work in nixpkgs stable".

I know that's not great news for macOS users, but the situation isn't any worse than the one we have implicitly now and the burden of checking compatibility is greatly reduced. Does that sound reasonable? @Mic92: Do you have thoughts?

[Mic92]

I've given this some thought and I think that "whatever version of bash is in nixpkgs stable" is the version we support. This seems the only reasonable way to ensure that everyone has the same version and reduces the burden on us to only check "does this work in nixpkgs stable".

I know that's not great news for macOS users, but the situation isn't any worse than the one we have implicitly now and the burden of checking compatibility is greatly reduced. Does that sound reasonable? @Mic92: Do you have thoughts?

Mhm. I think having a version check that gives proper errors is also nice, because people can just slap in the source_url section and than it's being automatically used by every user of the project. And it's hard to control how people are going to setup their system. Direnv is often the gateway drug to introduce nix in teams and you don't want to cause there more friction than required. In the end those people have to than install direnv with nix again but at least they now know they have to do it.

[bbenne10]

Mhm. I think having a version check that gives proper errors is also nice, because people can just slap in the source_url section and than it's being automatically used by every user of the project. And it's hard to control how people are going to setup their system. Direnv is often the gateway drug to introduce nix in teams and you don't want to cause there more friction than required.

It is a hard call for me, honestly. If we decide to explicitly say we support any given version, we MUST test against that version, I believe. We are making a commitment to that and should ensure we are holding ourselves to our promise and I am frankly not entirely sure how to do that. I COMPLETELY see that direnv is an entry point to the larger nix ecosystem and I want us to be a good ambassador. This seems extreme, but... Is there a possibility to add bash to the dependency list of nix's direnv derivation and homebrew's direnv package so that it picks a modern bash (which seems useful more generally for direnv) or something similar? It seems useful to know what runtime you can be expected to execute on with some certainty...

[kingarrrt]

Pushed again with BASH_MIN_VERSION=5.2.

[kingarrrt]

Is there a possibility to add bash to the dependency list of nix's direnv derivation and homebrew's direnv package so that it picks a modern bash (which seems useful more generally for direnv) or something similar? It seems useful to know what runtime you can be expected to execute on with some certainty...

Nixpkgs already has the bash dependency: https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/misc/direnv/default.nix#L23

Homebrew needs it: https://github.com/Homebrew/homebrew-core/blob/master/Formula/d/direnv.rb

I'll try and fiddle a homebrew pull request later on.

[Mic92]

Mhm. I think having a version check that gives proper errors is also nice, because people can just slap in the source_url section and than it's being automatically used by every user of the project. And it's hard to control how people are going to setup their system. Direnv is often the gateway drug to introduce nix in teams and you don't want to cause there more friction than required. It is a hard call for me, honestly. If we decide to explicitly say we support any given version, we MUST test against that version, I believe. We are making a commitment to that and should ensure we are holding ourselves to our promise and I am frankly not entirely sure how to do that. I COMPLETELY see that direnv is an entry point to the larger nix ecosystem and I want us to be a good ambassador. This seems extreme, but... Is there a possibility to add bash to the dependency list of nix's direnv derivation and homebrew's direnv package so that it picks a modern bash (which seems useful more generally for direnv) or something similar? It seems useful to know what runtime you can be expected to execute on with some certainty...

I think we can balance that based on how much ugly the required code looks like. Just now it looks like the more portable code is even shorter than what we currently have. We can revisit the decision later again if things are changing.

[Mic92]

@mergify queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at e8acd40

[kingarrrt]

homebrew bash dependency: Homebrew/homebrew-core#156230

[bbenne10]

I think we can balance that based on how much ugly the required code looks like. Just now it looks like the more portable code is even shorter than what we currently have. We can revisit the decision later again if things are changing.

Completely agree that this is better and significantly simpler than our old approach. I do not think we should've avoided merging the major parts of this changeset, but rather I worry about promising backwards compatibility as far as we are with the merge of this PR. We are now saying with some certainty that we know we execute on bash 3.2, which I know I cannot keep up with without additional burden - either by researching on what version specific features were introduced into or explicit testing on 3.2, which isn't available in nixpkgs and so means I can only really do with changes to my profile on my macOS machine (a machine I would prefer to leave behind, to be perfectly honest). The burden of work on introducing new non-trivial changes is thus already increased for myself and for additional contributors.

We can, for instance, introduce an explicit test against bash 3.2, but I am afraid (as with the sort compatibility question earlier in this PR) that we won't know we've introduced other breaking changes with that too.

Again: I'm trying to find a balance between end-user utility and the overall burden on developers and contributors, not trying to stand in the way of good forward progress. I appreciate you all listening to my concerns. Thank you!

[Mic92]

@kingarrrt if you got the time & motivation: having a test in our matrix that override uses bash 3.2 would be nice.

[kingarrrt]

Yes. I'll see what I can do. The test is only going to verify that it fails the version check though, so nothing much.

@bbenne10 Note that 31984f6 makes no backwards compatibility promises.

[Mic92]

I think the version check is a good start, that's all the support we promise for this version of bash.

[bbenne10]

@bbenne10 Note that 31984f6 makes no backwards compatibility promises.

Whoops. Somehow I missed this entirely. Sorry for the noise, but I'm happy to have an opportunity to spell out my thoughts on backwards compatibility and supported underlying software revisions.