script: improve scripted-diff check

max-p-log-p commented at 10:17 am on January 25, 2021: none

Print generic warning if script fails and add extensible way to check for non portable commands. Tested on OpenBSD and Gentoo Linux using the command line with OpenBSD version 6.9 and GNU sed version 4.8 (Gentoo). Checked for POSIX compliance on https://www.shellcheck.net/. Fixes #19815

fanquake added the label Scripts and tools on Jan 25, 2021

in test/lint/commit-script-check.sh:1 in ca3586b350 outdated

0@@ -1,4 +1,4 @@
1-#!/bin/sh
2+#!/bin/env sh

MarcoFalke commented at 10:31 am on January 25, 2021:

./ci/lint/06_script.sh: test/lint/commit-script-check.sh: /bin/env: bad interpreter: No such file or directory

max-p-log-p commented at 10:35 am on January 25, 2021:

I made changes according to developer notes here: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#shebang

ajtowns commented at 12:33 pm on January 25, 2021:

Missing the /usr – not everyone has merged /usr. If you’re changing to to env, probably should also change it to bash as well to avoid getting weird incompatibilities?

max-p-log-p commented at 12:34 pm on January 25, 2021:

I changed it back to #!/bin/sh in a later commit

in test/lint/commit-script-check.sh:1 in 06d421cfe5 outdated

0@@ -1,4 +1,4 @@
1-#!/bin/sh
2+#!/usr/bin/env sh

MarcoFalke commented at 10:54 am on January 25, 2021:

Missing expected shebang “#!/usr/bin/env bash” or “#!/bin/sh” in test/lint/commit-script-check.sh

in test/lint/commit-script-check.sh:28 in 88cf80b7f0 outdated

23@@ -24,10 +24,16 @@ for commit in $(git rev-list --reverse $1); do
24     if git rev-list -n 1 --pretty="%s" $commit | grep -q "^scripted-diff:"; then
25         git checkout --quiet $commit^ || exit
26         SCRIPT="$(git rev-list --format=%b -n1 $commit | sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d')"
27+        echo "Script: $SCRIPT"
28+        SEDREGEX="^([^#'\"\v]|('[^'\v]*'|\"[^\"\v]*\"))*?(?<![a-z\d])sed(?1)*?-i(?:\.\w+)?[\h,]*(?2)[^#'\"\v]*(?2)"

MarcoFalke commented at 3:05 pm on January 25, 2021:

This seems a bit hard to read and impossible to maintain without comments if this should ever fail. I’d say that a simple prompt like “If you are using sed, make sure to not use bsd sed syntax” on failure is sufficient.

max-p-log-p commented at 3:22 pm on January 25, 2021:

So you don’t need me to check for sed bsd syntax or you want the regex to be simpler? I can increase false positives but increase readability and revert to an older commit.

max-p-log-p commented at 3:34 pm on January 25, 2021:

Or should I add comments to explain the regex better?

MarcoFalke commented at 3:54 pm on January 25, 2021:

I am not sure myself, but I think I’d rather not do the check. It just seems too fragile, especially with the non-greedy regex, which often isn’t implemented correctly in regex engines. And how would we check that the grep --perl-regexp is using the right regex engine, and not maybe a BSD regex engine? Add another check …?

max-p-log-p commented at 3:57 pm on January 25, 2021:

Ok, how about I just grep for sed without any regular expressions then? Do you want me to undo my commits or to just add a new one?

in test/lint/commit-script-check.sh:29 in 61fc6ea397 outdated

23@@ -24,6 +24,10 @@ for commit in $(git rev-list --reverse $1); do
24     if git rev-list -n 1 --pretty="%s" $commit | grep -q "^scripted-diff:"; then
25         git checkout --quiet $commit^ || exit
26         SCRIPT="$(git rev-list --format=%b -n1 $commit | sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d')"
27+        if echo "$SCRIPT" | grep --quiet "sed"; then
28+            echo "Warning: do not use BSD sed syntax"
29+            echo "Example: sed -i '' 's/foo/bar/' filename (Bad)"

