rust: macros: add authors

[PATCH] [PATCH] rust: macros: add authors

Posted by guilherme giacomo simoes 1 year ago

The module is only accepted to have a single author. If the module needs
more than one author, you cannot define this in the module creating
flow.
Add another key in the module stream that accepts a string array with
authors.
Author and authors keys cannot coexist, so add a check that if the
module authors addss these two keys, throw a panic!

Signed-off-by: guilherme giacomo simoes <trintaeoitogc@gmail.com>
---
 rust/macros/module.rs | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 2587f41b0d39..a6965354824f 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -83,6 +83,12 @@ fn emit_only_loadable(&mut self, field: &str, content: &str) {
         self.emit_base(field, content, false)
     }
 
+    fn emit_arr_str(&mut self, field: &str, arr_content: &Vec<String>) {
+        for content in arr_content {
+            self.emit(field, content);
+        }
+    }
+
     fn emit(&mut self, field: &str, content: &str) {
         self.emit_only_builtin(field, content);
         self.emit_only_loadable(field, content);
@@ -95,12 +101,30 @@ struct ModuleInfo {
     license: String,
     name: String,
     author: Option<String>,
+    authors: Option<Vec<String>>,
     description: Option<String>,
     alias: Option<Vec<String>>,
     firmware: Option<Vec<String>>,
 }
 
 impl ModuleInfo {
+    fn coexist(arr: &mut Vec<String>, cannot_coexist: &[&str]) -> bool {
+        let mut found: bool = false;
+
+        for cc in cannot_coexist {
+
+            for key in &mut *arr {
+                if cc == key {
+                    if found {
+                        return true;
+                    }
+                    found = true;
+                }
+            }
+        }
+        return false;
+    }
+
     fn parse(it: &mut token_stream::IntoIter) -> Self {
         let mut info = ModuleInfo::default();
 
@@ -108,6 +132,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
             "type",
             "name",
             "author",
+            "authors",
             "description",
             "license",
             "alias",
@@ -136,6 +161,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
                 "type" => info.type_ = expect_ident(it),
                 "name" => info.name = expect_string_ascii(it),
                 "author" => info.author = Some(expect_string(it)),
+                "authors" => info.authors = Some(expect_string_array(it)),
                 "description" => info.description = Some(expect_string(it)),
                 "license" => info.license = expect_string_ascii(it),
                 "alias" => info.alias = Some(expect_string_array(it)),
@@ -153,6 +179,16 @@ fn parse(it: &mut token_stream::IntoIter) -> Self {
 
         expect_end(it);
 
+        const CANNOT_COEXIST: &[&[&str]] = &[
+            &["author", "authors"]
+        ];
+
+        for cannot_coexist in CANNOT_COEXIST {
+            if Self::coexist(&mut seen_keys, cannot_coexist) {
+                panic!("keys {:?} coexist", cannot_coexist);
+            }
+        }
+
         for key in REQUIRED_KEYS {
             if !seen_keys.iter().any(|e| e == key) {
                 panic!("Missing required key \"{}\".", key);
@@ -183,6 +219,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
     let info = ModuleInfo::parse(&mut it);
 
     let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
+    if let Some(authors) = info.authors {
+        modinfo.emit_arr_str("author", &authors);
+    }
     if let Some(author) = info.author {
         modinfo.emit("author", &author);
     }
@@ -191,9 +230,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
     }
     modinfo.emit("license", &info.license);
     if let Some(aliases) = info.alias {
-        for alias in aliases {
-            modinfo.emit("alias", &alias);
-        }
+        modinfo.emit_arr_str("alias", &aliases);
     }
     if let Some(firmware) = info.firmware {
         for fw in firmware {
-- 
2.34.1

Re: [PATCH] [PATCH] rust: macros: add authors

Posted by kernel test robot 1 year ago

Hi guilherme,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rust/rust-next]
[also build test WARNING on linus/master v6.13-rc1 next-20241206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/guilherme-giacomo-simoes/rust-macros-add-authors/20241207-032454
base:   https://github.com/Rust-for-Linux/linux rust-next
patch link:    https://lore.kernel.org/r/20241206192244.443486-1-trintaeoitogc%40gmail.com
patch subject: [PATCH] [PATCH] rust: macros: add authors
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20241207/202412070956.2hbXpu7r-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241207/202412070956.2hbXpu7r-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412070956.2hbXpu7r-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> warning: unneeded `return` statement
   --> rust/macros/module.rs:125:9
   |
   125 |         return false;
   |         ^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
   = note: `-W clippy::needless-return` implied by `-W clippy::all`
   = help: to override `-W clippy::all` add `#[allow(clippy::needless_return)]`
   help: remove `return`
   |
   125 -         return false;
   125 +         false
   |

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] [PATCH] rust: macros: add authors

Posted by Miguel Ojeda 1 year ago

On Fri, Dec 6, 2024 at 8:22 PM guilherme giacomo simoes
<trintaeoitogc@gmail.com> wrote:
>
> The module is only accepted to have a single author. If the module needs
> more than one author, you cannot define this in the module creating
> flow.
> Add another key in the module stream that accepts a string array with
> authors.
> Author and authors keys cannot coexist, so add a check that if the
> module authors addss these two keys, throw a panic!

Thanks for the patch!

There are several ways we could do this:

  - A single field, that only accepts a list.

  - A single field that accepts both a string or a list.

  - Two fields like this (that cannot coexist).

  - Accepting several "author" fields and append them all into a list.

Any thoughts on what is best? Could you please describe why you picked
the one you picked? (Ideally in the commit message). For instance, the
first one is e.g. what Cargo does and is the simplest, though slightly
annoying in the most common case of a single author. I wouldn't mind
it though, since this is likely copy-pasted from file to file anyway.

In addition, there was a PR [1] by Wayne (Cc'd) that implemented the
first approach, but it was never sent to the list. I pinged in the
GitHub issue too.

[1] https://github.com/Rust-for-Linux/linux/pull/904

> Signed-off-by: guilherme giacomo simoes <trintaeoitogc@gmail.com>

Ideally add (before the Signed-off-by) a Link: and Suggested-by: tag.

> -        for alias in aliases {
> -            modinfo.emit("alias", &alias);
> -        }
> +        modinfo.emit_arr_str("alias", &aliases);

Spurious change? Or am I missing something?

Also, this patch should update the documentation of the macro.

Finally, the title has an extra "[PATCH]" prefix.

Cheers,
Miguel

Re: [PATCH] [PATCH] rust: macros: add authors

Posted by Daniel Sedlak 1 year ago

On 12/6/24 9:17 PM, Miguel Ojeda wrote:
> There are several ways we could do this:
> 
>    - A single field, that only accepts a list.
> 
>    - A single field that accepts both a string or a list.

Since module is a macro, if we would allow syntax in the macro like:

	authors: ["author1", "author2", ...]

I think we could fight with the code formatting, because when it comes 
to the rust macros, rustfmt is often very confused and we could end up 
with variations like:

	authors: ["author1", "author2",
			"author3"]

or

	authors: [
		   "author1",
		   "author2",
		  ]

and rustfmt would be totally ok with both of them.

> 
>    - Two fields like this (that cannot coexist).
> 
>    - Accepting several "author" fields and append them all into a list.
> 

I think accepting several "author" fields is the best one because it 
mirrors the C API, where in C when you want to specify more authors you 
just repeat the MODULE_AUTHOR("author<N>") macro.

Re: [PATCH] [PATCH] rust: macros: add authors

Posted by Miguel Ojeda 1 year ago

On Sat, Dec 7, 2024 at 11:15 AM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> I think we could fight with the code formatting, because when it comes
> to the rust macros, rustfmt is often very confused and we could end up
> with variations like:
>
>         authors: ["author1", "author2",
>                         "author3"]
>
> or
>
>         authors: [
>                    "author1",
>                    "author2",
>                   ]
>
> and rustfmt would be totally ok with both of them.

Yeah, that is a good point. There are hundreds of drivers with 2+
authors, so this could indeed be an issue eventually.

Having said that, we already have e.g. the `alias` and `firmware` keys
that take a list, so I think we already have the potential issue, thus
being consistent in our use of lists sounds simpler (unless we are
discussing migrating those away too).

We could also try to mitigate the formatting issue via e.g.
`checkpatch.pl` if needed.

Thanks!

Cheers,
Miguel