test: Prevent non-compatible sed binary for scripted-diffs #23543

pull anouarkappitou wants to merge 2 commits into bitcoin:master from anouarkappitou:improve-scripted-diff-check changing 1 files +5 −0
  1. anouarkappitou commented at 1:50 PM on November 18, 2021: contributor

    This Pull request improve scripted diff by checking for non-compatible sed binary. Fixes #19815

  2. MarcoFalke commented at 1:56 PM on November 18, 2021: member

    Heh, seems brittle, but sufficiently acceptable as an approximation.

    cr ACK be5286f8a2f8647f71a51d2af07bfaa70577b3d5

  3. DrahtBot added the label Tests on Nov 18, 2021
  4. in test/lint/commit-script-check.sh:21 in be5286f8a2 outdated 
      16 | @@ -17,6 +17,11 @@ if test -z "$1"; then
  17 |      exit 1
  18 |  fi
  19 |  
  20 | +if ! sed --help | grep -q 'GNU'; then
  21 | +    echo "Error: the installed sed package is not compatible, please make sure you have GNU sed installed in your system";
    shaavan commented at 3:00 PM on November 18, 2021: 
        echo "Error: the installed sed package is not compatible. Please make sure you have GNU sed installed in your system.";
    anouarkappitou commented at 3:08 PM on November 18, 2021:

    Thank you for the suggestion @shaavan

  5. shaavan commented at 3:06 PM on November 18, 2021: contributor

    Concept ACK

    This PR runs the sed --help command and checks silently (i.e., without printing results) for the Keyword "GNU" presence in it. This makes sure that GNU sed is present in the system.

    I agree with the approach; however, I have some suggestions to improve the error message.

  6. katesalazar commented at 8:38 PM on November 18, 2021: contributor 
    $ if ! sed --help | grep -q 'GNU'; then echo "error"; fi
sed: illegal option -- -
usage: sed script [-Ealnru] [-i extension] [file ...]
        sed [-Ealnu] [-i extension] [-e script] ... [-f script_file] ... [file ...]
error
$ ▮

    Concept ACK.

  7. shaavan commented at 7:54 AM on November 20, 2021: contributor

    @anouarkappitou The updated error message looks good. I would recommend squashing the commits together.

  8. script: preventing non-compatible sed binary. 30df5c3dd4
  9. anouarkappitou force-pushed on Nov 20, 2021
  10. anouarkappitou commented at 11:09 AM on November 20, 2021: contributor

    Thank you @shaavan for the suggestions, it's been updated.

  11. shaavan approved
  12. shaavan commented at 11:57 AM on November 20, 2021: contributor

    ACK 30df5c3dd4d39d9027b0341d01d3233400104893

  13. in test/lint/commit-script-check.sh:24 in 30df5c3dd4 outdated 
      16 | @@ -17,6 +17,11 @@ if test -z "$1"; then
  17 |      exit 1
  18 |  fi
  19 |  
  20 | +if ! sed --help | grep -q 'GNU'; then
  21 | +    echo "Error: the installed sed package is not compatible. Please make sure you have GNU sed installed in your system.";
  22 | +    exit 1;
  23 | +fi
  24 | +
    hebasto commented at 7:21 PM on November 21, 2021:

    Heh, seems brittle...

    Why don't check the required functionality directly:

    echo "test" > tmp
sed -i 's/test/ok/' tmp &> /dev/null
ret=$?
rm tmp
if [ $ret -ne 0 ]; then
  echo "Error: the installed sed package is not compatible. Please make sure you have GNU sed installed in your system."
  exit 1
fi

    ?

    anouarkappitou commented at 1:15 PM on November 28, 2021:

    Hello @hebasto, Thank you for the suggestion. I was thinking to use this approach in-fact, but I avoided it due to performance reasons, please correct me if I'm wrong, by running string substitution, will require to start PCRE engine before performing string replacement, also it has overhead of creating and removing temporary file.

    hebasto commented at 1:22 PM on November 28, 2021:

    Correct. There are performance issues with this approach. But, as almost always, performance should not be considered at first place :) I mean, are performance drawbacks a real issue?

    anouarkappitou commented at 2:49 PM on November 28, 2021:

    I see your point, if we have enough consent regarding this approach, I'll be happy to refactor the current changes. Thank you again @hebasto What do you think @shaavan, @katesalazar and @MarcoFalke ?

    katesalazar commented at 2:19 PM on November 30, 2021:

    I think all of these ways to do this same thing are good. I could also concept NAK both this and the parent issue. IDK who are we forcing this on, besides macOS users. But I suppose macOS users should be OK to slip in their stack just one more trusted build.

  14. dongcarl commented at 5:55 PM on November 22, 2021: member

    Perhaps a point of discussion for another place, but is it possible to do sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d' by chaining a few different UNIX string manipulation tools? Perhaps it'd be a bit more readable too?

  15. katesalazar commented at 8:05 PM on November 22, 2021: contributor 
    $ sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d'
