diff mbox series

[v2,1/1] scripts/rust_is_available: Fix clang version check

Message ID 20230528131802.6390-2-ethan.twardy@gmail.com
State New
Headers
Series Fix libclang version check for rustavailable |

Commit Message

Ethan D. Twardy May 28, 2023, 1:18 p.m. UTC 
  During out-of-tree builds where the path to the kernel source tree
contains a version string, scripts/rust_is_available.sh incorrectly
identified the version string of libclang to be the version string in
the kernel sources path, resulting in CONFIG_RUST_IS_AVAILABLE
erroneously set to 'n'.

This issue was previously affecting builds on distributions, such as
Gentoo Linux, where the kernel source tree is under version control,
and placed under a path containing the current kernel version string
in /usr/src.

The fix is to take special care to match only the version string
following the string 'clang version' in the output.

To reproduce:

  $ cd ~/build && make -C ~/linux-6.2.0 O=$PWD LLVM=1 rustavailable
  [...]
  *** libclang (used by the Rust bindings generator 'bindgen') is too old.
  ***   Your version:    6.2.0
  ***   Minimum version: 11.0.0
  [...]

Fixes: 78521f3399ab ("scripts: add `rust_is_available.sh`")
Signed-off-by: Ethan D. Twardy <ethan.twardy@gmail.com>

Comments

Martin Rodriguez Reboredo May 28, 2023, 2:39 p.m. UTC | #1 
On 5/28/23 10:18, Ethan D. Twardy wrote:
> [...]
> --- a/scripts/rust_is_available.sh
> +++ b/scripts/rust_is_available.sh
> @@ -102,8 +102,8 @@ fi
>   # Check that the `libclang` used 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 \
> -		| grep -F 'clang version ' \
> -		| grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
> +		| grep -oE 'clang version [0-9]+\.[0-9]+\.[0-9]+' \
> +		| cut -d' ' -f3 \
>   		| head -n 1 \
>   )
>   bindgen_libclang_min_version=$($min_tool_version llvm)

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Tested-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Ethan D. Twardy June 14, 2023, 12:08 p.m. UTC | #2 
Greetings, Martin!

Thanks very much for your testing and review. Do I need to take any
further action to make this patch ready for review? I sent out a v3
version of the patch, where the only change was adding Reviewed-By and
Tested-By tags to my git commit--since this is my first time
contributing to this community, I wasn't sure whether I needed to do
that. Thanks!

Sincerely,

Ethan Twardy
Martin Rodriguez Reboredo June 14, 2023, 4:44 p.m. UTC | #3 
On 6/14/23 09:08, Ethan D. Twardy wrote:
> Greetings, Martin!
> 
> Thanks very much for your testing and review. Do I need to take any
> further action to make this patch ready for review? I sent out a v3
> version of the patch, where the only change was adding Reviewed-By and
> Tested-By tags to my git commit--since this is my first time
> contributing to this community, I wasn't sure whether I needed to do
> that. Thanks!
> 
> Sincerely,
> 
> Ethan Twardy

Unless other reviewers requested changes, you have to rebase your
work or you want to make a modification, it is not necessary to send a
new version of the patch series, generally, maintainers will, and have
to, add any Reviewed-bys or Tested-bys prior to merging.
Miguel Ojeda June 16, 2023, 12:30 a.m. UTC | #4 
On Sun, May 28, 2023 at 3:21 PM Ethan D. Twardy <ethan.twardy@gmail.com> wrote:
>
> During out-of-tree builds where the path to the kernel source tree
> contains a version string, scripts/rust_is_available.sh incorrectly
> identified the version string of libclang to be the version string in
> the kernel sources path, resulting in CONFIG_RUST_IS_AVAILABLE
> erroneously set to 'n'.
>
> This issue was previously affecting builds on distributions, such as
> Gentoo Linux, where the kernel source tree is under version control,
> and placed under a path containing the current kernel version string
> in /usr/src.
>
> The fix is to take special care to match only the version string
> following the string 'clang version' in the output.
>
> To reproduce:
>
>   $ cd ~/build && make -C ~/linux-6.2.0 O=$PWD LLVM=1 rustavailable
>   [...]
>   *** libclang (used by the Rust bindings generator 'bindgen') is too old.
>   ***   Your version:    6.2.0
>   ***   Minimum version: 11.0.0
>   [...]
>
> Fixes: 78521f3399ab ("scripts: add `rust_is_available.sh`")
> Signed-off-by: Ethan D. Twardy <ethan.twardy@gmail.com>

Thanks a lot for this patch! This was previously reported and should
be fixed in this patch series (I added you as a reporter in v2):

    https://lore.kernel.org/rust-for-linux/20230616001631.463536-1-ojeda@kernel.org/

If you could please test the series, to make sure it solves your
issue, it would be great!

Cheers,
Miguel
Miguel Ojeda June 16, 2023, 12:41 a.m. UTC | #5 
On Wed, Jun 14, 2023 at 2:08 PM Ethan D. Twardy <ethan.twardy@gmail.com> wrote:
>
> Thanks very much for your testing and review. Do I need to take any
> further action to make this patch ready for review? I sent out a v3
> version of the patch, where the only change was adding Reviewed-By and
> Tested-By tags to my git commit--since this is my first time
> contributing to this community, I wasn't sure whether I needed to do
> that. Thanks!

You did everything well! The patch looked fine to me, and the commit
message is detailed. The tags are all fine too. Thanks a lot for the
effort!

I had v2 of the script improvement patch series in my backlog, which
normalizes to `sed` (rather than use `grep`/`cut` etc.), adds a test
suite, and a few other things; but otherwise we could have easily
taken this one, because it is well done.

So, you did everything right, and you are more than welcome to send
more patches like that :)

Cheers,
Miguel
diff mbox series

Patch

diff --git a/scripts/rust_is_available.sh b/scripts/rust_is_available.sh
index aebbf1913970..e8a1439be9f8 100755
--- a/scripts/rust_is_available.sh
+++ b/scripts/rust_is_available.sh
@@ -102,8 +102,8 @@  fi
 # Check that the `libclang` used 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 \
-		| grep -F 'clang version ' \
-		| grep -oE '[0-9]+\.[0-9]+\.[0-9]+' \
+		| grep -oE 'clang version [0-9]+\.[0-9]+\.[0-9]+' \
+		| cut -d' ' -f3 \
 		| head -n 1 \
 )
 bindgen_libclang_min_version=$($min_tool_version llvm)