doc: Add bash dependency of lint tests #24750

pull fjahr wants to merge 2 commits into bitcoin:master from fjahr:202203-mapfile-doc changing 5 files +19 −0

fjahr commented at 11:05 PM on April 3, 2022: member

This issue has already been the topic of #24610.

macOS, sadly, does not ship with a bash version that is compatible with all lint tests since they now include the usage of mapfile. mapfile was introduced in bash 4.0 but macOS still uses 3.x because the newer version changed their license from GPL2 to GPL3. macOS has changed the default shell to zsh but that doesn't help since bash 3.x is still installed and will be used due to the shebang. Also, zsh does not have mapfile the same way bash has. Confusingly, the zsh version of the command does something completely different than the bash version.

Including guard code was rejected in #24610 and I guess this is probably reasonable since only a hand full of developers will ever run into this. But I think it should still be mentioned in the docs.
DrahtBot added the label Docs on Apr 3, 2022
DrahtBot added the label Tests on Apr 3, 2022
in test/README.md:306 in b26ee83570 outdated
```
 302 | @@ -303,6 +303,15 @@ Use the `-v` option for verbose output.
 303 |  
 304 |  #### Dependencies
 305 |  
 306 | +##### Shell
```
jarolrod commented at 12:17 AM on April 4, 2022:

You could changes this from Shell to Bash. This would help hint that a macOS user should switch from zhs to bash.

fjahr commented at 8:03 PM on April 4, 2022:

done, thanks!

in test/README.md:310 in b26ee83570 outdated

 302 | @@ -303,6 +303,15 @@ Use the `-v` option for verbose output.
 303 |  
 304 |  #### Dependencies
 305 |  
 306 | +##### Shell
 307 | +
 308 | +Several lint tests require a `bash` shell version 4 or higher to be available.
 309 | +
 310 | +Especially modern macOS versions (at least 10.15) still require installation of

jarolrod commented at 12:17 AM on April 4, 2022:

I think the additions here could be condensed into:


diff --git a/test/README.md b/test/README.md
index 87a3acb76..cb96697e4 100644
--- a/test/README.md
+++ b/test/README.md
@@ -303,6 +303,13 @@ Use the `-v` option for verbose output.
 
 #### Dependencies
 
+##### Bash
+
+Several lint tests require a `bash` shell version of 4 or higher.
+macOS systems require upgrading the system provided `bash`.
+
+##### Python
+

fjahr commented at 8:03 PM on April 4, 2022:

done, thanks!

jarolrod commented at 12:18 AM on April 4, 2022: member

concept ack
RandyMcMillan commented at 4:00 AM on April 4, 2022: contributor

concept ACK
RandyMcMillan commented at 4:04 AM on April 4, 2022: contributor

In a related issue - the default sed version fails on MacOS...

#24159

It may be useful to add gsed to the list of homebrew dependancies.
brunoerg commented at 12:12 PM on April 4, 2022: member

Since #24610 has been closed, concept ACK.
MarcoFalke commented at 12:18 PM on April 4, 2022: member

If there are systems that ship an outdated bash, it may be better to reopen #24610 instead. A simple automated check is faster and more friendly than documentation, which needs to be searched and interpreted by a human.
brunoerg commented at 1:48 PM on April 4, 2022: member

If there are systems that ship an outdated bash, it may be better to reopen #24610 instead. A simple automated check is faster and more friendly than documentation, which needs to be searched and interpreted by a human.

I agree. It would be more friendly.
laanwj commented at 1:50 PM on April 4, 2022: member

I'd prefer one external scripting dependency (Python), but if we insist on modern bash-isms, could do both #24610 and this. Both documenting and detecting dependencies makes sense.
fjahr commented at 6:35 PM on April 4, 2022: member

I have added a commit that adds the bash warning to all the linters that use mapfile. I had written it yesterday but ended up not pushing it because of the negative sentiment in #24610. If there is interest in adding this together with the docs we can do it here. I added @brunoerg as co-author of course.

I agree switching to python is the better solution and happy to help with that as well. Then the only question is if it still makes sense to still add this now.
doc: Add bash dependency of lint tests 117a5abd4d
lint: Skip and warn in case of old bash version
co-authored-by: brunoerg <brunoely.gc@gmail.com>
d8cc7993aa
fjahr force-pushed on Apr 4, 2022
fjahr commented at 8:04 PM on April 4, 2022: member

Also addressed @jarolrod 's comments on the doc changes, thanks for reviewing!
DrahtBot commented at 1:13 AM on April 5, 2022: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24766 (lint: convert spellchecking lint test to python by fjahr)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
brunoerg approved
brunoerg commented at 11:51 PM on April 5, 2022: member

crACK d8cc7993aa2985e785acc09c7a1b64989db5877a
MarcoFalke commented at 6:06 AM on April 6, 2022: member

NACK. Agree with laanwj
fjahr commented at 6:56 AM on April 6, 2022: member

Thanks for the reviews but I will close this for now since I have already converted almost all of the offending code to Python and I will finish the rest today. I have not seen too many compatibility issues myself previously but I think converting to Python makes this also easier to maintain in the future because more developers are available.

See #24766 and #24778.
fjahr closed this on Apr 6, 2022
laanwj commented at 10:52 AM on April 6, 2022: member

Thanks for the reviews but I will close this for now since I have already converted almost all of the offending code to Python and I will finish the rest today.

That's awesome! Thank you.
DrahtBot locked this on Apr 6, 2023

Contributors

Labels

Linked (view graph)

#24610 test, lint: check bash version before executing codespell

doc: Add bash dependency of lint tests #24750

Conflicts