Re: [RFC PATCH 11/11] rust: ci: add job that runs Rust tools

Re: [RFC PATCH 11/11] rust: ci: add job that runs Rust tools

[Daniel P. Berrangé]

On Fri, Nov 08, 2024 at 07:01:39PM +0100, Paolo Bonzini wrote:
> Code checks, as well as documentation generation, are not yet tied
> to "make check" because they need new version of the Rust toolchain
> (even nightly in the case of "rustfmt").  Run them in CI using the
> existing nightly-Rust container.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  .gitlab-ci.d/buildtest-template.yml                | 14 ++++++++++++++
>  .gitlab-ci.d/buildtest.yml                         | 14 ++++++++++++++
>  .../docker/dockerfiles/fedora-rust-nightly.docker  |  4 ++++
>  tests/lcitool/refresh                              |  4 ++++
>  4 files changed, 36 insertions(+)
> 
> diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml
> index 39da7698b09..612e968ff19 100644
> --- a/.gitlab-ci.d/buildtest-template.yml
> +++ b/.gitlab-ci.d/buildtest-template.yml
> @@ -79,6 +79,20 @@
>      - $MAKE NINJA=":" $MAKE_CHECK_ARGS
>      - section_end test
>  
> +.rust_test_job_template:
> +  extends: .base_job_template
> +  stage: test
> +  image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:$QEMU_CI_CONTAINER_TAG
> +  script:
> +    - source scripts/ci/gitlab-ci-section
> +    - section_start test "Running Rust code checks"
> +    - cd build
> +    - pyvenv/bin/meson devenv -w ../rust ${CARGO-cargo} fmt --check
> +    - make clippy
> +    - pyvenv/bin/meson devenv -w ../rust ${CARGO-cargo} doc --no-deps
> +    - pyvenv/bin/meson devenv -w ../rust ${CARGO-cargo} test --doc
> +    - section_end test
> +

I'd suggest that the static checks "fmt" and "doc" should be separated
from the dynamic (unit test) check in  "tests", and that the former
should be in a job defined in the static-checks.yml file.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 02/11] rust: build: move rustc_args.py invocation to individual crates

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Only qemu-api needs access to the symbols in config-host.h.

This may no longer be the case when more complex, build-time
configurable devices are added in the future.

Moving rustc_args.py calls to each crate is still helpful because of the
changes in patches 3-6 in this series. So I think every crate under
rust/ needs a run_command(rustc_args, ...) for crate-specific arguments.

--
Best Regards
Junjie Mao

> Remove
> the temptation to use them by limiting the --cfg arguments to the
> qemu-api crate.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build               | 54 +++++++++++++++++----------------------
>  rust/qemu-api/meson.build |  4 +++
>  2 files changed, 28 insertions(+), 30 deletions(-)

Re: [RFC PATCH 05/11] rust: cargo: store desired warning levels in workspace Cargo.toml

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il mar 12 nov 2024, 04:17 Junjie Mao <junjie.mao@hotmail.com> ha scritto:
>
>  Making a universal unexpected_cfgs apply to the whole workspace may lead
>  to a lengthy cfg list when more devices in Rust are added. As cargo does
>  not allow overriding workspace-defined lints once inherited, I think it
>  better to keep unexpected_cfgs crate-specific.
>
> Is it possible? I thought you cannot override at a finer granularity once you have a "workspace = true" line.

No, such overriding is not supported by cargo today. I'm thinking about
removing the workspace.lints.rust section, but ...

>
> Based on the experience with C we shouldn't have many cfgs, but if it's possible I would definitely make unexpected_cfgs specific to qemu-api.

... a quick grep finds 33 different CONFIG_* being used in C under
hw/. In that case one universal list of expected cfgs does not look like
a problem. Thanks for pointing this out.

--
Best Regards
Junjie Mao

