On Mon, Sep 02, 2024 at 06:08:28PM +0200, Miguel Ojeda wrote: > Like patch "rust: suppress error messages from > CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], do not assume the file existing > and being executable implies executing it will succeed. > > Instead, bail out if executing it fails for any reason, as well as if > the program does not print to standard output what we are expecting from > `pahole --version`. The script needs to ensure that it always returns > an integer, since its output will go into a Kconfig `int`-type symbol. > > In addition, check whether the program exists first, and provide > error messages for each error condition, similar to how it is done in > e.g. `scripts/rust_is_available.sh`. > > For instance, currently `pahole` may be built for another architecture > or may be a program we do not expect that fails: > > $ echo 'bad' > bad-pahole > $ chmod u+x bad-pahole > $ make PAHOLE=./bad-pahole defconfig > ... > ./bad-pahole: 1: bad: not found > init/Kconfig:112: syntax error > init/Kconfig:112: invalid statement > > With this applied, we get instead: > > *** > *** Running './bad-pahole' to check the pahole version failed with > *** code 127. pahole will not be used. > *** > ... > $ grep CONFIG_PAHOLE_VERSION .config > CONFIG_PAHOLE_VERSION=0 > > Similarly, `pahole` currently may be a program that returns successfully, > but that does not print the version that we would expect: > > $ echo 'echo' > bad-pahole > $ chmod u+x bad-pahole > $ make PAHOLE=./bad-pahole defconfig > ... > init/Kconfig:114: syntax error > init/Kconfig:114: invalid statement > > With this applied, we get instead: > > *** > *** pahole './bad-pahole' returned an unexpected version output. > *** pahole will not be used. > *** > ... > $ grep CONFIG_PAHOLE_VERSION .config > CONFIG_PAHOLE_VERSION=0 > > Finally, with this applied, if the program does not exist, we get: > > $ make PAHOLE=./bad-pahole defconfig > ... > *** > *** pahole './bad-pahole' could not be found. pahole will not be used. > *** > ... > $ grep CONFIG_PAHOLE_VERSION .config > CONFIG_PAHOLE_VERSION=0 > > Link: https://lore.kernel.org/rust-for-linux/20240727140302.1806011-1-masahiroy@xxxxxxxxxx/ [1] > Co-developed-by: Nicolas Schier <nicolas@xxxxxxxxx> > Signed-off-by: Nicolas Schier <nicolas@xxxxxxxxx> > Signed-off-by: Miguel Ojeda <ojeda@xxxxxxxxxx> > --- > v1: https://lore.kernel.org/all/20240728125527.690726-1-ojeda@xxxxxxxxxx/ > v2: > > Reworked to catch successful programs that may not output what we expect from > pahole as well as to do the checking step-by-step (for better error messages). > > I didn't change the regular expression to reduce the changes (except adding > `p`) -- it can be improved separately. > > Also, please note that I added Nicolas as co-author since he proposed part of > the solution, but he has not formally signed off yet. thanks, no objections. > scripts/pahole-version.sh | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh > index f8a32ab93ad1..cdb80a3d6e4f 100755 > --- a/scripts/pahole-version.sh > +++ b/scripts/pahole-version.sh > @@ -5,9 +5,33 @@ > # > # Prints pahole's version in a 3-digit form, such as 119 for v1.19. > > -if [ ! -x "$(command -v "$@")" ]; then > - echo 0 > +set -e > +trap "echo 0; exit 1" EXIT > + > +if ! command -v "$@" >/dev/null; then > + echo >&2 "***" > + echo >&2 "*** pahole '$@' could not be found. pahole will not be used." shellcheck likes to have '$*' instead of '$@', but I can't think of a set of arguments which might cause problems. > + echo >&2 "***" > + exit 1 > +fi > + > +output=$("$@" --version 2>/dev/null) || code=$? > +if [ -n "$code" ]; then > + echo >&2 "***" > + echo >&2 "*** Running '$@' to check the pahole version failed with" > + echo >&2 "*** code $code. pahole will not be used." > + echo >&2 "***" > + exit 1 > +fi > + > +output=$(echo "$output" | sed -nE 's/v([0-9]+)\.([0-9]+)/\1\2/p') I'd rather like to have output=$(echo "$output" | sed -nE 's/v([0-9]+)\.([0-9][0-9])/\1\2/p') here (thus, explicitly check against a two number subversion), so that we can detect also versions like 1.100 or 2.1 and bail out. Kind regards, Nicolas