Pure bash & reduce code duplication

[dotysan]

• sleep and date aren't part of bash
• also more portable by removing non-standard nanoseconds
• still uses clock in microseconds - move common routines into single sourced file
• increase portability by checking env for location of bash
• some minor code linting
• also display the bash version in docker run.sh

Description

This is now pure bash. It might improve performance slightly by reducing syscalls to sleep and date.

Also only GNU date supports %N (nanoseconds), so it would have broken on BSD or other platforms that only support strftim(3) proper. The only drawback is internal printf %(datefmt)T supports only standard strftim(3) so we must use microseconds instead of nano.

The other changes are mostly cosmetic. But making the scripts honor the environment, now users can test against their own bash wherever is it installed, not just /bin/bash.

Contributing requirements

• [x] I read the contribution guidelines in CONTRIBUTING.md.
• [x] I placed my solution in the correct solution folder.
• [x] I added a README.md with the right badge(s).
• [x] I added a Dockerfile that builds and runs my solution.
• [x] I selected drag-race as the target branch.
• [x] All code herein is licensed compatible with BSD-3.

[rbergen]

@dotysan Looking at the PR commits, it seems to me like this PR incorporates the changes of #894. If that's indeed the case, then I suggest we close #894 and handle both updates in this PR. It would also mean we only have to ask @Nitepone once for feedback on these changes to their solution.

[dotysan]

Yes, correct. Two commits separate objectives/commits. But this second one depends on the first.

If it's easier for @Nitepone, @Blzut3 and others to give their input on both ideas here, sure!

[dotysan]

It's much easier to review them separately, though.

[rbergen]

That would be true if both sets of changes were indeed separated (for instance, by having created two branches from the same branch point, each containing the changes of one commit). As it stands, this PR includes both commits and hence the changes of both commits. Now it's only easier to review if we process and merge #894 first, you then sync/rebase to the updated drag-race, and we only then look at this PR.

Anyway, I don't think it's that important in this case, because the combined set of code changes is quite manageable to review in my opinion.

[dotysan]

Yes, I was expecting them to be done consecutively. Guess I could have held this one back until #894 was accepted...

I don't mind rebasing this one if/after the other is cleaned up and merged. Either way. I'm just having fun.

[rbergen]

I hope so. That means we both are. :)

Let's just roll up both changes in this PR, close #894 and wait for @Nitepone's feedback on this one.

[Blzut3]

According to the bash manual in regards to $(()) vs $[]:

The old format $[expression] is deprecated and will be removed in upcoming versions of bash.

The date change seems fine to me with the already stated microsecond vs nanosecond caveat.

In regards to your while [ $tNow -lt $tStop ], It doesn't really matter since it pretty much only executes once but bash's [[ ]] is typically faster than the posix [ ] since it uses a different parser for the contents, and in the case of arithmetic operations (( )), which performs similarly, is probably the better choice in my experience. If going this route it might even be better to use the for (( )) syntax.

My suspicion is that the while kill pattern is still faster yet since, with only built-ins and no function calls at play, you can generally estimate bash's performance at this level by counting the number of variable references which has gone from 1 to 4 (two in comparison, two in assignment at the end of the loop). Again though, the actual code under test takes long enough to execute that it's kind of moot either way.

I wasn't particularly concerned about sleep not being a built in since it's not the code under test (same goes for date really), but rather just a mechanism for telling time elapsed. But you could do a sort of best of both worlds by having a timing function run in the background (busy loop) and then kill can still be used to see if the timer is up without wasting time referencing variables on the main thread.

[Nitepone]

Hey, sorry to be looking at this a bit late!

At first, I was a little concerned that moving so much into a common file might hurt readability.. But I really enjoy how the common file improves the ability to compare the different implementations! I think this is a great improvement for anyone reading through this code quickly out of curiosity.

also more portable by removing non-standard nanoseconds

This is interesting, I didn't know that, and it makes sense as bug since I've really only worked with GNU utils.

These changes mostly lgtm! Though @Blzut3 's comment about $[] deprecation would make me prefer $(()) instead. Especially since this change is outside the code to test and doesn't provide any performance improvements.

[rbergen]

@Nitepone That's okay, thank you for responding now.

@dotysan Would you be willing to revert the switch from $(())) to $[]? If so, I think we have consensus that the remaining changes are welcome improvements to the solution.

[dotysan]

All good input. Willdo!

Should I create a new PR or just push to this one again?

[dotysan]

I've borrowed a trick from https://github.com/dylanaraps/pure-bash-bible/blob/8c19d0b482b04d8d50fb72b4c5148b41ce605c6d/README.md?plain=1#L1950 and put the kill -0 back in!

[rbergen]

Should I create a new PR or just push to this one again?

Please just update this one. That means we have the full history concerning the soon-to-be-merged changes (commits and discussion) in one PR.

[dotysan]

OK @rbergen @Blzut3 @Nitepone thunk I've addressed all of your concerns.

[rbergen]

It sure LGTM!

@Blzut3 @Nitepone Would you agree?

[Blzut3]

An instance of $[] was missed, but otherwise looks good.

[dotysan]

Good eye @Blzut3. That was vestigial code from another unpublished branch. And old habits die hard. ;-)

Final patch coming up in a minute...

[rbergen]

An instance of $[] was missed, but otherwise looks good.

Good catch indeed! I guess I've developed a bit of a blind spot for code that's commented out, unless it is obviously out of place. :)

We'll give @Nitepone a few more days to respond as well. I'll merge the PR this weekend, at the latest.

[rbergen]

It seems we're running into a snag with merging this, due to Phix starting to misbehave. I've opened #898 to disable automated builds of Phix/solution_1. When that's been merged I'll sync this PR's source branch with the updated drag-race and then move forward with merging this.

EDIT: #898 has been changed to update Phix instead of disabling its automated builds. Beyond that, the plan for proceeding is unchanged.