| Message ID | 20240902160828.1092891-1-ojeda@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] kbuild: pahole-version: improve overall checking and error messages | expand |
On Mon, Sep 2, 2024 at 6:09 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > +if ! command -v "$@" >/dev/null; then > + echo >&2 "***" > + echo >&2 "*** pahole '$@' could not be found. pahole will not be used." > + echo >&2 "***" > + exit 1 > +fi We may not want to print a warning in this case if this case/setup is too common, though. Cheers, Miguel
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@kernel.org/ [1] > Co-developed-by: Nicolas Schier <nicolas@fjasle.eu> > Signed-off-by: Nicolas Schier <nicolas@fjasle.eu> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > --- > v1: https://lore.kernel.org/all/20240728125527.690726-1-ojeda@kernel.org/ > 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
On Mon, Sep 02, 2024 at 06:12:52PM +0200, Miguel Ojeda wrote: > On Mon, Sep 2, 2024 at 6:09 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > > > +if ! command -v "$@" >/dev/null; then > > + echo >&2 "***" > > + echo >&2 "*** pahole '$@' could not be found. pahole will not be used." > > + echo >&2 "***" > > + exit 1 > > +fi > > We may not want to print a warning in this case if this case/setup is > too common, though. yes, I think it's a good idea to skip to remove this one. Kind regards, Nicolas
On Tue, Sep 3, 2024 at 9:49 PM Nicolas Schier <nicolas@fjasle.eu> wrote: > > thanks, no objections. Thanks for taking a look! > 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. So I didn't change that here to avoid more changes in the same commit, but happy to do that if preferred. However, do we want to make it too strict? i.e. I don't think it is very unexpected to get v1.100 or v2.1 -- it may not be what current `pahole` does or ever do, but I am not sure we gain much by being so strict. (Similarly, for the ^..$ suggestion, it could be that `pahole` decides to to something like `pahole v1.25`, i.e. `name version`, like other programs). Either way, I am happy -- I doubt `pahole` changes too much, and if it does, we can change this too. Cheers, Miguel
On Tue, Sep 3, 2024 at 1:09 AM Miguel Ojeda <ojeda@kernel.org> wrote: > > Like patch "rust: suppress error messages from > CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT" [1], Better to point the commit instead of the patch in ML. Like commit 5ce86c6c8613 ("rust: suppress error messages from CONFIG_{RUSTC,BINDGEN}_VERSION_TEXT"), > 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@kernel.org/ [1] > Co-developed-by: Nicolas Schier <nicolas@fjasle.eu> > Signed-off-by: Nicolas Schier <nicolas@fjasle.eu> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > --- > v1: https://lore.kernel.org/all/20240728125527.690726-1-ojeda@kernel.org/ > 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. > > 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." > + 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') > +if [ -z "${output}" ]; then > + echo >&2 "***" > + echo >&2 "*** pahole '$@' returned an unexpected version output." > + echo >&2 "*** pahole will not be used." > + echo >&2 "***" > exit 1 > fi > > -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > +echo "${output}" > +trap EXIT > > base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6 > -- > 2.46.0
On Tue, Sep 3, 2024 at 1:13 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Mon, Sep 2, 2024 at 6:09 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > > > +if ! command -v "$@" >/dev/null; then > > + echo >&2 "***" > > + echo >&2 "*** pahole '$@' could not be found. pahole will not be used." > > + echo >&2 "***" > > + exit 1 > > +fi > > We may not want to print a warning in this case if this case/setup is > too common, though. I am fine with your color (in case someone wants to run it manually?) as long as stderr is suppressed in Kconfig. default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE) 2>/dev/null) scripts/rust_is_available.sh is very verbose, but we are not annoyed while running Kconfig because the $(success ) macro suppressed stderr.
On Wed, Sep 4, 2024 at 9:15 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Tue, Sep 3, 2024 at 9:49 PM Nicolas Schier <nicolas@fjasle.eu> wrote: > > > > thanks, no objections. > > Thanks for taking a look! > > > 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. > > So I didn't change that here to avoid more changes in the same commit, > but happy to do that if preferred. > > However, do we want to make it too strict? i.e. I don't think it is > very unexpected to get v1.100 or v2.1 -- it may not be what current > `pahole` does or ever do, but I am not sure we gain much by being so > strict. I am not sure whether pahole never releases v2.0 $ echo v2.0 | sed -nE 's/v([0-9]+)\.([0-9]+)/\1\2/p' 20 Not a syntax error, but the version comparison will not work correctly. > > (Similarly, for the ^..$ suggestion, it could be that `pahole` decides > to to something like `pahole v1.25`, i.e. `name version`, like other > programs). > > Either way, I am happy -- I doubt `pahole` changes too much, and if it > does, we can change this too. > > Cheers, > Miguel >
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." + 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') +if [ -z "${output}" ]; then + echo >&2 "***" + echo >&2 "*** pahole '$@' returned an unexpected version output." + echo >&2 "*** pahole will not be used." + echo >&2 "***" exit 1 fi -"$@" --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' +echo "${output}" +trap EXIT
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@kernel.org/ [1] Co-developed-by: Nicolas Schier <nicolas@fjasle.eu> Signed-off-by: Nicolas Schier <nicolas@fjasle.eu> Signed-off-by: Miguel Ojeda <ojeda@kernel.org> --- v1: https://lore.kernel.org/all/20240728125527.690726-1-ojeda@kernel.org/ 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. scripts/pahole-version.sh | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6 -- 2.46.0