Sed compatibility

[desh2608]

To solve SED compatibility issue #2225

Replaced all occurences of sed -i with perl -i -pe.
Tested on AMI recipe and seems to be working fine.

[danpovey]

I tested this and it seemed to remove the final newline.

[kkm000]

@danpovey, as a project to use clang-tidy is under way, maybe we should not touch this file at all? It's a maintainer's file, so presumably sed is fine here for the time being, only a handful of people run it if ever.

[desh2608]

Made some further modifications:

• grep -v to delete strings matching a pattern
• Changed perl syntax for replacing strings or deleting specific line numbers
• ex -s -c (Vim executable command) to add a line at beginning of file

[danpovey]

@desh2608 there is a conflict here, could you please resolve it?

Also it would be nice to have someone experienced to look over these changes and make sure they look safe.

[desh2608]

Conflicts are resolved, but yes, perhaps it would be best if someone looks over the changes.

[danpovey]

@kkm000 do you have time to check over this?

[kkm000]

@danpovey Will do. So the goal is to get rid of sed entirely, because of differences in the wild, right?

[kkm000]

@desh2608, give me about an hour or two to get to this. I spy a a couple problems in the change. One is grep cannot modify a file in-place, like sed -i or perl -i, but we can do everything in perl. Another is a subtle difference regarding the handling of \n at the end. In sed, the final \n is not whitespace but in perl it is, so deleting whitespace at EOL will glue lines together in perl but not in sed. I'm sure both your grep and ex uses could be replaced with perl. ex would be a whole new can of worms to open (it can be vi, vim, or, heaven save, the original ex, "a better ed", that both vi and vim emulate, with better or worse accuracy), let's better not use it! : )

perl has practically no quirks across systems, AFAIK, so it's a good idea to use it alone.

[danpovey]

That makes sense, you can leave that maintainer's file alone.

…

On Tue, Mar 5, 2019 at 12:46 PM kkm (aka Kirill Katsnelson) < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In misc/maintenance/fix_cpplint_whitespace.sh <#2981 (comment)>: > @@ -3,11 +3,11 @@ # Let's run a set of in-place modifications by sed-commands, for file in $@; do - sed -i 's/; \/\//; \/\//' $file # '; //' -> '; //' - sed -i 's/{ \/\//{ \/\//' $file # '{ //' -> '{ //' - sed -i 's/} \/\//} \/\//' $file # '} //' -> '} //' - sed -i 's/for(/for (/' $file # 'for(' -> 'for (' - sed -i 's/if(/if (/' $file # 'if(' -> 'if (' - sed -i 's/\s\s*$//' $file # 'remove white-space at the end of lines' + perl -i -pe 's/; \/\//; \/\//' $file # '; //' -> '; //' + perl -i -pe 's/{ \/\//{ \/\//' $file # '{ //' -> '{ //' + perl -i -pe 's/} \/\//} \/\//' $file # '} //' -> '} //' + perl -i -pe 's/for(/for (/' $file # 'for(' -> 'for (' + perl -i -pe 's/if(/if (/' $file # 'if(' -> 'if (' + perl -i -pe 's/\s\s*$//' $file # 'remove white-space at the end of lines' @danpovey <https://github.com/danpovey>, as a project to use clang-tidy is under way, maybe we should not touch this file at all? It's a maintainer's file, so presumably sed is fine here for the time being, only a handful of people run it if ever. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2981 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVu0ZfM0HleEFtlzVC1ttX8zxOIUSGks5vTq1rgaJpZM4Z26Th> .

[desh2608]

@kkm000 Thanks for your detailed comments! I made the changes that you suggested, including deleting by pattern rather than line numbers (except in 1 file where no such pattern existed). Also rewrote some code blocks to make changes in single pass through file rather than looping.

@danpovey I believe this can be merged now.

[kkm000]

Thanks. Just these two little things.

[desh2608]

@kkm000 Thanks for the comments. I have made these fixes.

[kkm000]

LGTM, thanks!

Not everyone shares my fondness for bash, but I like it as a scripting programming language. It has it quirks, but what language doesn't, and it's a very powerful shell. As such, I appreciated the literally one-liner bashism to get the expression that matches all phones! : )

[desh2608]

@danpovey Perhaps this can be merged now before conflicts arise.

[danpovey]

Thanks, guys! Will merge.