Skip to content

doc: Add bash dependency of lint tests #24750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
fjahr wants to merge 2 commits into from
Closed

doc: Add bash dependency of lint tests #24750

fjahr wants to merge 2 commits into from

Conversation

@fjahr
Copy link
Contributor

@fjahr fjahr commented Apr 3, 2022

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.

@fjahr fjahr mentioned this pull request Apr 3, 2022
Closed
jarolrod
jarolrod reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concept ack

@RandyMcMillan
Copy link
Contributor

RandyMcMillan commented Apr 4, 2022

concept ACK

@RandyMcMillan
Copy link
Contributor

RandyMcMillan commented Apr 4, 2022 •
edited
Loading

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
Copy link
Contributor

brunoerg commented Apr 4, 2022

Since #24610 has been closed, concept ACK.

@maflcko
Copy link
Member

maflcko commented Apr 4, 2022

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
Copy link
Contributor

brunoerg commented Apr 4, 2022

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
Copy link
Member

laanwj commented Apr 4, 2022

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
Copy link
Contributor Author

fjahr commented Apr 4, 2022

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.

@fjahr fjahr force-pushed the 202203-mapfile-doc branch from 2f623e5 to d8cc799 Compare April 4, 2022 20:03
@fjahr
Copy link
Contributor Author

fjahr commented Apr 4, 2022

Also addressed @jarolrod 's comments on the doc changes, thanks for reviewing!

@DrahtBot DrahtBot mentioned this pull request Apr 4, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 5, 2022

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
brunoerg approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK d8cc799

@maflcko
Copy link
Member

maflcko commented Apr 6, 2022

NACK. Agree with laanwj

@fjahr
Copy link
Contributor Author

fjahr commented Apr 6, 2022 •
edited
Loading

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 fjahr closed this Apr 6, 2022
@laanwj
Copy link
Member

laanwj commented Apr 6, 2022

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.

@bitcoin bitcoin locked and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jarolrod jarolrod jarolrod left review comments

@brunoerg brunoerg brunoerg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fjahr @RandyMcMillan @brunoerg @maflcko @laanwj @DrahtBot @jarolrod