sed: 1: "/^-BEGIN VERIFY SCRIPT- ...": unexpected EOF (pending }'s)
$ echo $?
1

    🤔

    beats me

  16. dongcarl commented at 3:56 AM on November 23, 2021: member

    @katesalazar I think this is exactly what this PR is addressing, you're running BSD-sed, which does not have the GNU extensions required to have this sed script run successfully

  17. laanwj renamed this:
    script: preventing non-compatible sed binary.
    test: Prevent non-compatible sed binary for scripted-diffs
    on Nov 30, 2021
  18. laanwj commented at 6:18 PM on November 30, 2021: member

    Concept ACK, I've renamed the PR because script: is ambigious (it can also refer to bitcoin script).

  19. hebasto commented at 7:37 PM on November 30, 2021: member

    Concept ACK, I've renamed the PR because script: is ambigious (it can also refer to bitcoin script).

    Sorry for off-topic here, but in that case https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request should be improved, no?

  20. laanwj commented at 1:22 PM on December 6, 2021: member

    Sorry for off-topic here, but in that case https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request should be improved, no?

    Maybe or maybe I'm just wrong. I still think "test:" is more appropriate for this specific PR though.

  21. DrahtBot commented at 1:35 PM on December 6, 2021: member

    I agree, as I assigned this label, too. #23543#event-5639386043

  22. 2310Mas approved
  23. in test/lint/commit-script-check.sh:20 in 30df5c3dd4 outdated 
      16 | @@ -17,6 +17,11 @@ if test -z "$1"; then
  17 |      exit 1
  18 |  fi
  19 |  
  20 | +if ! sed --help | grep -q 'GNU'; then
    theStack commented at 11:35 PM on January 1, 2022: 
    if ! sed --help 2>&1 | grep -q 'GNU'; then

    Could redirect stderr to stdout in order to not show any output?

    On OpenBSD's sed, there is no --help parameter, i.e. an error message and the usage is printed on stderr:

    $ ./test/lint/commit-script-check.sh HEAD~100
sed: unknown option -- -
usage: sed [-aEnru] [-i[extension]] command [file ...]
       sed [-aEnru] [-e command] [-f command_file] [-i[extension]] [file ...]
Error: the installed sed package is not compatible. Please make sure you have GNU sed installed in your system.
    anouarkappitou commented at 11:38 AM on January 22, 2022:

    Hello @theStack, Thank you for the suggestion, it was applied.

  24. theStack commented at 11:41 PM on January 1, 2022: member

    Concept ACK

  25. script: redirecting stderr to stdout before pipelining into grep
    Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
    d8dfc403f7
  26. shaavan approved
  27. shaavan commented at 3:12 PM on January 22, 2022: contributor

    reACK d8dfc403f74858b79c87f650b5d609aa60a4dc3b

    Changes since my last review:

    • Added 2>&1 in the command to redirect standard error to standard output.

    I agree with this change for the reasons @theStack discussed in this comment.

    p.s.: Here's one excellent article I found about understanding 2>&1.

  28. theStack approved
  29. theStack commented at 9:13 PM on January 22, 2022: member

    Tested ACK d8dfc403f74858b79c87f650b5d609aa60a4dc3b

    Checked that the introduced error message appears on OpenBSD 7.0 (which doesn't have GNU sed in the base system):

    $ ./test/lint/commit-script-check.sh HEAD~100
Error: the installed sed package is not compatible. Please make sure you have GNU sed installed in your system.
$ echo $?
1

    Also verified on Debian bookworm/sid that the script still runs if GNU sed is available.

  30. MarcoFalke merged this on Jan 24, 2022
  31. MarcoFalke closed this on Jan 24, 2022
  32. sidhujag referenced this in commit bd8973ed4c on Jan 28, 2022
  33. PastaPastaPasta referenced this in commit 01361608e9 on Apr 7, 2022
  34. PastaPastaPasta referenced this in commit 4e9dc7e046 on Apr 11, 2022
  35. DrahtBot locked this on Jan 24, 2023
Contributors
anouarkappitouMarcoFalkeshaavankatesalazarhebastodongcarllaanwjDrahtBot2310MastheStack
Labels
Tests
Linked (view graph)
#19815 improve scripted-diff check
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me