diff mbox series

[3/6] kbuild: rust_is_available: add check for `bindgen` invocation

Message ID 20230109204520.539080-3-ojeda@kernel.org
State New
Headers
Series [1/6] docs: rust: add paragraph about finding a suitable `libclang` |

Commit Message

Miguel Ojeda Jan. 9, 2023, 8:45 p.m. UTC 
  `scripts/rust_is_available.sh` calls `bindgen` with a special
header in order to check whether the `libclang` version in use
is suitable.

However, the invocation itself may fail if, for instance, `bindgen`
cannot locate `libclang`. This is fine for Kconfig (since the
script will still fail and therefore disable Rust as it should),
but it is pretty confusing for users of the `rustavailable` target
given the error will be unrelated:

    ./scripts/rust_is_available.sh: 21: arithmetic expression: expecting primary: "100000 *  + 100 *  + "
    make: *** [Makefile:1816: rustavailable] Error 2

Instead, run the `bindgen` invocation independently in a previous
step, saving its output and return code. If it fails, then show
the user a proper error message. Otherwise, continue as usual
with the saved output.

Since the previous patch we show a reference to the docs, and
the docs now explain how `bindgen` looks for `libclang`,
thus the error message can leverage the documentation, avoiding
duplication here (and making users aware of the setup guide in
the documentation).

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Reported-by: fvalenduc (@fvalenduc)
Reported-by: Alexandru Radovici <msg4alex@gmail.com>
Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
Link: https://github.com/Rust-for-Linux/linux/issues/934
Link: https://github.com/Rust-for-Linux/linux/pull/921
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 scripts/rust_is_available.sh | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

Boqun Feng Jan. 9, 2023, 10:46 p.m. UTC | #1 
On Mon, Jan 09, 2023 at 09:45:17PM +0100, Miguel Ojeda wrote:
> `scripts/rust_is_available.sh` calls `bindgen` with a special
> header in order to check whether the `libclang` version in use
> is suitable.
> 
> However, the invocation itself may fail if, for instance, `bindgen`
> cannot locate `libclang`. This is fine for Kconfig (since the
> script will still fail and therefore disable Rust as it should),
> but it is pretty confusing for users of the `rustavailable` target
> given the error will be unrelated:
> 
>     ./scripts/rust_is_available.sh: 21: arithmetic expression: expecting primary: "100000 *  + 100 *  + "
>     make: *** [Makefile:1816: rustavailable] Error 2
> 
> Instead, run the `bindgen` invocation independently in a previous
> step, saving its output and return code. If it fails, then show
> the user a proper error message. Otherwise, continue as usual
> with the saved output.
> 
> Since the previous patch we show a reference to the docs, and
> the docs now explain how `bindgen` looks for `libclang`,
> thus the error message can leverage the documentation, avoiding
> duplication here (and making users aware of the setup guide in
> the documentation).
> 
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Reported-by: fvalenduc (@fvalenduc)

Per Documentation/process/maintainer-tip.rst, the "Reported-by" tag does
require "Name <mailaddress>" format. Given we already have the GitHub
issue link, I wonder whether it's better we ask for the reporter's
email address (and real name) for the "Reported-by" field, and if they
prefer to not providing one, we just don't use the "Reported-by" tag
since we still have the GitHub issue link for their contribution.

Thoughts?

Regards,
Boqun

> Reported-by: Alexandru Radovici <msg4alex@gmail.com>
> Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
> Link: https://github.com/Rust-for-Linux/linux/issues/934
> Link: https://github.com/Rust-for-Linux/linux/pull/921
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>  scripts/rust_is_available.sh | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
> index c907cf881c2c..cd87729ca3bf 100755
> --- a/scripts/rust_is_available.sh
> +++ b/scripts/rust_is_available.sh
> @@ -108,8 +108,29 @@ if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers
>  fi
>  
>  # Check that the `libclang` used by the Rust bindings generator is suitable.
> +#
> +# In order to do that, first invoke `bindgen` to get the `libclang` version
> +# found by `bindgen`. This step may already fail if, for instance, `libclang`
> +# is not found, thus inform the user in such a case.
> +set +e
> +bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null)
> +bindgen_libclang_code=$?
> +set -e
> +if [ $bindgen_libclang_code -ne 0 ]; then
> +	echo >&2 "***"
> +	echo >&2 "*** Running '$BINDGEN' to check the libclang version (used by the Rust"
> +	echo >&2 "*** bindings generator) failed with code $bindgen_libclang_code. This may be caused by"
> +	echo >&2 "*** a failure to locate libclang. See output and docs below for details:"
> +	echo >&2 "***"
> +	echo >&2 "$bindgen_libclang_output"
> +	echo >&2 "***"
> +	exit 1
> +fi
> +
> +# `bindgen` returned successfully, thus use the output to check that the version
> +# of the `libclang` found by the Rust bindings generator is suitable.
>  bindgen_libclang_version=$( \
> -	LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null \
> +	echo "$bindgen_libclang_output" \
>  		| grep -F 'clang version ' \
>  		| grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
>  		| head -n 1 \
> -- 
> 2.39.0
>
Miguel Ojeda Jan. 9, 2023, 11:27 p.m. UTC | #2 
On Mon, Jan 9, 2023 at 11:47 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Per Documentation/process/maintainer-tip.rst, the "Reported-by" tag does
> require "Name <mailaddress>" format. Given we already have the GitHub
> issue link, I wonder whether it's better we ask for the reporter's
> email address (and real name) for the "Reported-by" field, and if they
> prefer to not providing one, we just don't use the "Reported-by" tag
> since we still have the GitHub issue link for their contribution.
>
> Thoughts?

As far as I understand, that is for the tip tree (though
`checkpatch.pl` complained too), and I am not sure in that guide they
intend it to mean it is the only form accepted.

In this case, I ended up deciding to add it since it was not a
Signed-off-by/Reviewed-by/Acked-by (so not as critical, i.e. no
DCO/RSO/...) and there are quite a few other instances, including
different CIs and tools, raw emails, security teams, etc.

So it doesn't look like it is required to be a "real name" like some
of the other tags, and sometimes we may need to do otherwise anyway
(for those cases), and I guess we don't want to discourage reports too
much. Perhaps we can, at least, ask for an email address -- that is
way more common in the log, and gives us a potential way to contact
people and send the patches to.

In any case, I agree we should prefer the "real name" way as much as
possible. I had sent a message to each GitHub issue/PR linking back to
the patches, but I will send another to offer them to use their real
name if they prefer.

Thanks for taking a look! :)

Cheers,
Miguel
Masahiro Yamada Jan. 12, 2023, 4:33 a.m. UTC | #3 
On Tue, Jan 10, 2023 at 5:45 AM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> `scripts/rust_is_available.sh` calls `bindgen` with a special
> header in order to check whether the `libclang` version in use
> is suitable.
>
> However, the invocation itself may fail if, for instance, `bindgen`
> cannot locate `libclang`. This is fine for Kconfig (since the
> script will still fail and therefore disable Rust as it should),
> but it is pretty confusing for users of the `rustavailable` target
> given the error will be unrelated:
>
>     ./scripts/rust_is_available.sh: 21: arithmetic expression: expecting primary: "100000 *  + 100 *  + "
>     make: *** [Makefile:1816: rustavailable] Error 2
>
> Instead, run the `bindgen` invocation independently in a previous
> step, saving its output and return code. If it fails, then show
> the user a proper error message. Otherwise, continue as usual
> with the saved output.
>
> Since the previous patch we show a reference to the docs, and
> the docs now explain how `bindgen` looks for `libclang`,
> thus the error message can leverage the documentation, avoiding
> duplication here (and making users aware of the setup guide in
> the documentation).
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Reported-by: fvalenduc (@fvalenduc)
> Reported-by: Alexandru Radovici <msg4alex@gmail.com>
> Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
> Link: https://github.com/Rust-for-Linux/linux/issues/934
> Link: https://github.com/Rust-for-Linux/linux/pull/921
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>  scripts/rust_is_available.sh | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
> index c907cf881c2c..cd87729ca3bf 100755
> --- a/scripts/rust_is_available.sh
> +++ b/scripts/rust_is_available.sh
> @@ -108,8 +108,29 @@ if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers
>  fi
>
>  # Check that the `libclang` used by the Rust bindings generator is suitable.
> +#
> +# In order to do that, first invoke `bindgen` to get the `libclang` version
> +# found by `bindgen`. This step may already fail if, for instance, `libclang`
> +# is not found, thus inform the user in such a case.
> +set +e
> +bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null)
> +bindgen_libclang_code=$?
> +set -e
> +if [ $bindgen_libclang_code -ne 0 ]; then
> +       echo >&2 "***"
> +       echo >&2 "*** Running '$BINDGEN' to check the libclang version (used by the Rust"
> +       echo >&2 "*** bindings generator) failed with code $bindgen_libclang_code. This may be caused by"
> +       echo >&2 "*** a failure to locate libclang. See output and docs below for details:"
> +       echo >&2 "***"
> +       echo >&2 "$bindgen_libclang_output"
> +       echo >&2 "***"
> +       exit 1
> +fi
>


Instead of toggling -e, why don't you write like this?


if ! bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname
$0)/rust_is_available_bindgen_libclang.h 2>&1); then
       [snip]
fi







--
Best Regards
Masahiro Yamada
Masahiro Yamada Jan. 12, 2023, 4:35 a.m. UTC | #4 
On Thu, Jan 12, 2023 at 1:33 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Jan 10, 2023 at 5:45 AM Miguel Ojeda <ojeda@kernel.org> wrote:
> >
> > `scripts/rust_is_available.sh` calls `bindgen` with a special
> > header in order to check whether the `libclang` version in use
> > is suitable.
> >
> > However, the invocation itself may fail if, for instance, `bindgen`
> > cannot locate `libclang`. This is fine for Kconfig (since the
> > script will still fail and therefore disable Rust as it should),
> > but it is pretty confusing for users of the `rustavailable` target
> > given the error will be unrelated:
> >
> >     ./scripts/rust_is_available.sh: 21: arithmetic expression: expecting primary: "100000 *  + 100 *  + "
> >     make: *** [Makefile:1816: rustavailable] Error 2
> >
> > Instead, run the `bindgen` invocation independently in a previous
> > step, saving its output and return code. If it fails, then show
> > the user a proper error message. Otherwise, continue as usual
> > with the saved output.
> >
> > Since the previous patch we show a reference to the docs, and
> > the docs now explain how `bindgen` looks for `libclang`,
> > thus the error message can leverage the documentation, avoiding
> > duplication here (and making users aware of the setup guide in
> > the documentation).
> >
> > Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> > Reported-by: fvalenduc (@fvalenduc)
> > Reported-by: Alexandru Radovici <msg4alex@gmail.com>
> > Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
> > Link: https://github.com/Rust-for-Linux/linux/issues/934
> > Link: https://github.com/Rust-for-Linux/linux/pull/921
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > ---
> >  scripts/rust_is_available.sh | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
> > index c907cf881c2c..cd87729ca3bf 100755
> > --- a/scripts/rust_is_available.sh
> > +++ b/scripts/rust_is_available.sh
> > @@ -108,8 +108,29 @@ if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers
> >  fi
> >
> >  # Check that the `libclang` used by the Rust bindings generator is suitable.
> > +#
> > +# In order to do that, first invoke `bindgen` to get the `libclang` version
> > +# found by `bindgen`. This step may already fail if, for instance, `libclang`
> > +# is not found, thus inform the user in such a case.
> > +set +e
> > +bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null)
> > +bindgen_libclang_code=$?
> > +set -e
> > +if [ $bindgen_libclang_code -ne 0 ]; then
> > +       echo >&2 "***"
> > +       echo >&2 "*** Running '$BINDGEN' to check the libclang version (used by the Rust"
> > +       echo >&2 "*** bindings generator) failed with code $bindgen_libclang_code. This may be caused by"
> > +       echo >&2 "*** a failure to locate libclang. See output and docs below for details:"
> > +       echo >&2 "***"
> > +       echo >&2 "$bindgen_libclang_output"
> > +       echo >&2 "***"
> > +       exit 1
> > +fi
> >
>
>
> Instead of toggling -e, why don't you write like this?
>
>
> if ! bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname
> $0)/rust_is_available_bindgen_libclang.h 2>&1); then
>        [snip]
> fi
>


I meant this:



if ! bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname
$0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null); then
       [snip]
fi




(">/dev/null" was lost in the previous email)
Miguel Ojeda Jan. 13, 2023, 11:10 p.m. UTC | #5 
On Thu, Jan 12, 2023 at 5:35 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> I meant this:
>
> if ! bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname
> $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null); then
>        [snip]
> fi
>
> (">/dev/null" was lost in the previous email)

I used the error code in the message below. I am happy either way.

Cheers,
Miguel
Masahiro Yamada Jan. 14, 2023, 9:44 a.m. UTC | #6 
On Sat, Jan 14, 2023 at 8:10 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Jan 12, 2023 at 5:35 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > I meant this:
> >
> > if ! bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname
> > $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null); then
> >        [snip]
> > fi
> >
> > (">/dev/null" was lost in the previous email)
>
> I used the error code in the message below. I am happy either way.
>
> Cheers,
> Miguel


Ah, I see.



How about this?




bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname
$0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null) \
         || bindgen_libclang_code=$?

if [ -n "$bindgen_libclang_code" ]; then
       echo >&2 "***"
       echo >&2 "*** Running '$BINDGEN' to check the libclang version
(used by the Rust"
       echo >&2 "*** bindings generator) failed with code
$bindgen_libclang_code. This may be caused by"
       echo >&2 "*** a failure to locate libclang. See output and docs
below for details:"
       echo >&2 "***"
       echo >&2 "$bindgen_libclang_output"
       echo >&2 "***"
       exit 1
fi





You can get the error code of bindgen without toggling -e.
Miguel Ojeda Jan. 14, 2023, 12:11 p.m. UTC | #7 
On Sat, Jan 14, 2023 at 10:44 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Ah, I see.
>
> How about this?
>
> bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname
> $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null) \
>          || bindgen_libclang_code=$?
>
> You can get the error code of bindgen without toggling -e.

As you prefer -- personally I tend to avoid assigning two variables in
a single "statement" (like in C), but I am also happy avoiding to
toggle `-e` since it is global state and therefore ugly too anyway.

Cheers,
Miguel
Miguel Ojeda Jan. 14, 2023, 12:12 p.m. UTC | #8 
On Mon, Jan 9, 2023 at 9:45 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Reported-by: fvalenduc (@fvalenduc)

Cc'ing François who gave emailed me his name and address, thus a
better tag can be written here for v2.

Cheers,
Miguel
diff mbox series

Patch

diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index c907cf881c2c..cd87729ca3bf 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -108,8 +108,29 @@  if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers
 fi
 
 # Check that the `libclang` used by the Rust bindings generator is suitable.
+#
+# In order to do that, first invoke `bindgen` to get the `libclang` version
+# found by `bindgen`. This step may already fail if, for instance, `libclang`
+# is not found, thus inform the user in such a case.
+set +e
+bindgen_libclang_output=$(LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null)
+bindgen_libclang_code=$?
+set -e
+if [ $bindgen_libclang_code -ne 0 ]; then
+	echo >&2 "***"
+	echo >&2 "*** Running '$BINDGEN' to check the libclang version (used by the Rust"
+	echo >&2 "*** bindings generator) failed with code $bindgen_libclang_code. This may be caused by"
+	echo >&2 "*** a failure to locate libclang. See output and docs below for details:"
+	echo >&2 "***"
+	echo >&2 "$bindgen_libclang_output"
+	echo >&2 "***"
+	exit 1
+fi
+
+# `bindgen` returned successfully, thus use the output to check that the version
+# of the `libclang` found by the Rust bindings generator is suitable.
 bindgen_libclang_version=$( \
-	LC_ALL=C "$BINDGEN" $(dirname $0)/rust_is_available_bindgen_libclang.h 2>&1 >/dev/null \
+	echo "$bindgen_libclang_output" \
 		| grep -F 'clang version ' \
 		| grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
 		| head -n 1 \