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

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

[guilherme giacomo simoes]

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

[Miguel Ojeda]

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] rust: macros: add authors

[guilherme giacomo simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrotes: 
> 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.

I maked this change like this way because in my thoughts I just thought about:
- A single field, that only accepts a list.
- Two fields (author and authors).

And how in the first option I would need found all modules that have the author
tags and I would need change the code (from `author: "author"` to `author:
["author"])`, I was think that is the best don't change in a thing that is
already work well. And this is why I choose the second option "Two field". And
if the new developers need include two or more authors, he can make this since
now.

But now, that you put the option "A single field that accepts both a string or
a list" for me this make sense too. I think that the A single field that
accepts both a string  or a list, and two fields (author and authors) is the
two best options. 
I don't like the options:
- A single field, that only accepts a list. 
Because we need found all module
that use the author and change his implementation.

- Accepting several "author" fields and append them all into a list. 
because in the modules the developers would need a several author field if the 
module is development for several author. And the module! macro would grow a lot. 
Like:
  author: guilherme, 
  author: miguel, 
  author: another_poor_developer, 
  .... 


> > -        for alias in aliases {
> > -            modinfo.emit("alias", &alias);
> > -        }
> > +        modinfo.emit_arr_str("alias", &aliases);
> 
> Spurious change? Or am I missing something?
I make this change because, the for() would need repeat for the alias and for
the authors and for avoid unnecessary code repetition I create the
emit_arr_str() function.

> 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.
.. 
> Also, this patch should update the documentation of the macro.
About this, I will sent a v2 PATCH with this informations

Re: [PATCH] rust: macros: add authors

[Miguel Ojeda]

On Fri, Dec 6, 2024 at 10:19 PM guilherme giacomo simoes
<trintaeoitogc@gmail.com> wrote:
>
> And how in the first option I would need found all modules that have the author
> tags and I would need change the code (from `author: "author"` to `author:
> ["author"])`, I was think that is the best don't change in a thing that is
> already work well.

I understand what you mean, but changing a few existing lines here and
there is fine. There aren't that many modules out there. If we are
going to change this, then the best time is now. Backporting should
not be too bad either.

In any case, in general, I would focus in deciding what we want to end
up with, and then we can discuss how to get there (assuming there was
a problem getting there).

> - Accepting several "author" fields and append them all into a list.
> because in the modules the developers would need a several author field if the
> module is development for several author. And the module! macro would grow a lot.
> Like:
>   author: guilherme,
>   author: miguel,
>   author: another_poor_developer,
>   ....

Sorry, I am not sure I understand what you mean. Yes, the idea of this
option is to repeat one line per author, but that is not a big deal: C
modules do that already.

> I make this change because, the for() would need repeat for the alias and for
> the authors and for avoid unnecessary code repetition I create the
> emit_arr_str() function.

In general, it is best to mention it in the commit message or, even
better, do the cleanup in another patch. Having said that, I am not
sure I would introduce the function -- we are not really saving lines
and it is yet one more function to read.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: macros: add authors

[guilherme giacomo simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrotes:
> I understand what you mean, but changing a few existing lines here and
> there is fine. There aren't that many modules out there. If we are
> going to change this, then the best time is now. Backporting should
> not be too bad either.
Yeah, in the truth, don't have file a lot for changes.

> Sorry, I am not sure I understand what you mean. Yes, the idea of this
> option is to repeat one line per author, but that is not a big deal: C
> modules do that already.
I understand that in the C side is already this way, but maybe we have a better
way for make this.

I'm think about this, and for me, the less the developer need a think about
"what field I need use in this problem" is better. This is not because is really
difficult think about "I need use author or authors", but if we can avoid, I
think that is better. 

Well .. that said, after your tips, I change my mind and how we don't have file
a lot for change the type authors, maybe the better way is a unique field
called "authors" and this should ever be array. 

If my module have a unique author, so `authors: ["author"]`, if the module have
two or more authors, so `authors: ["author_1", "author_2", "author_n"]`. With
this way, the module developers just have a one way to declare the author of
module. Don't have different fields, don't have different types. Have a only
one way for declare the authors of module.  

Thoughts? 
You think that we need open a chat in zulipchat for this, and collect more
opinions?

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

[Daniel Sedlak]

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] rust: macros: add authors

[guilherme giacomo simoes]

Daniel Sedlak <daniel@sedlak.dev> wrote:
> 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.
It seems to me that the rustfmt.toml in the kernel, don't have a max width for
line. Are you sure that the rustfmt would broke the line for big enough lines?

> 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.
If you (daniel and miguel) are ok with repeat the `author` field and think that
this is the better option I is happy to make this change. 

I was run the follow command:
grep -rwo 'MODULE_AUTHOR' . | awk -F: '{count[$1]++} END {for (file in count) if (count[file] > 1) print file, count[file]}' | sort -k2 -n > res
for found the modules with more than one MODULE_AUTHOR.
I see that the maximum of MODULE_AUTHOR that is contains in a module is 11. The
marjority have 2 MODULE_AUTHOR. Maybe, repeat the `author` field, is don't a
bad idea.

Thoughs?

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

[kernel test robot]

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

[Miguel Ojeda]

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

Re: [PATCH] rust: macros: add authors

[Daniel Sedlak]

On 12/7/24 5:07 PM, guilherme giacomo simoes wrote:
> Daniel Sedlak <daniel@sedlak.dev> wrote:
>> 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.
> It seems to me that the rustfmt.toml in the kernel, don't have a max width for
> line. Are you sure that the rustfmt would broke the line for big enough lines?

That is not what I meant. See [1] or [2] as an example (there are plenty 
of those cases).

Link: https://github.com/rust-lang/rustfmt/discussions/5437 [1]
Link: https://users.rust-lang.org/t/rustfmt-skips-macro-arguments/74807 [2]
> 
>> 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.
> If you (daniel and miguel) are ok with repeat the `author` field and think that
> this is the better option I is happy to make this change.

I am Ok with repeating the field, so I would vote for that. However if 
Miguel thinks that it is a bad idea, I will not contest that.
> 
> I was run the follow command:
> grep -rwo 'MODULE_AUTHOR' . | awk -F: '{count[$1]++} END {for (file in count) if (count[file] > 1) print file, count[file]}' | sort -k2 -n > res
> for found the modules with more than one MODULE_AUTHOR.
> I see that the maximum of MODULE_AUTHOR that is contains in a module is 11. 

Thank you for posting your results and working on that.

Daniel

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

[Daniel Sedlak]

On 12/9/24 12:47 PM, Miguel Ojeda wrote:
> 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).

Ops, it did not occur to me, that we already have lists. I would propose 
to migrate them too, however it may be controversial.
> 
> We could also try to mitigate the formatting issue via e.g.
> `checkpatch.pl` if needed.

That is true, it could be part of `checkpatch.pl`, however I would argue 
that if we can overcame the formatting problems by repeating the field, 
instead of modifying `checkpatch.pl`, then none code is better than some 
code (regarding modifying `checkpatch.pl`).

Thank you for feedback

Daniel

Re: [PATCH] rust: macros: add authors

[guilherme giacomo simoes]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 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.
It is true, we already have the formatting problem with `alias` and `firmware`
fields.
If we will follow this logic (doing equal in C side repeating fields), maybe we
can end have the lines a lot for modules that have a `alias` a lot, or
`firmaware` a lot. Two example about this is the `sound/oao/fabrics/layout.c`
that have the 38 MODULE_ALIAS and `drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c` that
have 77 MODULE_FIRMWARE.

Maybe, have something like:
	authors: ["author", "author", "author", "author", 
		"author", "author", "author", "author", "author"]

and having a check on checkpath.pl doesn't seem like a bad idea. Because for me
it's better than:

	author: "author",
	author: "author",
	author: "author",
	author: "author",
	author: "author",
	author: "author",
	author: "author",
	author: "author",
	author: "author"

due to the fact that we spend more time scrolling the screen code than
programming.

But I don't sure. 

Thanks and regards,
Guilherme

Re: [PATCH] rust: macros: add authors

[guilherme giacomo simoes]

Daniel Sedlak <daniel@sedlak.dev> wrote:
> That is true, it could be part of `checkpatch.pl`, however I would argue 
> that if we can overcame the formatting problems by repeating the field, 
> instead of modifying `checkpatch.pl`, then none code is better than some 
> code (regarding modifying `checkpatch.pl`).
I understand, but if we choose have a multiples author, firmware and alias
fields, we need modify some codes in `module.rs`, and I think that in the end
is the same work, because if we choose have a multiples fields we need change
modify the module.rs and if we choose have a array we need modify the
checkpatch.pl... 

It seems for me is the same work.

Unless you convince me otherswise, I think that is the best thing is to
maintain this as a array, and avoid boring scrolling screen.

On the other hand, I understand that the module that have a author, alias or
firmware a lot is the minority, and the marjority modules we don't need
scrolling screen a lot.

Thanks and regards,
guilherme