Skip to content

Conversation

@max-p-log-p
Copy link

@max-p-log-p max-p-log-p commented Jan 25, 2021

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

sed doesn't have an exit code for "error", so checking the exit code wouldn't work either way.

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

Copy link
Author

Choose a reason for hiding this comment

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

"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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

failing to replace is not an error

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

DrahtBot commented Apr 22, 2021

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.

Copy link

@78051301012 78051301012 left a comment

Choose a reason for hiding this comment

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

.

@adamjonas
Copy link
Member

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

…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..."
@DrahtBot
Copy link
Contributor

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

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • 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.

@maflcko
Copy link
Member

maflcko commented Feb 22, 2022

Looks like ##19815 is already fixed?

@maflcko maflcko closed this Feb 22, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Feb 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

improve scripted-diff check

7 participants