Skip to content
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.

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

treewide: Remove some bashism #2647

Closed
wants to merge 2 commits into from

Conversation

SvenRoederer
Copy link
Contributor

@SvenRoederer SvenRoederer commented Dec 25, 2019

This removes some non POSIX extensions from the shell-scripts, as already done in d6ac8ca. It targets "[[" and "==".

@adschm
Copy link
Member

adschm commented Dec 26, 2019

For commit messages like "[[ is a bash-extension for test the ash-implementation is in fully compatible." I think it makes more sense to just put the "[[" in standard quotes.

@dedeckeh dedeckeh added core packages pull request/issue for core (in-tree) packages work in progress pull request the author is still working on labels Dec 26, 2019
@neheb
Copy link
Contributor

neheb commented Dec 27, 2019

If bashisms are to be removed, it makes sense to change the top shebang to point to /bin/sh as dash is faster than bash.

edit: A moot point as these do not run on the host but it's still correct to add a shebang.

@adschm
Copy link
Member

adschm commented Dec 30, 2019

@SvenRoederer SvenRoederer force-pushed the remove_bashism branch 3 times, most recently from a55ce50 to 9624548 Compare December 30, 2019 22:57
@SvenRoederer
Copy link
Contributor Author

@neheb I added the shebang to the files I edited. The scripts I edited are only running on the target-board, so /bin/sh links to busyboy-ash, right?

@adrianschmutzler Regarding "-a": see 01c84b4

@adschm
Copy link
Member

adschm commented Dec 31, 2019

@SvenRoederer I think it will be more efficient if you already provide properly formatted patches in the first place. That will allow me to pick what's ready (or looks correct) right away. Currently, I would have to look at rather "trivial" patches several times, but I do not think this will benefit so much from having it pre-reviewed.
However, feel free to stick to your current plan if you prefer.

@adschm
Copy link
Member

adschm commented Jan 18, 2020

Ping.

@SvenRoederer SvenRoederer force-pushed the remove_bashism branch 4 times, most recently from 4b3dfee to 3f7d2ae Compare January 26, 2020 01:07
@SvenRoederer SvenRoederer marked this pull request as ready for review January 26, 2020 01:08
@SvenRoederer
Copy link
Contributor Author

@adrianschmutzler commits db23f92 , 38bebf5 , aeb858d , 0cc2d7c can be cherry-picked.

The commits related to igmpproxy should be reviewed.

@adschm
Copy link
Member

adschm commented Jan 26, 2020

Note that the author of your patches (From:) does not match your SOB. I corrected that manually for those patches already merged, but please take care of future patches yourself.

@SvenRoederer
Copy link
Contributor Author

Adding the shebang was based on this comment #2647 (comment).

I changed my github settings, which hopefully improves the "From:" header.

"[[" is a bash extension for test. As the ash-implementation is not
fully compatible we drop its usage.
This follows up 3519bf4

Signed-off-by: Sven Roederer <devel-sven@geroedel.de>
@SvenRoederer
Copy link
Contributor Author

@adrianschmutzler I just got a ping for this PR and reworked commits according to your comments. I also tested the change of the 2 commit to give the same results as the original.

@adschm
Copy link
Member

adschm commented May 4, 2020

Hmm, just had another look and I still don't like it. The first commit tells to change brackets, but also changes &&/-a and -z/-n, only to have it changed again in the second commit.

Please separate these changes properly. Despite, be aware that I'm treating this low-priority as it's cosmetic.

Thanks for your work, however.

@adschm adschm removed the work in progress pull request the author is still working on label May 4, 2020
@adschm
Copy link
Member

adschm commented Jun 23, 2020

I've squashed the two patches together with a slightly modified message and will merge them later today.

@adschm adschm closed this Jun 23, 2020
@SvenRoederer
Copy link
Contributor Author

Thanks for taking care.... I had it on my list with a list with a low priority ...

@SvenRoederer SvenRoederer deleted the remove_bashism branch June 24, 2020 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core packages pull request/issue for core (in-tree) packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants