Sometimes users need to tweak the finding process of `libclang`
for `bindgen` via the `clang-sys`-provided environment variables.
Thus add a paragraph to the setting up guide, including a reference
to `clang-sys`'s relevant documentation.
Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
Signed-off-by: Miguel Ojeda <[email protected]>
---
Documentation/rust/quick-start.rst | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
index 13b7744b1e27..cae21ea7de41 100644
--- a/Documentation/rust/quick-start.rst
+++ b/Documentation/rust/quick-start.rst
@@ -100,6 +100,23 @@ Install it via (note that this will download and build the tool from source)::
cargo install --locked --version $(scripts/min-tool-version.sh bindgen) bindgen
+``bindgen`` needs to find a suitable ``libclang`` in order to work. If it is
+not found (or a different ``libclang`` than the one found should be used),
+the process can be tweaked using the environment variables understood by
+``clang-sys`` (the Rust bindings crate that ``bindgen`` uses to access
+``libclang``):
+
+* ``LLVM_CONFIG_PATH`` can be pointed to an ``llvm-config`` executable.
+
+* Or ``LIBCLANG_PATH`` can be pointed to a ``libclang`` shared library
+ or to the directoy containing it.
+
+* Or ``CLANG_PATH`` can be pointed to a ``clang`` executable.
+
+For details, please see ``clang-sys``'s documentation at:
+
+ https://github.com/KyleMayes/clang-sys#environment-variables
+
Requirements: Developing
------------------------
--
2.39.0
Sometimes [1] users may attempt to setup the Rust support by
checking what Kbuild does and they end up finding out about
`scripts/rust_is_available.sh`. Inevitably, they run the script
directly, but unless they setup the required variables,
the result of the script is not meaningful.
We could add some defaults to the variables, but that could be
confusing for those that may override the defaults (compared
to their kernel builds), and `$CC` would not be a simple default
in any case.
Therefore, instead, print a warning when the script detects
the user may be invoking it, by testing `$MAKEFLAGS`.
Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/ [1]
Signed-off-by: Miguel Ojeda <[email protected]>
---
scripts/rust_is_available.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index cd87729ca3bf..0c082a248f15 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -35,6 +35,16 @@ print_docs_reference()
warning=0
trap 'if [ $? -ne 0 ] || [ $warning -ne 0 ]; then print_docs_reference; fi' EXIT
+# Check whether the script was invoked from Kbuild.
+if [ -z "${MAKEFLAGS+x}" ]; then
+ echo >&2 "***"
+ echo >&2 "*** This script is intended to be called from Kbuild."
+ echo >&2 "*** Please use the 'rustavailable' target to call it instead."
+ echo >&2 "*** Otherwise, the results may not be meaningful."
+ echo >&2 "***"
+ warning=1
+fi
+
# Check that the Rust compiler exists.
if ! command -v "$RUSTC" >/dev/null; then
echo >&2 "***"
--
2.39.0
`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 <[email protected]>
Reported-by: fvalenduc (@fvalenduc)
Reported-by: Alexandru Radovici <[email protected]>
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 <[email protected]>
---
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
People trying out the Rust support in the kernel may get
warnings and errors from `scripts/rust_is_available.sh`
from the `rustavailable` target or the build step.
Some of those users may be following the Quick Start guide,
but others may not (likely those getting warnings from
the build step instead of the target).
While the messages are fairly clear on what the problem is,
it may not be clear how to solve the particular issue,
especially for those not aware of the documentation.
We could add all sorts of details on the script for each one,
but it is better to point users to the documentation instead,
where it is easily readable in different formats. It also
avoids duplication.
Thus add a reference to the documentation whenever the script
fails or there is at least a warning.
Signed-off-by: Miguel Ojeda <[email protected]>
---
Note that is based on top of patch "kbuild: rust: remove -v
option of scripts/rust_is_available.sh" applied on v6.2-rc3:
https://lore.kernel.org/rust-for-linux/[email protected]/
scripts/rust_is_available.sh | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index eaeafebf8572..c907cf881c2c 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -21,6 +21,20 @@ get_canonical_version()
echo $((100000 * $1 + 100 * $2 + $3))
}
+# Print a reference to the setup guide in the documentation.
+print_docs_reference()
+{
+ echo >&2 "***"
+ echo >&2 "*** Please see Documentation/rust/quick-start.rst for details"
+ echo >&2 "*** on how to setup Rust support."
+ echo >&2 "***"
+}
+
+# If the script fails for any reason, or if there was any warning, then
+# print a reference to the documentation on exit.
+warning=0
+trap 'if [ $? -ne 0 ] || [ $warning -ne 0 ]; then print_docs_reference; fi' EXIT
+
# Check that the Rust compiler exists.
if ! command -v "$RUSTC" >/dev/null; then
echo >&2 "***"
@@ -62,6 +76,7 @@ if [ "$rust_compiler_cversion" -gt "$rust_compiler_min_cversion" ]; then
echo >&2 "*** Your version: $rust_compiler_version"
echo >&2 "*** Expected version: $rust_compiler_min_version"
echo >&2 "***"
+ warning=1
fi
# Check that the Rust bindings generator is suitable.
@@ -89,6 +104,7 @@ if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers
echo >&2 "*** Your version: $rust_bindings_generator_version"
echo >&2 "*** Expected version: $rust_bindings_generator_min_version"
echo >&2 "***"
+ warning=1
fi
# Check that the `libclang` used by the Rust bindings generator is suitable.
@@ -128,6 +144,7 @@ if [ "$cc_name" = Clang ]; then
echo >&2 "*** libclang version: $bindgen_libclang_version"
echo >&2 "*** Clang version: $clang_version"
echo >&2 "***"
+ warning=1
fi
fi
--
2.39.0
On Mon, Jan 9, 2023 at 9:54 PM Nick Desaulniers <[email protected]> wrote:
>
> This is super helpful for me, since I build clang from source and
> would like to use my libclang.so! Thanks for this documentation
> Miguel!
> Reviewed-by: Nick Desaulniers <[email protected]>
Thanks for the quick review Nick!
By the way, I didn't add your Reported-by here because apparently it
is only intended for bug fixes and not features.
Cheers,
Miguel
In order to match the version string, `sed` is used in a couple
cases, and `grep` and `head` in a couple others.
Make the script more consistent and easier to understand by
using the same method, `sed`, for all of them.
This makes the version matching also a bit more strict for
the changed cases, since the strings `rustc ` and `bindgen `
will now be required, which should be fine since `rustc`
complains if one attempts to call it with another program
name, and `bindgen` uses a hardcoded string.
In addition, clarify why one of the existing `sed` commands
does not provide an address like the others.
Signed-off-by: Miguel Ojeda <[email protected]>
---
scripts/rust_is_available.sh | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index a86659410e48..99811842b61f 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -66,8 +66,7 @@ fi
# Non-stable and distributions' versions may have a version suffix, e.g. `-dev`.
rust_compiler_version=$( \
LC_ALL=C "$RUSTC" --version 2>/dev/null \
- | head -n 1 \
- | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
+ | sed -nE '1s:.*rustc ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
)
rust_compiler_min_version=$($min_tool_version rustc)
rust_compiler_cversion=$(get_canonical_version $rust_compiler_version)
@@ -94,8 +93,7 @@ fi
# Non-stable and distributions' versions may have a version suffix, e.g. `-dev`.
rust_bindings_generator_version=$( \
LC_ALL=C "$BINDGEN" --version 2>/dev/null \
- | head -n 1 \
- | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
+ | sed -nE '1s:.*bindgen ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
)
rust_bindings_generator_min_version=$($min_tool_version bindgen)
rust_bindings_generator_cversion=$(get_canonical_version $rust_bindings_generator_version)
@@ -139,6 +137,9 @@ fi
# `bindgen` returned successfully, thus use the output to check that the version
# of the `libclang` found by the Rust bindings generator is suitable.
+#
+# Unlike other version checks, note that this one does not necessarily appear
+# in the first line of the output, thus no `sed` address is provided.
bindgen_libclang_version=$( \
echo "$bindgen_libclang_output" \
| sed -nE 's:.*clang version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
--
2.39.0
`bindgen`'s output for `libclang`'s version check contains paths, which
in turn may contain strings that look like version numbers [1]:
.../6.1.0-dev/.../rust_is_available_bindgen_libclang.h:2:9: warning: clang version 11.1.0 [-W#pragma-messages], err: false
which the script will pick up as the version instead of the latter.
It is also the case that versions may appear after the actual version
(e.g. distribution's version text), which was the reason behind `head` [2]:
.../rust-is-available-bindgen-libclang.h:2:9: warning: clang version 13.0.0 (Fedora 13.0.0-3.fc35) [-W#pragma-messages], err: false
Thus instead ask for a match after the `clang version` string.
Reported-by: Jordan (@jordanisaacs)
Link: https://github.com/Rust-for-Linux/linux/issues/942 [1]
Link: https://github.com/Rust-for-Linux/linux/pull/789 [2]
Signed-off-by: Miguel Ojeda <[email protected]>
---
scripts/rust_is_available.sh | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index 0c082a248f15..a86659410e48 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -141,9 +141,7 @@ fi
# of the `libclang` found by the Rust bindings generator is suitable.
bindgen_libclang_version=$( \
echo "$bindgen_libclang_output" \
- | grep -F 'clang version ' \
- | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
- | head -n 1 \
+ | sed -nE 's:.*clang version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
)
bindgen_libclang_min_version=$($min_tool_version llvm)
bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version)
--
2.39.0
On Mon, Jan 9, 2023 at 12:45 PM Miguel Ojeda <[email protected]> wrote:
>
> Sometimes users need to tweak the finding process of `libclang`
> for `bindgen` via the `clang-sys`-provided environment variables.
>
> Thus add a paragraph to the setting up guide, including a reference
> to `clang-sys`'s relevant documentation.
>
> Link: https://lore.kernel.org/rust-for-linux/CAKwvOdm5JT4wbdQQYuW+RT07rCi6whGBM2iUAyg8A1CmLXG6Nw@mail.gmail.com/
> Signed-off-by: Miguel Ojeda <[email protected]>
This is super helpful for me, since I build clang from source and
would like to use my libclang.so! Thanks for this documentation
Miguel!
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
> Documentation/rust/quick-start.rst | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
> index 13b7744b1e27..cae21ea7de41 100644
> --- a/Documentation/rust/quick-start.rst
> +++ b/Documentation/rust/quick-start.rst
> @@ -100,6 +100,23 @@ Install it via (note that this will download and build the tool from source)::
>
> cargo install --locked --version $(scripts/min-tool-version.sh bindgen) bindgen
>
> +``bindgen`` needs to find a suitable ``libclang`` in order to work. If it is
> +not found (or a different ``libclang`` than the one found should be used),
> +the process can be tweaked using the environment variables understood by
> +``clang-sys`` (the Rust bindings crate that ``bindgen`` uses to access
> +``libclang``):
> +
> +* ``LLVM_CONFIG_PATH`` can be pointed to an ``llvm-config`` executable.
> +
> +* Or ``LIBCLANG_PATH`` can be pointed to a ``libclang`` shared library
> + or to the directoy containing it.
> +
> +* Or ``CLANG_PATH`` can be pointed to a ``clang`` executable.
> +
> +For details, please see ``clang-sys``'s documentation at:
> +
> + https://github.com/KyleMayes/clang-sys#environment-variables
> +
>
> Requirements: Developing
> ------------------------
> --
> 2.39.0
>
--
Thanks,
~Nick Desaulniers
On Mon, Jan 9, 2023 at 9:45 PM Miguel Ojeda <[email protected]> wrote:
>
> +* Or ``LIBCLANG_PATH`` can be pointed to a ``libclang`` shared library
> + or to the directoy containing it.
I just noticed the typo here, sorry: directoy -> directory
Masahiro: if you take them, please feel free to correct it.
Cheers,
Miguel
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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
On Mon, Jan 9, 2023 at 11:47 PM Boqun Feng <[email protected]> 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
On 9 Jan 2023, at 21:45, Miguel Ojeda wrote:
> People trying out the Rust support in the kernel may get
> warnings and errors from `scripts/rust_is_available.sh`
> from the `rustavailable` target or the build step.
>
> Some of those users may be following the Quick Start guide,
> but others may not (likely those getting warnings from
> the build step instead of the target).
>
> While the messages are fairly clear on what the problem is,
> it may not be clear how to solve the particular issue,
> especially for those not aware of the documentation.
>
> We could add all sorts of details on the script for each one,
> but it is better to point users to the documentation instead,
> where it is easily readable in different formats. It also
> avoids duplication.
>
> Thus add a reference to the documentation whenever the script
> fails or there is at least a warning.
As I always use my systems rustc/bindgen, I always get the warning, which already clutters the build output a bit. But I see why it is helpful, so not a fan, but this patch is reasonable.
>
> Signed-off-by: Miguel Ojeda <[email protected]>
Reviewed-by: Finn Behrens <[email protected]>
Regards,
Finn
> ---
> Note that is based on top of patch "kbuild: rust: remove -v
> option of scripts/rust_is_available.sh" applied on v6.2-rc3:
> https://lore.kernel.org/rust-for-linux/[email protected]/
>
> scripts/rust_is_available.sh | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
> index eaeafebf8572..c907cf881c2c 100755
> --- a/scripts/rust_is_available.sh
> +++ b/scripts/rust_is_available.sh
> @@ -21,6 +21,20 @@ get_canonical_version()
> echo $((100000 * $1 + 100 * $2 + $3))
> }
>
> +# Print a reference to the setup guide in the documentation.
> +print_docs_reference()
> +{
> + echo >&2 "***"
> + echo >&2 "*** Please see Documentation/rust/quick-start.rst for details"
> + echo >&2 "*** on how to setup Rust support."
> + echo >&2 "***"
> +}
> +
> +# If the script fails for any reason, or if there was any warning, then
> +# print a reference to the documentation on exit.
> +warning=0
> +trap 'if [ $? -ne 0 ] || [ $warning -ne 0 ]; then print_docs_reference; fi' EXIT
> +
> # Check that the Rust compiler exists.
> if ! command -v "$RUSTC" >/dev/null; then
> echo >&2 "***"
> @@ -62,6 +76,7 @@ if [ "$rust_compiler_cversion" -gt "$rust_compiler_min_cversion" ]; then
> echo >&2 "*** Your version: $rust_compiler_version"
> echo >&2 "*** Expected version: $rust_compiler_min_version"
> echo >&2 "***"
> + warning=1
> fi
>
> # Check that the Rust bindings generator is suitable.
> @@ -89,6 +104,7 @@ if [ "$rust_bindings_generator_cversion" -gt "$rust_bindings_generator_min_cvers
> echo >&2 "*** Your version: $rust_bindings_generator_version"
> echo >&2 "*** Expected version: $rust_bindings_generator_min_version"
> echo >&2 "***"
> + warning=1
> fi
>
> # Check that the `libclang` used by the Rust bindings generator is suitable.
> @@ -128,6 +144,7 @@ if [ "$cc_name" = Clang ]; then
> echo >&2 "*** libclang version: $bindgen_libclang_version"
> echo >&2 "*** Clang version: $clang_version"
> echo >&2 "***"
> + warning=1
> fi
> fi
>
> --
> 2.39.0
On Tue, Jan 10, 2023 at 11:16 AM Finn Behrens <[email protected]> wrote:
>
> As I always use my systems rustc/bindgen, I always get the warning, which already clutters the build output a bit. But I see why it is helpful, so not a fan, but this patch is reasonable.
Indeed, if one uses a different version, it may end up becoming too
annoying when running it during build -- it is something I worried
about when adding it back then in commit 11c0cf1e8c06 ("rust: run
rust-is-available on build") in our repository.
I think, for a while, until more people is accustomed to dealing with
Rust, it may be worth the pain for some of us in order to help to
catch bad setups, since otherwise users may not attempt to check with
the `rustavailable` target themselves.
In any case, of course, the "too new" warnings will go away when we
reach a stable version situation since they will not be needed anymore
(but we can also do it sooner than that, for the build step
especially).
Thanks for the review!
Cheers,
Miguel
On Tue, Jan 10, 2023 at 5:45 AM Miguel Ojeda <[email protected]> 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 <[email protected]>
> Reported-by: fvalenduc (@fvalenduc)
> Reported-by: Alexandru Radovici <[email protected]>
> 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 <[email protected]>
> ---
> 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
On Thu, Jan 12, 2023 at 1:33 PM Masahiro Yamada <[email protected]> wrote:
>
> On Tue, Jan 10, 2023 at 5:45 AM Miguel Ojeda <[email protected]> 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 <[email protected]>
> > Reported-by: fvalenduc (@fvalenduc)
> > Reported-by: Alexandru Radovici <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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)
--
Best Regards
Masahiro Yamada
On Tue, Jan 10, 2023 at 5:46 AM Miguel Ojeda <[email protected]> wrote:
>
> `bindgen`'s output for `libclang`'s version check contains paths, which
> in turn may contain strings that look like version numbers [1]:
>
> .../6.1.0-dev/.../rust_is_available_bindgen_libclang.h:2:9: warning: clang version 11.1.0 [-W#pragma-messages], err: false
>
> which the script will pick up as the version instead of the latter.
>
> It is also the case that versions may appear after the actual version
> (e.g. distribution's version text), which was the reason behind `head` [2]:
>
> .../rust-is-available-bindgen-libclang.h:2:9: warning: clang version 13.0.0 (Fedora 13.0.0-3.fc35) [-W#pragma-messages], err: false
>
> Thus instead ask for a match after the `clang version` string.
>
> Reported-by: Jordan (@jordanisaacs)
> Link: https://github.com/Rust-for-Linux/linux/issues/942 [1]
> Link: https://github.com/Rust-for-Linux/linux/pull/789 [2]
> Signed-off-by: Miguel Ojeda <[email protected]>
> ---
> scripts/rust_is_available.sh | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
> index 0c082a248f15..a86659410e48 100755
> --- a/scripts/rust_is_available.sh
> +++ b/scripts/rust_is_available.sh
> @@ -141,9 +141,7 @@ fi
> # of the `libclang` found by the Rust bindings generator is suitable.
> bindgen_libclang_version=$( \
> echo "$bindgen_libclang_output" \
> - | grep -F 'clang version ' \
> - | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
> - | head -n 1 \
> + | sed -nE 's:.*clang version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
> )
> bindgen_libclang_min_version=$($min_tool_version llvm)
> bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version)
> --
> 2.39.0
>
You do not need to fork sed.
diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index 1f478d7e0f77..ebe427e27379 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -137,14 +137,9 @@ 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=$( \
- echo "$bindgen_libclang_output" \
- | grep -F 'clang version ' \
- | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
- | head -n 1 \
-)
+set -- ${bindgen_libclang_output#**clang version}
+bindgen_libclang_cversion=$(get_canonical_version $1)
bindgen_libclang_min_version=$($min_tool_version llvm)
-bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version)
bindgen_libclang_min_cversion=$(get_canonical_version
$bindgen_libclang_min_version)
if [ "$bindgen_libclang_cversion" -lt "$bindgen_libclang_min_cversion" ]; then
echo >&2 "***"
--
Best Regards
Masahiro Yamada
On Tue, Jan 10, 2023 at 6:06 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Mon, Jan 9, 2023 at 9:45 PM Miguel Ojeda <[email protected]> wrote:
> >
> > +* Or ``LIBCLANG_PATH`` can be pointed to a ``libclang`` shared library
> > + or to the directoy containing it.
>
> I just noticed the typo here, sorry: directoy -> directory
>
> Masahiro: if you take them, please feel free to correct it.
Yes, I can take this, but the doc change
is independent of the rest, and will not conflict with
any Kbuild changes.
So, you can apply this one to your tree.
>
> Cheers,
> Miguel
--
Best Regards
Masahiro Yamada
On Tue, Jan 10, 2023 at 5:46 AM Miguel Ojeda <[email protected]> wrote:
>
> Sometimes [1] users may attempt to setup the Rust support by
> checking what Kbuild does and they end up finding out about
> `scripts/rust_is_available.sh`. Inevitably, they run the script
> directly, but unless they setup the required variables,
> the result of the script is not meaningful.
>
> We could add some defaults to the variables, but that could be
> confusing for those that may override the defaults (compared
> to their kernel builds), and `$CC` would not be a simple default
> in any case.
>
> Therefore, instead, print a warning when the script detects
> the user may be invoking it, by testing `$MAKEFLAGS`.
>
> Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/ [1]
> Signed-off-by: Miguel Ojeda <[email protected]>
> ---
> scripts/rust_is_available.sh | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
> index cd87729ca3bf..0c082a248f15 100755
> --- a/scripts/rust_is_available.sh
> +++ b/scripts/rust_is_available.sh
> @@ -35,6 +35,16 @@ print_docs_reference()
> warning=0
> trap 'if [ $? -ne 0 ] || [ $warning -ne 0 ]; then print_docs_reference; fi' EXIT
>
> +# Check whether the script was invoked from Kbuild.
> +if [ -z "${MAKEFLAGS+x}" ]; then
> + echo >&2 "***"
> + echo >&2 "*** This script is intended to be called from Kbuild."
> + echo >&2 "*** Please use the 'rustavailable' target to call it instead."
> + echo >&2 "*** Otherwise, the results may not be meaningful."
> + echo >&2 "***"
> + warning=1
> +fi
I do not like this.
We do not need to cater to every oddity.
Checking MAKEFLAGS is too much.
You can check RUSTC/BINDGEN/CC if you persist in this.
diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index a6f84dd2f71c..524aee03384a 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -34,7 +34,7 @@ warning=0
trap 'if [ $? -ne 0 ] || [ $warning -ne 0 ]; then
print_docs_reference; fi' EXIT
# Check that the Rust compiler exists.
-if ! command -v "$RUSTC" >/dev/null; then
+if ! command -v "${RUSTC:?RUSTC is not set}" >/dev/null; then
echo >&2 "***"
echo >&2 "*** Rust compiler '$RUSTC' could not be found."
echo >&2 "***"
@@ -42,7 +42,7 @@ if ! command -v "$RUSTC" >/dev/null; then
fi
# Check that the Rust bindings generator exists.
-if ! command -v "$BINDGEN" >/dev/null; then
+if ! command -v "${BINDGEN:?BINDGEN is not set}" >/dev/null; then
echo >&2 "***"
echo >&2 "*** Rust bindings generator '$BINDGEN' could not be found."
echo >&2 "***"
@@ -150,7 +150,7 @@ fi
#
# In the future, we might be able to perform a full version check, see
# https://github.com/rust-lang/rust-bindgen/issues/2138.
-cc_name=$($(dirname $0)/cc-version.sh "$CC" | cut -f1 -d' ')
+cc_name=$($(dirname $0)/cc-version.sh ${CC:?CC is not set} | cut -f1 -d' ')
if [ "$cc_name" = Clang ]; then
clang_version=$( \
LC_ALL=C "$CC" --version 2>/dev/null \
--
Best Regards
Masahiro Yamada
On Tue, Jan 10, 2023 at 5:46 AM Miguel Ojeda <[email protected]> wrote:
>
> In order to match the version string, `sed` is used in a couple
> cases, and `grep` and `head` in a couple others.
>
> Make the script more consistent and easier to understand by
> using the same method, `sed`, for all of them.
>
> This makes the version matching also a bit more strict for
> the changed cases, since the strings `rustc ` and `bindgen `
> will now be required, which should be fine since `rustc`
> complains if one attempts to call it with another program
> name, and `bindgen` uses a hardcoded string.
>
> In addition, clarify why one of the existing `sed` commands
> does not provide an address like the others.
>
> Signed-off-by: Miguel Ojeda <[email protected]>
Maybe, your purpose is to use sed consistently, but
perhaps you can avoid forking sed if you know the
format of the first line.
BTW, what is missing here is, you do not check if
${RUSTC} is really rustc.
I can fool this script to print
"arithmetic expression: expecting primary: "100000 * + 100 * + "
$ make RUSTC=true rustavailable
./scripts/rust_is_available.sh: 19: arithmetic expression: expecting
primary: "100000 * + 100 * + "
***
*** Please see Documentation/rust/quick-start.rst for details
*** on how to setup Rust support.
***
make: *** [Makefile:1809: rustavailable] Error 2
scripts/{as,ld}-version.sh tried their best to
parse the line with shell syntax only, and
print "unknown assembler invoked" if the given
tool does not seem to be a supported assembler.
> ---
> scripts/rust_is_available.sh | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
> index a86659410e48..99811842b61f 100755
> --- a/scripts/rust_is_available.sh
> +++ b/scripts/rust_is_available.sh
> @@ -66,8 +66,7 @@ fi
> # Non-stable and distributions' versions may have a version suffix, e.g. `-dev`.
> rust_compiler_version=$( \
> LC_ALL=C "$RUSTC" --version 2>/dev/null \
> - | head -n 1 \
> - | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
> + | sed -nE '1s:.*rustc ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
> )
> rust_compiler_min_version=$($min_tool_version rustc)
> rust_compiler_cversion=$(get_canonical_version $rust_compiler_version)
> @@ -94,8 +93,7 @@ fi
> # Non-stable and distributions' versions may have a version suffix, e.g. `-dev`.
> rust_bindings_generator_version=$( \
> LC_ALL=C "$BINDGEN" --version 2>/dev/null \
> - | head -n 1 \
> - | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
> + | sed -nE '1s:.*bindgen ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
> )
> rust_bindings_generator_min_version=$($min_tool_version bindgen)
> rust_bindings_generator_cversion=$(get_canonical_version $rust_bindings_generator_version)
> @@ -139,6 +137,9 @@ fi
>
> # `bindgen` returned successfully, thus use the output to check that the version
> # of the `libclang` found by the Rust bindings generator is suitable.
> +#
> +# Unlike other version checks, note that this one does not necessarily appear
> +# in the first line of the output, thus no `sed` address is provided.
> bindgen_libclang_version=$( \
> echo "$bindgen_libclang_output" \
> | sed -nE 's:.*clang version ([0-9]+\.[0-9]+\.[0-9]+).*:\1:p'
> --
> 2.39.0
>
--
Best Regards
Masahiro Yamada
On Thu, Jan 12, 2023 at 6:32 AM Masahiro Yamada <[email protected]> wrote:
>
> +set -- ${bindgen_libclang_output#**clang version}
> +bindgen_libclang_cversion=$(get_canonical_version $1)
> bindgen_libclang_min_version=$($min_tool_version llvm)
> -bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version)
Nice trick :) To be honest, I am not really fond of `set`, and in this
case it means the command is not symmetric (we remove the prefix using
parameter expansion, and the suffix via positional argument
selection), but if you prefer it that way, I think it would be fine.
However, why the double asterisk? One already matches any string,
including spaces, no?
Cheers,
Miguel
On Thu, Jan 12, 2023 at 6:29 AM Masahiro Yamada <[email protected]> wrote:
>
> I do not like this.
> We do not need to cater to every oddity.
>
> Checking MAKEFLAGS is too much.
I agree we should not attempt to catch every possible mistake in the
script, but there have been several people hitting precisely this case
(the latest is in the linked thread in the commit message), i.e. some
people read the `Makefile` and notice the script invocation, and go
execute it, but they are unlikely to be aware of the target in that
case.
> You can check RUSTC/BINDGEN/CC if you persist in this.
This is fine, and actually we should do it regardless of `MAKEFLAGS`.
I can add it to v2.
However, that does not cover the same thing as `MAKEFLAGS` is trying
to here. The reason is that even if they see e.g. "RUSTC is not set",
they will not know about how to call the script properly, i.e. through
the `Makefile` target.
For `RUSTC` and `BINDGEN`, it does not really matter (and we could
give a default to the variable, since the name rarely would be
different). However, for `CC`, the logic that Kbuild uses is more
complex, so it seems best to me to let Kbuild tell us what the actual
compiler is.
Cheers,
Miguel
On Thu, Jan 12, 2023 at 7:05 AM Masahiro Yamada <[email protected]> wrote:
>
> Yes, I can take this, but the doc change
> is independent of the rest, and will not conflict with
> any Kbuild changes.
>
> So, you can apply this one to your tree.
The doc change is not fully independent: this patch is first because
the next commit uses the fact that the documentation is written (to
point the user to it), and the commit message mentions this.
Not a big deal, but it would look better if all are in at once.
Cheers,
Miguel
On Thu, Jan 12, 2023 at 7:23 AM Masahiro Yamada <[email protected]> wrote:
>
> Maybe, your purpose is to use sed consistently, but
> perhaps you can avoid forking sed if you know the
> format of the first line.
The most unknown format would be the one of the libclang check, where
there may be other lines before the one we are interested in. However,
the pattern expansion would still match newlines, right?
> BTW, what is missing here is, you do not check if
> ${RUSTC} is really rustc.
>
> I can fool this script to print
> "arithmetic expression: expecting primary: "100000 * + 100 * + "
We can test if nothing was printed by `sed` for that (or do it with
shell builtins).
Having said that, I would say fooling the script on purpose is an more
of an oddity compared to the case `MAKEFLAGS` attempts to cover
(please see my reply on the other patch). So if we cover this, then I
would say we should really cover the other one.
Cheers,
Miguel
On Mon, Jan 9, 2023 at 9:46 PM Miguel Ojeda <[email protected]> wrote:
>
> Reported-by: Jordan (@jordanisaacs)
Cc'ing Jordan who gave us the email address in GitHub and wants to
send a `Tested-by` tag.
Cheers,
Miguel
On Thu, Jan 12, 2023 at 5:35 AM Masahiro Yamada <[email protected]> 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
On Sat, Jan 14, 2023 at 8:10 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, Jan 12, 2023 at 5:35 AM Masahiro Yamada <[email protected]> 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.
--
Best Regards
Masahiro Yamada
On Sat, Jan 14, 2023 at 10:44 AM Masahiro Yamada <[email protected]> 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
On Mon, Jan 9, 2023 at 9:45 PM Miguel Ojeda <[email protected]> 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
On Sat, Jan 14, 2023 at 8:12 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, Jan 12, 2023 at 6:29 AM Masahiro Yamada <[email protected]> wrote:
> >
> > I do not like this.
> > We do not need to cater to every oddity.
> >
> > Checking MAKEFLAGS is too much.
>
> I agree we should not attempt to catch every possible mistake in the
> script, but there have been several people hitting precisely this case
> (the latest is in the linked thread in the commit message), i.e. some
> people read the `Makefile` and notice the script invocation, and go
> execute it, but they are unlikely to be aware of the target in that
> case.
>
> > You can check RUSTC/BINDGEN/CC if you persist in this.
>
> This is fine, and actually we should do it regardless of `MAKEFLAGS`.
> I can add it to v2.
>
> However, that does not cover the same thing as `MAKEFLAGS` is trying
> to here. The reason is that even if they see e.g. "RUSTC is not set",
> they will not know about how to call the script properly, i.e. through
> the `Makefile` target.
>
> For `RUSTC` and `BINDGEN`, it does not really matter (and we could
> give a default to the variable, since the name rarely would be
> different). However, for `CC`, the logic that Kbuild uses is more
> complex, so it seems best to me to let Kbuild tell us what the actual
> compiler is.
>
> Cheers,
> Miguel
OK, you maintain this script, so it is up to you.
--
Best Regards
Masahiro Yamada
On Sat, Jan 14, 2023 at 8:13 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, Jan 12, 2023 at 6:32 AM Masahiro Yamada <[email protected]> wrote:
> >
> > +set -- ${bindgen_libclang_output#**clang version}
> > +bindgen_libclang_cversion=$(get_canonical_version $1)
> > bindgen_libclang_min_version=$($min_tool_version llvm)
> > -bindgen_libclang_cversion=$(get_canonical_version $bindgen_libclang_version)
>
> Nice trick :) To be honest, I am not really fond of `set`, and in this
> case it means the command is not symmetric (we remove the prefix using
> parameter expansion, and the suffix via positional argument
> selection), but if you prefer it that way, I think it would be fine.
I just tend to write efficient code.
(scripts/{cc,ld,as}-version.sh do not use sed or grep at all.)
Especially, I avoid unneeded process forks
in the process forks.
> However, why the double asterisk? One already matches any string,
> including spaces, no?
Sorry, it is my mistake.
I meant double pound.
set -- ${bindgen_libclang_output##*clang version}
The double pound strips "the longest matching pattern",
just in case "clang version" is contained in the file path.
(but if a space is contained in the directory path,
it would have failed earlier.
>
> Cheers,
> Miguel
--
Best Regards
Masahiro Yamada
On Sat, Jan 14, 2023 at 8:15 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, Jan 12, 2023 at 7:23 AM Masahiro Yamada <[email protected]> wrote:
> >
> > Maybe, your purpose is to use sed consistently, but
> > perhaps you can avoid forking sed if you know the
> > format of the first line.
>
> The most unknown format would be the one of the libclang check, where
> there may be other lines before the one we are interested in. However,
> the pattern expansion would still match newlines, right?
>
> > BTW, what is missing here is, you do not check if
> > ${RUSTC} is really rustc.
> >
> > I can fool this script to print
> > "arithmetic expression: expecting primary: "100000 * + 100 * + "
>
> We can test if nothing was printed by `sed` for that (or do it with
> shell builtins).
>
> Having said that, I would say fooling the script on purpose is an more
> of an oddity compared to the case `MAKEFLAGS` attempts to cover
> (please see my reply on the other patch). So if we cover this, then I
> would say we should really cover the other one.
get_canonical_version() in scripts/as-version.sh has
a little more trick to avoid
"arithmetic expression: expecting primary: "100000 * + 100 * + "
but it is up to you.
> Cheers,
> Miguel
--
Best Regards
Masahiro Yamada
On Sat, Jan 14, 2023 at 8:13 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, Jan 12, 2023 at 7:05 AM Masahiro Yamada <[email protected]> wrote:
> >
> > Yes, I can take this, but the doc change
> > is independent of the rest, and will not conflict with
> > any Kbuild changes.
> >
> > So, you can apply this one to your tree.
>
> The doc change is not fully independent: this patch is first because
> the next commit uses the fact that the documentation is written (to
> point the user to it), and the commit message mentions this.
>
> Not a big deal, but it would look better if all are in at once.
>
> Cheers,
> Miguel
Now I think it is better to ask you to pick up my patch [1]
and apply all of this patch set in your tree
since you are adding bigger changes.
[1]: https://patchwork.kernel.org/project/linux-kbuild/patch/[email protected]/
--
Best Regards
Masahiro Yamada
On Sun, Jan 15, 2023 at 11:48 AM Masahiro Yamada <[email protected]> wrote:
>
> On Sat, Jan 14, 2023 at 8:15 AM Miguel Ojeda
> <[email protected]> wrote:
> >
> > On Thu, Jan 12, 2023 at 7:23 AM Masahiro Yamada <[email protected]> wrote:
> > >
> > > Maybe, your purpose is to use sed consistently, but
> > > perhaps you can avoid forking sed if you know the
> > > format of the first line.
> >
> > The most unknown format would be the one of the libclang check, where
> > there may be other lines before the one we are interested in. However,
> > the pattern expansion would still match newlines, right?
> >
> > > BTW, what is missing here is, you do not check if
> > > ${RUSTC} is really rustc.
> > >
> > > I can fool this script to print
> > > "arithmetic expression: expecting primary: "100000 * + 100 * + "
> >
> > We can test if nothing was printed by `sed` for that (or do it with
> > shell builtins).
> >
> > Having said that, I would say fooling the script on purpose is an more
> > of an oddity compared to the case `MAKEFLAGS` attempts to cover
> > (please see my reply on the other patch). So if we cover this, then I
> > would say we should really cover the other one.
>
>
>
> get_canonical_version() in scripts/as-version.sh has
> a little more trick to avoid
> "arithmetic expression: expecting primary: "100000 * + 100 * + "
> but it is up to you.
My code accepts anything that is separated by dots
(and non-numerical strings are silently turned into zero).
Your code takes exactly the "([0-9]+\.[0-9]+\.[0-9]+)" pattern,
so it works very safely.
I think using sed is fine.
--
Best Regards
Masahiro Yamada