Re: [RFC PATCH 01/11] rust: qemu_api: do not disable lints outside bindgen-generated code

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> rust/qemu-api/src/lib.rs is disabling lints that cause problems
> with code generated by bindgen.

The commit title is misleading. Today the lint-disabling attributes are
outer ones which apply only to the bindings module. Those lints already
apply to the others in the qemu_api crate.

That said, moving the bindings mod and the related extensions (trait
impls, etc.) into a separate file still looks a good idea to me.

> Instead, include the bindgen
> code via include!(...) and move the #![allow()] directives
> into the bindings module.
>
> Add MESON_BUILD_ROOT to the devenv, so that it's easy for
> build.rs to find the include file.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                   |  4 +++-
>  rust/qemu-api/.gitignore      |  2 +-
>  rust/qemu-api/build.rs        | 20 ++++++++++++++------
>  rust/qemu-api/meson.build     |  1 +
>  rust/qemu-api/src/bindings.rs | 29 +++++++++++++++++++++++++++++
>  rust/qemu-api/src/lib.rs      | 22 ----------------------
>  6 files changed, 48 insertions(+), 30 deletions(-)
>  create mode 100644 rust/qemu-api/src/bindings.rs
>
> diff --git a/meson.build b/meson.build
> index e0b880e4e13..a7342c6edbd 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3,6 +3,8 @@ project('qemu', ['c'], meson_version: '>=1.5.0',
>                            'b_staticpic=false', 'stdsplit=false', 
> 'optimization=2', 'b_pie=true'],
>          version: files('VERSION'))
>
> +meson.add_devenv({ 'MESON_BUILD_ROOT' : meson.project_build_root() })
> +
>  add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: 
> true)
>  add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1', 
> 'SPEED=slow'])
>  add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
> @@ -4089,7 +4091,7 @@ if have_rust
>    bindings_rs = rust.bindgen(
>      input: 'rust/wrapper.h',
>      dependencies: common_ss.all_dependencies(),
> -    output: 'bindings.rs',
> +    output: 'bindings.rs.inc',
>      include_directories: include_directories('.', 'include'),
>      bindgen_version: ['>=0.60.0'],
>      args: bindgen_args,
> diff --git a/rust/qemu-api/.gitignore b/rust/qemu-api/.gitignore
> index b9e7e004c86..2accb8745dc 100644
> --- a/rust/qemu-api/.gitignore
> +++ b/rust/qemu-api/.gitignore
> @@ -1,2 +1,2 @@
>  # Ignore generated bindings file overrides.
> -src/bindings.rs
> +/src/bindings.rs.inc
> diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs
> index 20f8f718b90..e4eab718553 100644
> --- a/rust/qemu-api/build.rs
> +++ b/rust/qemu-api/build.rs
> @@ -2,18 +2,26 @@
>  // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>  // SPDX-License-Identifier: GPL-2.0-or-later
>
> -use std::path::Path;
> +use std::{env, path::Path};
>
>  use version_check as rustc;
>
>  fn main() {
> -    if !Path::new("src/bindings.rs").exists() {
> -        panic!(
> -            "No generated C bindings found! Either build them manually with 
> bindgen or with meson \
> -             (`ninja bindings.rs`) and copy them to src/bindings.rs, or 
> build through meson."
> -        );
> +    // Placing bindings.rs.inc in the source directory is supported
> +    // but not documented or encouraged.

I agree that storing generated stuff in the source directory should not
be encouraged.

Just want to mention that such changes can lead to trouble to
rust-analyzer. Today there are two ways to inform rust-analyzer of the
project structure:

  1. Use rust/Cargo.toml. In this case we'll hit an issue in
  rust-analyzer [1] that prevents it from including sources outside the
  crate directory. Thus, definitions in the bindgen-generated code
  cannot be found.

  2. Use the meson-generated rust-project.json. Due to the use of
  structured_sources(), that json file refers to the copied sources of
  qemu-api in the build directory. Rust-analyzer can find every symbol
  in the qemu-api crate but will jump to those copied files, making it a
  bit more annoying when developing the crate.

We can perhaps leave it as a separate topic for another series.

[1] https://github.com/rust-lang/rust-analyzer/issues/17040

--
Best Regards
Junjie Mao

> +    let path = env::var("MESON_BUILD_ROOT")
> +        .unwrap_or_else(|_| format!("{}/src", env!("CARGO_MANIFEST_DIR")));
> +
> +    let file = format!("{}/bindings.rs.inc", path);
> +    if !Path::new(&file).exists() {
> +        panic!(concat!(
> +            "No generated C bindings found! If you want to run `cargo`, 
> start a subshell\n",
> +            "with `meson devenv`, or point MESON_BUILD_ROOT to the top of 
> the build tree."
> +        ));
>      }
>
> +    println!("cargo:rustc-env=BINDINGS_RS_INC={}", file);
> +
>      // Check for available rustc features
>      if rustc::is_min_version("1.77.0").unwrap_or(false) {
>          println!("cargo:rustc-cfg=has_offset_of");
> diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
> index 6f637af7b1b..e3870e901e3 100644
> --- a/rust/qemu-api/meson.build
> +++ b/rust/qemu-api/meson.build
> @@ -9,6 +9,7 @@ _qemu_api_rs = static_library(
>    structured_sources(
>      [
>        'src/lib.rs',
> +      'src/bindings.rs',
>        'src/c_str.rs',
>        'src/definitions.rs',
>        'src/device_class.rs',
> diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
> new file mode 100644
> index 00000000000..1dac310594d
> --- /dev/null
> +++ b/rust/qemu-api/src/bindings.rs
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#![allow(
> +    dead_code,
> +    improper_ctypes_definitions,
> +    improper_ctypes,
> +    non_camel_case_types,
> +    non_snake_case,
> +    non_upper_case_globals,
> +    unsafe_op_in_unsafe_fn,
> +    clippy::missing_const_for_fn,
> +    clippy::too_many_arguments,
> +    clippy::approx_constant,
> +    clippy::use_self,
> +    clippy::useless_transmute,
> +    clippy::missing_safety_doc
> +)]
> +
> +#[cfg(MESON)]
> +include!("bindings.rs.inc");
> +
> +#[cfg(not(MESON))]
> +include!(env!("BINDINGS_RS_INC"));
> +
> +unsafe impl Send for Property {}
> +unsafe impl Sync for Property {}
> +unsafe impl Sync for TypeInfo {}
> +unsafe impl Sync for VMStateDescription {}
> +unsafe impl Sync for VMStateField {}
> +unsafe impl Sync for VMStateInfo {}
> diff --git a/rust/qemu-api/src/lib.rs b/rust/qemu-api/src/lib.rs
> index aa8d16ec94b..440aff3817d 100644
> --- a/rust/qemu-api/src/lib.rs
> +++ b/rust/qemu-api/src/lib.rs
> @@ -4,31 +4,9 @@
>
>  #![cfg_attr(not(MESON), doc = include_str!("../README.md"))]
>
> -#[allow(
> -    dead_code,
> -    improper_ctypes_definitions,
> -    improper_ctypes,
> -    non_camel_case_types,
> -    non_snake_case,
> -    non_upper_case_globals,
> -    unsafe_op_in_unsafe_fn,
> -    clippy::missing_const_for_fn,
> -    clippy::too_many_arguments,
> -    clippy::approx_constant,
> -    clippy::use_self,
> -    clippy::useless_transmute,
> -    clippy::missing_safety_doc,
> -)]
>  #[rustfmt::skip]
>  pub mod bindings;
>
> -unsafe impl Send for bindings::Property {}
> -unsafe impl Sync for bindings::Property {}
> -unsafe impl Sync for bindings::TypeInfo {}
> -unsafe impl Sync for bindings::VMStateDescription {}
> -unsafe impl Sync for bindings::VMStateField {}
> -unsafe impl Sync for bindings::VMStateInfo {}
> -
>  pub mod c_str;
>  pub mod definitions;
>  pub mod device_class;