>
> Paolo
>

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

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il mar 12 nov 2024, 03:47 Junjie Mao <junjie.mao@hotmail.com> ha scritto:
>
>  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.
>
> Would it help to move the bindgen-generated code to a completely separate crate (e.g. qemu-api-sys), and avoid structured_sources for qemu-api? It might even help build times.

I just noticed that rust-analyzer is able to include files under
OUT_DIR. With the following changes, rust-analyzer under meson devenv
works nicely:

  1. Rust-analyzer refers to rust/qemu-api/src/* unless the definition
  is in bindings.inc.rs.

  2. No manual copy / symbolic link required, neither bindings.inc.rs
  nor rust-project.json.

The bindgen-generated file is renamed to bindings.inc.rs only because
rust-analyzer seems to refuse including a file without the .rs suffix.

That's at the cost of another file copy, though.

--- diff starts here ---

diff --git a/meson.build b/meson.build
index 1239f5c48c..8cea09ffe1 100644
--- a/meson.build
+++ b/meson.build
@@ -4,6 +4,7 @@ project('qemu', ['c'], meson_version: '>=1.5.0',
         version: files('VERSION'))

 meson.add_devenv({ 'MESON_BUILD_ROOT' : meson.project_build_root() })
+meson.add_devenv({ 'CARGO_TARGET_DIR' : meson.project_build_root() / 'cargo' })

 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'])
@@ -4083,7 +4084,7 @@ if have_rust
   bindings_rs = rust.bindgen(
     input: 'rust/wrapper.h',
     dependencies: common_ss.all_dependencies(),
-    output: 'bindings.rs.inc',
+    output: 'bindings.inc.rs',
     include_directories: include_directories('.', 'include'),
     bindgen_version: ['>=0.60.0'],
     args: bindgen_args,
diff --git a/rust/qemu-api/build.rs b/rust/qemu-api/build.rs
index d7b6d76828..1de86c721b 100644
--- a/rust/qemu-api/build.rs
+++ b/rust/qemu-api/build.rs
@@ -2,17 +2,17 @@
 // Author(s): Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
 // SPDX-License-Identifier: GPL-2.0-or-later

-use std::{env, path::Path};
+use std::{env, fs::copy, io::Result, path::Path};

 use version_check as rustc;

-fn main() {
+fn main() -> Result<()> {
     // Placing bindings.rs.inc in the source directory is supported
     // but not documented or encouraged.
     let path = env::var("MESON_BUILD_ROOT")
         .unwrap_or_else(|_| format!("{}/src", env!("CARGO_MANIFEST_DIR")));

-    let file = format!("{}/bindings.rs.inc", path);
+    let file = format!("{}/bindings.inc.rs", path);
     if !Path::new(&file).exists() {
         panic!(concat!(
             "\n",
@@ -24,7 +24,9 @@ fn main() {
         ));
     }

-    println!("cargo:rustc-env=BINDINGS_RS_INC={}", file);
+    let out_dir = env::var("OUT_DIR").unwrap();
+    let dest_path = format!("{}/bindings.inc.rs", out_dir);
+    copy(&file, Path::new(&dest_path))?;

     // Check for available rustc features
     if rustc::is_min_version("1.77.0").unwrap_or(false) {
@@ -32,4 +34,6 @@ fn main() {
     }

     println!("cargo:rerun-if-changed=build.rs");
+
+    Ok(())
 }
diff --git a/rust/qemu-api/src/bindings.rs b/rust/qemu-api/src/bindings.rs
index 972b1f1ee9..8a9b821bb9 100644
--- a/rust/qemu-api/src/bindings.rs
+++ b/rust/qemu-api/src/bindings.rs
@@ -16,10 +16,10 @@
 )]

 #[cfg(MESON)]
-include!("bindings.rs.inc");
+include!("bindings.inc.rs");

 #[cfg(not(MESON))]
-include!(env!("BINDINGS_RS_INC"));
+include!(concat!(env!("OUT_DIR"), "/bindings.inc.rs"));

 unsafe impl Send for Property {}
 unsafe impl Sync for Property {}

--
Best Regards
Junjie Mao

>
> Paolo
>
>  We can perhaps leave it as a separate topic for another series.
>
>  [1] https://github.com/rust-lang/rust-analyzer/issues/17040
>

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

[Paolo Bonzini]

On 11/12/24 11:10, Junjie Mao wrote:
> diff --git a/meson.build b/meson.build
> index 1239f5c48c..8cea09ffe1 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -4,6 +4,7 @@ project('qemu', ['c'], meson_version: '>=1.5.0',
>           version: files('VERSION'))
> 
>   meson.add_devenv({ 'MESON_BUILD_ROOT' : meson.project_build_root() })
> +meson.add_devenv({ 'CARGO_TARGET_DIR' : meson.project_build_root() / 'cargo' })

I think I'd rather avoid using CARGO_TARGET_DIR, in case someone forgets 
he/she is in the devenv.

>   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'])
> @@ -4083,7 +4084,7 @@ if have_rust
>     bindings_rs = rust.bindgen(
>       input: 'rust/wrapper.h',
>       dependencies: common_ss.all_dependencies(),
> -    output: 'bindings.rs.inc',
> +    output: 'bindings.inc.rs',

Needs a comment, but I guess it's okay.

> -    println!("cargo:rustc-env=BINDINGS_RS_INC={}", file);
> +    let out_dir = env::var("OUT_DIR").unwrap();
> +    let dest_path = format!("{}/bindings.inc.rs", out_dir);
> +    copy(&file, Path::new(&dest_path))?;

A symbolic link seems to work too.  Thanks for the tip!

Paolo

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

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/12/24 11:10, Junjie Mao wrote:
>> diff --git a/meson.build b/meson.build
>> index 1239f5c48c..8cea09ffe1 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -4,6 +4,7 @@ project('qemu', ['c'], meson_version: '>=1.5.0',
>>           version: files('VERSION'))
>>   meson.add_devenv({ 'MESON_BUILD_ROOT' : meson.project_build_root() })
>> +meson.add_devenv({ 'CARGO_TARGET_DIR' : meson.project_build_root() / 'cargo' })
>
> I think I'd rather avoid using CARGO_TARGET_DIR, in case someone forgets he/she
> is in the devenv.
>

I should have dropped this line earlier. It was from my first attempt
where I wanted to put the generated bindings directly to cargo OUT_DIR
(so that file copy can be avoided). That didn't work because OUT_DIR
contains hashes that are hard to predict.

>>   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'])
>> @@ -4083,7 +4084,7 @@ if have_rust
>>     bindings_rs = rust.bindgen(
>>       input: 'rust/wrapper.h',
>>       dependencies: common_ss.all_dependencies(),
>> -    output: 'bindings.rs.inc',
>> +    output: 'bindings.inc.rs',
>
> Needs a comment, but I guess it's okay.
>
>> -    println!("cargo:rustc-env=BINDINGS_RS_INC={}", file);
>> +    let out_dir = env::var("OUT_DIR").unwrap();
>> +    let dest_path = format!("{}/bindings.inc.rs", out_dir);
>> +    copy(&file, Path::new(&dest_path))?;
>
> A symbolic link seems to work too.  Thanks for the tip!

Linux-based hosts should be fine. Should we test if symlinks also work
on W32/W64 systems?

>
> Paolo

--
Best Regards
Junjie Mao

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

[Paolo Bonzini]

On Wed, Nov 13, 2024 at 7:52 AM Junjie Mao <junjie.mao@hotmail.com> wrote:
> > A symbolic link seems to work too.  Thanks for the tip!
>
> Linux-based hosts should be fine. Should we test if symlinks also work
> on W32/W64 systems?

We already use symlinks for scripts/symlink-install-tree.py, fortunately.

Paolo