Move bash 3 version check to top of file for early exit

[wolfgang42]

Bash executes the script as it's reading it. When it encounters a function, it parses the function body before continuing. The bashly-generated script has all of its code in functions which appear before any code is executed (the initialize/run functions are only called at the bottom of the script). Normally this is useful, since it ensures that bash has loaded/parsed the whole script before beginning execution; however, in the case of this version check it means that if any of the functions contain syntax that Bash 3 doesn't understand, it will choke on that and crash (with a much less helpful error) before it has a chance to execute the version check:

/app/cli: line 117: conditional binary operator expected
/app/cli: line 117: syntax error near `missingno'
/app/cli: line 117: `  if [[ -v missingno ]]; then'

This PR adds a test for this situation and moves that check to the very top of the file, where it will be executed and trigger an early exit before bash tries to parse the rest of the script, so that it will work correctly regardless of the remainder of the contents.

(Extracted from #113 for clarity.)

[DannyBen]

I am still reluctant to add something (that exits, no less) outside of any function.
I am not yet convinced this is needed.

Could you provide the simplest bashly config + code for, say, root_command that will fails?

Also see #113 (comment) - perhaps providing user hooks in some places for the developer to inject code could solve all these problems without introducing strict and dangerous limitations.

Bash 3 is really something I have no plans on spending a lot of time on - it is borderline archaic, and would be like spending time on IE 11 today. The friendly bounce warning was just a courtesy - if it serves any purpose, good, if it can be slightly improved without introducing side effects or complexity, also good - otherwise, it should simply be removed and replaced with places for users to inject whatever they need.

[wolfgang42]

Yes, if you check out only the first commit of the series you will find a failing test. (Though I made it a test_command rather than root_command, happy to rearrange if you prefer the former.)

[DannyBen]

Yes, if you check out only the first commit of the series you will find a failing test. (Though I made it a test_command rather than root_command, happy to rearrange if you prefer the former.)

no need to rearrange - just a reference.

You mean these yes?

bf3955a#diff-89b4c3fdacc5cec5f8b9a9fd5ee665c0640b1e1966574f32a7422862b592ec6f

bf3955a#diff-9abd7c2657991bbfacd6f44065713e70d12327fc17d97b412f7658cbfb0db2a5

[DannyBen]

Well.

I am able to reproduce the issue clear as day.

Did you consider the bashly generate --wrap my_function use case?
As far as I understand it, this fix effectively kills it (since generating a function for the user means nothing should live outside of it).

[wolfgang42]

I did think briefly about the wrap feature, though maybe there's a use-case for it I'm not considering. My thoughts were:

• for Bash 4+: this change does cause some code to run, but since the check is passing it should have no perceptible effect (almost—if set -x is in effect there will be an additional ++ [[ 4 -lt 4 ]] printed in addition to the existing ++ return 0).
• for Bash 3: the code would already have been exiting when it used the function. In theory the sourcing script could have been sourcing it and then never calling the function, I suppose, though I have a hard time imagining why this would be generally useful. (It can't have been using set +e to ignore the problem, since the function would have set -e before doing the check, so I think the only case where it could have been doing this gracefully is in an if-type block which seems like a stretch.)

(Edit: I was thinking about the Bash 3 case all wrong, though the result still stands)

[wolfgang42]

From #116:

The very top of the bash script (solves Move bash 3 version check to top of file for early exit #114)

This method requires each script author to know about this situation; IMO it makes more sense for Bashly to do the check automatically.

[DannyBen]

Yeah. Well, my current position is this:

I realize you have put a lot of thought and effort into this, and I appreciate it.
However, as of this very moment, I feel that #116 is a more appropriate solution.

My reasoning:

1. Bash 3 is not a priority for bashly. Old software belongs on a Wikipedia page, not making the developer's life miserable.
2. I suspect that most bashly users develop scripts for themselves, their friends or colleagues. In that case, they control the environment. In any other case, it will be their responsibility to test and fix their own script on bash 3, if they choose to do so.
3. Out of the box scripts generated by bashly g will fail gracefully on bash 3 - anything beyond that, is developer land, and their responsibility.
4. In case other developers - like you - feel that they must add such a blocker at the very top of the script, they will be able to do so with Add hooks for more user code #116.

Your thoughts?

(and for future reference, to avoid work that may not be merged, perhaps lets open new ideas in an issue before PR).

[DannyBen]

This method requires each script author to know about this situation

... as is the case with any software, you need to know how to use it.
This is why there is a documentation site, tons of spec-verified examples, and bashly add <feature>.

This can be a bashly add hooks command.

IMO it makes more sense for Bashly to do the check automatically.

See my other comment. I am not convinced at all. This has the potential to cause unwanted side effects as I see it, than good.

[wolfgang42]

My thoughts on your reasoning, as requested:

1. Old software belongs on a Wikipedia page, not making the developer's life miserable.

I wish!

2. I suspect that most bashly users develop scripts for themselves, their friends or colleagues. In that case, they control the environment. In any other case, it will be their responsibility to test and fix their own script on bash 3, if they choose to do so.

This is true, though less inscrutable error messages are still helpful if their friend or colleague is using a Mac.

3. Out of the box scripts generated by bashly g will fail gracefully on bash 3 - anything beyond that, is developer land, and their responsibility.

This is true, but the out-of-the-box scripts also don’t actually do anything. It would be nice if, when you actually use bashly, the check that it ostensibly provides didn't break because of a seemingly innocent implementation.

---

On a much more general note, a quick summary of where we stand:

• The work on Make bashly-generated code set -u-safe #113 (set -u safety) depends on this PR (Bash 3 early exit); otherwise all bashly scripts will fail ungracefully in Bash 3.
• You would prefer not to merge this PR (resulting in keeping the status quo, that the bashly framework is “Bash 3 clean” and if individual script authors use syntax that isn’t it’s up to them to know about that),
• Although 113 on its own is fine (if it’s cleaned up to not interfere with everyone's scripts, which makes sense) it would break the “Bash 3 cleanness” (there may be a workaround to this, but IMO it's not worth my effort) and can’t be merged either.
• Instead, you have proposed using a hooks system (Add hooks for more user code #116) so that individual scripts can implement these checks (and others) as needed.

My philosophy is that error handling and correctness checks should be pushed as far upstream as possible, to make it easier for more people to benefit from them; but since the hooks will provide a workaround that fits my needs I’m OK with dropping my two open PRs and using hooks instead. Yours is also a reasonable stance, just a different one; and as it's not my project I leave it up to you to make the final decision on which direction to go in.

TLDR: Feel free to close #113 and #114 if you don't like them; I’d like to see Bashly have better support for these situations but am OK with keeping my opinions confined to my own scripts.

---

(and for future reference, to avoid work that may not be merged, perhaps lets open new ideas in an issue before PR).

(This is a tricky balancing act. In the case of these two PRs, most of what we’ve been discussing has come up as a result of having code to look at and wouldn't necessarily have been obvious before I started implementing. This is why I submitted #114 as a draft needing cleanup, once I realized that it wasn’t as simple as I had anticipated and would need discussion before merging.)

[DannyBen]

This is true, but the out-of-the-box scripts also don’t actually do anything

I wouldn't say that the ~500 lines of code that are generated by bashly g "don't do anything".
They manage to do so, without triggering any failure on bash 3. If I can do it, anybody can.

Although 113 on its own is fine (if it’s cleaned up to not interfere with everyone's scripts, which makes sense) it would break the “Bash 3 cleanness”

Interesting point - I didn't consider that.
I am assuming you are saying that if these changes are done, the script will in fact fail in bash 3 out of the box (without any user functions) - correct me if I misunderstood.

My philosophy is that error handling and correctness checks should be pushed as far upstream as possible.

This is my philosophy as well. The only difference between us is that I have two philosophies that take precedence:

1. Simplicity above all else, and
2. Do not bother with supporting old platforms that refuse to go away. I know this is a harsh and possibly debatable opinion, but I have been developing for enough years to be able to say that I have spent much more time on such compatibility than I should have.

I’d like to see Bashly have better support for these situations but am OK with keeping my opinions confined to my own scripts.

Me too, but not at any cost.
In any case, it is important for me to still provide solutions for use cases like yours.

---

For me - this is the important question:

Is there a way to make bashly compatible with set -u in a way that will not fail prematurely on bash 3.

If the answer is yes, then the solution is obvious:

1. Implement hooks - so people who want premature exit, can do so
2. Fix set -u compatibility in a way that still keeps the out of the box generated script fail gracefully on bash 3

[DannyBen]

And BTW - I may still decide to merge this. Perhaps after some sleep I will look at it differently.
Just a little concerned about the --wrap function.

[wolfgang42]

I am assuming you are saying that if these changes are done, the script will in fact fail in bash 3 out of the box (without any user functions) - correct me if I misunderstood.

Yes, exactly—mainly because of [[ -v var ]].

Is there a way to make bashly compatible with set -u in a way that will not fail prematurely on bash 3.

I think this could be fixed by changing that expression to [[ -z "${var+x}" ]], but I’m personally disinclined to spend the effort updating the PR to fix that (and whatever else needs to be changed). Like you, I have no patience for Bash 3, and I’d have to make this change to all of my scripts as well to get any benefit out of it; for my purposes it offers no advantages over the hook-based solution (using set -u only just before my scripts). If you would like to keep that PR but not this one, feel free to make the appropriate changes; I’ve marked it as “Maintainers can edit” for this purpose.

[DannyBen]

I have reviewed all changes, looks good. Now only left to decide if we want this or not.

It is now somewhat easier to accept, since there is an ability for the user to replace it with a custom header. It is the same reason that renders it a little less necessary though. Just a matter of what we feel is a better default, and I know your stand on the subject is that this is a better default.

If we merge it, I will also probably want to change the header for when generating with --wrap.

For now, lets keep it open until I decide one way or another, or until more evidence comes to light to tip the scale.