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

Paolo Bonzini posted 11 patches 2 weeks, 1 day ago
There is a newer version of this series
[RFC PATCH 05/11] rust: cargo: store desired warning levels in workspace Cargo.toml
Posted by Paolo Bonzini 2 weeks, 1 day ago
An extra benefit of workspaces is that they allow to place lint level
settings in a single Cargo.toml; the settings are then inherited by
packages in the workspace.

Correspondingly, teach rustc_build_args.py to get the unexpected_cfgs
configuration from the workspace Cargo.toml.

Note that it is still possible to allow or deny warnings per crate or
module, via the #![] attribute syntax.  The rust/qemu-api/src/bindings.rs
file is an example.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 meson.build                     |  8 +++++---
 rust/Cargo.toml                 |  8 ++++++++
 rust/hw/char/pl011/Cargo.toml   |  3 +++
 rust/qemu-api-macros/Cargo.toml |  3 +++
 rust/qemu-api/Cargo.toml        |  5 ++---
 rust/qemu-api/meson.build       |  2 +-
 scripts/rust/rustc_args.py      | 31 +++++++++++++++++++++++++++----
 7 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/meson.build b/meson.build
index 7a9a523467b..5726135b324 100644
--- a/meson.build
+++ b/meson.build
@@ -120,11 +120,13 @@ if have_rust
 endif
 
 if have_rust
-  rustc_args = find_program('scripts/rust/rustc_args.py')
+  rustc_args = [find_program('scripts/rust/rustc_args.py'),
+    '--workspace', meson.project_source_root() / 'rust']
   rustfmt = find_program('rustfmt', required: false)
 
-  # Prohibit code that is forbidden in Rust 2024
-  rustc_lint_args = ['-D', 'unsafe_op_in_unsafe_fn']
+  rustc_lint_args = run_command(rustc_args,
+     '--lints', files('rust/Cargo.toml'),
+     capture: true, check: true).stdout().strip().splitlines()
 
   # Occasionally, we may need to silence warnings and clippy lints that
   # were only introduced in newer Rust compiler versions.  Do not croak
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 0c94d5037da..0230b92a9fa 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -5,3 +5,11 @@ members = [
     "qemu-api",
     "hw/char/pl011",
 ]
+
+[workspace.lints.rust]
+unexpected_cfgs = { level = "warn", check-cfg = [
+    'cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)',
+    'cfg(has_offset_of)'] }
+
+# Prohibit code that is forbidden in Rust 2024
+unsafe_op_in_unsafe_fn = "deny"
diff --git a/rust/hw/char/pl011/Cargo.toml b/rust/hw/char/pl011/Cargo.toml
index a373906b9fb..58f3e859f7e 100644
--- a/rust/hw/char/pl011/Cargo.toml
+++ b/rust/hw/char/pl011/Cargo.toml
@@ -21,3 +21,6 @@ bilge = { version = "0.2.0" }
 bilge-impl = { version = "0.2.0" }
 qemu_api = { path = "../../../qemu-api" }
 qemu_api_macros = { path = "../../../qemu-api-macros" }
+
+[lints]
+workspace = true
diff --git a/rust/qemu-api-macros/Cargo.toml b/rust/qemu-api-macros/Cargo.toml
index a8f7377106b..5a27b52ee6e 100644
--- a/rust/qemu-api-macros/Cargo.toml
+++ b/rust/qemu-api-macros/Cargo.toml
@@ -20,3 +20,6 @@ proc-macro = true
 proc-macro2 = "1"
 quote = "1"
 syn = { version = "2", features = ["extra-traits"] }
+
+[lints]
+workspace = true
diff --git a/rust/qemu-api/Cargo.toml b/rust/qemu-api/Cargo.toml
index cc716d75d46..669f288d1cb 100644
--- a/rust/qemu-api/Cargo.toml
+++ b/rust/qemu-api/Cargo.toml
@@ -23,6 +23,5 @@ version_check = "~0.9"
 default = []
 allocator = []
 