MarcoFalke commented at 8:32 am on January 26, 2021:

Would be better to only print this warning if the check fails, no?

MarcoFalke commented at 8:40 am on January 26, 2021:

With “check” I mean “script evaluation”

in test/lint/commit-script-check.sh:36 in d15770a0c5 outdated

30@@ -31,7 +31,10 @@ for commit in $(git rev-list --reverse $1); do
31         else
32             echo "Running script for: $commit"
33             echo "$SCRIPT"
34-            (eval "$SCRIPT")
35+            if ! (eval "$SCRIPT") && (echo "$SCRIPT" | grep --quiet "sed"); then
36+                echo "Warning: do not use BSD sed syntax"
37+                echo "Example: sed -i '' 's/foo/bar/' filename (Bad)"

ajtowns commented at 3:52 pm on February 15, 2021:

I would have expected a generic warning that the command failed, maybe something more like:

0if ! (eval "$SCRIPT"); then
1    echo "Warning: Script failed (non-zero exit code)"
2    if echo "$SCRIPT" | grep -q "sed"; then
3        echo "Suggestion: did you use BSD sed syntax?"
4        echo "Example: sed -i '' 's/foo/bar/' filename (Bad)"
5    fi
6fi

MarcoFalke commented at 4:23 pm on February 15, 2021:

sed doesn’t have an exit code for “error”, so checking the exit code wouldn’t work either way.

0$ sed -i -e 's/FOOBAR/foobar/g' ./src/init.cpp && echo $?
10

max-p-log-p commented at 10:08 am on April 17, 2021:

“sed -i ’’ -e ’s/FOOBAR/foobar/g’ ./src/init.cpp && echo $?” results in no output for me since the sed command fails and the “echo $?” command is not executed but “sed -i -e ’s/FOOBAR/foobar/g’ ./src/init.cpp” does in fact work. I think this is because “sed -i -e ’s/FOOBAR/foobar/g’ ./src/init.cpp” is a legitimate sed command, it just doesn’t cause any changes since FOOBAR is not present in the file.

max-p-log-p commented at 12:13 pm on April 17, 2021:

In the OpenBSD 6.9 man pages for sed, it states EXIT STATUS The sed utility exits 0 on success, and >0 if an error occurs.

MarcoFalke commented at 2:50 pm on April 17, 2021:

failing to replace is not an error

max-p-log-p commented at 9:21 pm on April 17, 2021:

The line “git –no-pager diff –exit-code $commit && echo “OK” || (echo “Failed”; false) || RET=1” should check if the script failed to change anything, do you want to check each command and error if if has no change?

max-p-log-p commented at 9:36 pm on April 17, 2021:

Do you want to not check for non zero exit codes but only check if the output of the script is the same as the commit?

DrahtBot commented at 3:58 pm on April 22, 2021: 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:

#23462 (test: Enable SC2046 and SC2086 shellcheck rules by hebasto)

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.

78051301012 commented at 1:48 am on April 25, 2021: none

.

DrahtBot added the label Needs rebase on May 10, 2021

DrahtBot removed the label Needs rebase on Jun 12, 2021

adamjonas commented at 12:30 pm on June 23, 2021: member

Hi @4D617278, would you mind squashing your commits as described in these guidelines?

DrahtBot added the label Needs rebase on Jun 25, 2021

Print warning if the script fails and print warning for each line of a nonportable command which is present in the script when the script fails

Fix previous sed command that sometimes fails when using BSD sed, error "expected EOF..."

7eae56b5ac

DrahtBot removed the label Needs rebase on Jun 25, 2021

DrahtBot added the label Needs rebase on Nov 15, 2021

DrahtBot commented at 5:26 pm on November 15, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 12:17 pm on February 22, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

MarcoFalke commented at 12:23 pm on February 22, 2022: member

Looks like #https://github.com/bitcoin/bitcoin/issues/19815 is already fixed?

MarcoFalke closed this on Feb 22, 2022

DrahtBot locked this on Feb 22, 2023

script: improve scripted-diff check #21002

Conflicts