-[lints.rust]
-unexpected_cfgs = { level = "warn", check-cfg = ['cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)',
-                                                 'cfg(has_offset_of)'] }
+[lints]
+workspace = true
diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
index 4ba5607d66b..8013911a348 100644
--- a/rust/qemu-api/meson.build
+++ b/rust/qemu-api/meson.build
@@ -5,7 +5,7 @@ if rustc.version().version_compare('>=1.77.0')
 endif
 
 _qemu_api_cfg += run_command(rustc_args,
-  '--config-headers', config_host_h, '--features', '--lints', files('Cargo.toml'),
+  '--config-headers', config_host_h, '--features', files('Cargo.toml'),
   capture: true, check: true).stdout().strip().splitlines()
 
 _qemu_api_rs = static_library(
diff --git a/scripts/rust/rustc_args.py b/scripts/rust/rustc_args.py
index 9b9778a1cac..26733439ed4 100644
--- a/scripts/rust/rustc_args.py
+++ b/scripts/rust/rustc_args.py
@@ -38,12 +38,19 @@
 
 class CargoTOML:
     tomldata: Mapping[Any, Any]
+    workspace_data: Mapping[Any, Any]
     check_cfg: Set[str]
 
-    def __init__(self, path: str):
+    def __init__(self, path: str, workspace: Optional[str]):
         with open(path, 'rb') as f:
             self.tomldata = tomllib.load(f)
 
+        if workspace is not None:
+            with open(workspace, 'rb') as f:
+                self.workspace_data = tomllib.load(f)
+            if "workspace" not in self.workspace_data:
+                self.workspace_data["workspace"] = {}
+
         self.check_cfg = set(self.find_check_cfg())
 
     def find_check_cfg(self) -> Iterable[str]:
@@ -54,10 +61,12 @@ def find_check_cfg(self) -> Iterable[str]:
 
     @property
     def lints(self) -> Mapping[Any, Any]:
-        return self.get_table("lints")
+        return self.get_table("lints", True)
 
-    def get_table(self, key: str) -> Mapping[Any, Any]:
+    def get_table(self, key: str, can_be_workspace: bool = False) -> Mapping[Any, Any]:
         table = self.tomldata.get(key, {})
+        if can_be_workspace and table.get("workspace", False) is True:
+            table = self.workspace_data["workspace"].get(key, {})
 
         return table
 
@@ -136,6 +145,16 @@ def main() -> None:
         action="store",
         dest="cargo_toml",
         help="path to Cargo.toml file",
+        nargs='?',
+    )
+    parser.add_argument(
+        "--workspace",
+        metavar="DIR",
+        action="store",
+        dest="workspace",
+        help="path to root of the workspace",
+        required=False,
+        default=None,
     )
     parser.add_argument(
         "--features",
@@ -168,7 +187,11 @@ def main() -> None:
     logging.debug("args: %s", args)
 
     rustc_version = tuple((int(x) for x in args.rustc_version.split('.')[0:2]))
-    cargo_toml = CargoTOML(args.cargo_toml)
+    if args.workspace:
+        workspace_cargo_toml = Path(args.workspace, "Cargo.toml").resolve()
+        cargo_toml = CargoTOML(args.cargo_toml, str(workspace_cargo_toml))
+    else:
+        cargo_toml = CargoTOML(args.cargo_toml, None)
 
     if args.lints:
         for tok in generate_lint_flags(cargo_toml):
-- 
2.47.0
Re: [RFC PATCH 05/11] rust: cargo: store desired warning levels in workspace Cargo.toml
Posted by Junjie Mao 1 week, 4 days ago
Paolo Bonzini <pbonzini@redhat.com> writes:

> An extra benefit of workspaces is that they allow to place lint level
> settings in a single Cargo.toml; the settings are then inherited by
> packages in the workspace.
>
> Correspondingly, teach rustc_build_args.py to get the unexpected_cfgs
> configuration from the workspace Cargo.toml.
>
> Note that it is still possible to allow or deny warnings per crate or
> module, via the #![] attribute syntax.  The rust/qemu-api/src/bindings.rs
> file is an example.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  meson.build                     |  8 +++++---
>  rust/Cargo.toml                 |  8 ++++++++
>  rust/hw/char/pl011/Cargo.toml   |  3 +++
>  rust/qemu-api-macros/Cargo.toml |  3 +++
>  rust/qemu-api/Cargo.toml        |  5 ++---
>  rust/qemu-api/meson.build       |  2 +-
>  scripts/rust/rustc_args.py      | 31 +++++++++++++++++++++++++++----
>  7 files changed, 49 insertions(+), 11 deletions(-)
>
[snip]
> diff --git a/rust/Cargo.toml b/rust/Cargo.toml
> index 0c94d5037da..0230b92a9fa 100644
> --- a/rust/Cargo.toml
> +++ b/rust/Cargo.toml
> @@ -5,3 +5,11 @@ members = [
>      "qemu-api",
>      "hw/char/pl011",
>  ]
> +
> +[workspace.lints.rust]
> +unexpected_cfgs = { level = "warn", check-cfg = [
> +    'cfg(MESON)', 'cfg(HAVE_GLIB_WITH_ALIGNED_ALLOC)',
> +    'cfg(has_offset_of)'] }
> +

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.

--
Best Regards
Junjie Mao
Re: [RFC PATCH 05/11] rust: cargo: store desired warning levels in workspace Cargo.toml
Posted by Paolo Bonzini 1 week, 4 days ago
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.

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.

Paolo

> --
> Best Regards
> Junjie Mao
>
>
Re: [RFC PATCH 05/11] rust: cargo: store desired warning levels in workspace Cargo.toml
Posted by Junjie Mao 1 week, 4 days ago
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
>