Do you think we should target this for 2.56? (Do you have time for that?)

(In reply to Philip Withnall from comment #1)
> Do you think we should target this for 2.56? (Do you have time for that?)

If somebody comes up to do the work, sure; at the moment, I don't have spare cycles for this.

Created attachment 366281 [details] [review]
gdbus-codegen: Move from optparse to argparse

The optparse module is deprecated since version 2.7 and the development continues with the argparse.

The code has been moved to parse command-line options from optparse to argparse.

Created attachment 366282 [details] [review]
gdbus-codegen: Split license string

The license string which is embedded in the C header and body preambles has been moved to a global variable. This way it can be reused in both sections.

Created attachment 366285 [details] [review]
gdbus-codegen: Add support for pragma inclusion guard

The #pragma once is widely supported preprocessor directive that can be used instead of include guards.

This adds support for using optionally this directive instead of include guards.

Created attachment 366292 [details] [review]
gdbus-codegen: Split C header and code generation functions

The class that generated both C header and code has been split into two classes. These clases are now specialized on creating the header or the body code.

All parameters that do not belong to each class have also been deleted, so only the necessary parameters still remain. These also includes the header and code file descriptors, leaving only the corresponding file descriptor necessary for each class.

Created attachment 366293 [details] [review]
gdbus-codegen: Split C header and code generation functions

(Sorry the last description was wrong, this is right)

The generation of the C header and code preambles have been split in order to be able to generate both files separately in the future.

The functions for generating preambles and postambles have also been renamed following the function names used in the glib-genmarshal rewrite, so that they stay consistent.

Created attachment 366294 [details] [review]
gdbus-codegen: Split C header and code generation

The class that generated both C header and code has been split into two classes. These clases are now specialized on creating the header or the body code.

All parameters that do not belong to each class have also been deleted, so only the necessary parameters still remain. These also includes the header and code file descriptors, leaving only the corresponding file descriptor necessary for each class.

Created attachment 366295 [details] [review]
gdbus-codegen: Support for separate C header and code generation

gdbus-codegen's options only allow a simultaneous header and code generation.

A `--header` and `--body` options have been added along the `--output` option which allow separate C header and code generation.

These options cannot be used in addition to the old options such as `--generate-c-code` which generates both files at the same time, `--generate-docbook` because it generates a bunch of files which need an output directory, or `--output-directory` because it would be missing the output name.

These set of six patches try to implement a way to generate independently the header and the source files. This work has been done in order to fix the issues in gvfs' meson build port (#789877).

I started modernizing a little bit the code, but I ended removing most of my new code to do the changes incrementally.

As always any suggestion would be very appreciated.

Created attachment 366331 [details] [review]
docs: Add new options to gdbus-codegen

I have forgotten to update the documentation regarding the new added options to `gdbus-codegen`.

This patch extends the documentation with the required information.

Just as a side note, this is also affecting NetworkManager:

https://github.com/mesonbuild/meson/issues/2893

Review of attachment 366281 [details] [review]:

Looks good.

Review of attachment 366282 [details] [review]:

Yup.

Review of attachment 366285 [details] [review]:

One nitpick; feel free to commit with that change.

::: gio/gdbus-2.0/codegen/codegen.py
@@ +233,3 @@
+            self.h.write('#ifndef __{!s}__\n'.format(self.header_guard))
+            self.h.write('#define __{!s}__\n'.format(self.header_guard))
+            self.h.write('\n')

The `self.h.write('\n')` could be hoisted out of the if-block and executed after it.

Review of attachment 366285 [details] [review]:

Actually, this needs to be documented in docs/reference/gio/gdbus-codegen.xml. Please do that and resubmit it for review.

Review of attachment 366293 [details] [review]:

Yup.

Review of attachment 366294 [details] [review]:

That would have been significantly easier to review had self.c and self.h not changed names. Reviewing with `git show --word-diff=color -b --word-diff-regex='[^[:space:]\(\)\.]' $commit_id` helped.

Review of attachment 366295 [details] [review]:

Please merge in the patch which adds the documentation for the new options too.

::: gio/gdbus-2.0/codegen/codegen_main.py
@@ +186,3 @@
+            args.output is not None):
+        sys.stderr.write('Using --generate-c-code or --generate-docbook and '
+                         '--outfile at the same time is not allowed')

s/--outfile/--output/

@@ +198,3 @@
+    elif args.header:
+        h_file = args.output
+        header_name = os.path.basename(h_file)

There’s no code above which requires args.output (as opposed to args.output_directory) to be set when args.header is set.

@@ +201,3 @@
+    elif args.body:
+        outdir = args.output_directory
+        c_file = args.output

args.output_directory and args.output are mutually exclusive, so one of these is always going to be None.

Review of attachment 366281 [details] [review]:

::: gio/gdbus-2.0/codegen/codegen_main.py
@@ -149,3 @@
-    arg_parser = optparse.OptionParser('%prog [options]')
-    arg_parser.add_option('', '--xml-files', metavar='FILE', action='append',
-                          help='D-Bus introspection XML file')

Actually, you can’t remove the --xml-files argument, as that will break backwards compatibility. However, it does make sense to make the FILE list positional. Probably the solution here is to allow --xml-files *or* a positional argument (and concatenate the two lists if both are provided).

Review of attachment 366331 [details] [review]:

Please merge this into the patch which introduces the options.

I’d also like some test coverage of this. Emmanuele was working on a test harness for some of the other tools (mkenums, genmarshal), so will have some ideas about how best to go about adding tests for the gdbus-codegen command line.

::: docs/reference/gio/gdbus-codegen.xml
@@ -36,3 @@
     <arg><option>--output-directory</option> <replaceable>OUTDIR</replaceable></arg>
     <arg><option>--generate-docbook</option> <replaceable>OUTFILES</replaceable></arg>
-    <arg><option>--xml-files</option> <replaceable>FILE</replaceable></arg>

The --xml-files argument should not be removed, although it can be marked as deprecated in the documentation if you add support for providing it as a positional argument (see my other reviews).

@@ +154,2 @@
     <varlistentry>
+      <term><option>--files</option> <replaceable>FILE</replaceable></term>

Make this just be positional. Document that --xml-files is a deprecated fallback for it.

@@ +259,3 @@
+          Using <option>--generate-c-code</option>, <option>--generate-docbook</option> or
+          <option>--output-directory</option> are not allowed to be used along
+          <option>--header</option> because this option is used to generate only one file.

s/along/along with/. You should probably also include --body in that list.

@@ +274,3 @@
+          Using <option>--generate-c-code</option>, <option>--generate-docbook</option> or
+          <option>--output-directory</option> are not allowed to be used along
+          <option>--body</option> because this option is used to generate only one file.

s/along/along with/. You should probably also include --header in that list.

@@ +283,3 @@
+      <listitem>
+        <para>
+          The full path were the header, by using the <option>--header</option>, or the source code,

s/were/where/. I’d probably rephrase the first sentence as:
```
The full path where the header (<option>--header</option>) or the source code (<option>--body</option>) will be written, using the path and filename provided by <option>--output</option>. The full path…
```

@@ +291,3 @@
+          Using <option>--generate-c-code</option>, <option>--generate-docbook</option> or
+          <option>--output-directory</option> are not allowed to be used along
+          <option>--output</option> because this option is used to generate only one file.

Rephrase as:
```
Using <option>--generate-c-code</option>, <option>--generate-docbook</option> or <option>--output-directory</option> is not allowed along with <option>--output</option>, because the latter is used to generate only one file.
```

Comment on attachment 366282 [details] [review]
gdbus-codegen: Split license string

Pushed as a3d223d0e - gdbus-codegen: Split license string

Created attachment 366555 [details] [review]
gdbus-codegen: Move from optparse to argparse

(In reply to Philip Withnall from comment #20)
> Review of attachment 366281 [details] [review] [review]:
> 
> ::: gio/gdbus-2.0/codegen/codegen_main.py
> @@ -149,3 @@
> -    arg_parser = optparse.OptionParser('%prog [options]')
> -    arg_parser.add_option('', '--xml-files', metavar='FILE',
> action='append',
> -                          help='D-Bus introspection XML file')
> 
> Actually, you can’t remove the --xml-files argument, as that will break
> backwards compatibility. However, it does make sense to make the FILE list
> positional. Probably the solution here is to allow --xml-files *or* a
> positional argument (and concatenate the two lists if both are provided).

Yes, that was my major issue. I couldn't make an on option positional and optional at the same time. I've also supposed by following all the examples I could find that nobody was using `--xml-files` option, but that's only a guess.

I have updated the path, following your hint. I'm using two options now, one positional and the other one optional, by merging their lists before iterating over them.

Created attachment 366557 [details] [review]
gdbus-codegen: Add support for pragma inclusion guard

(In reply to Philip Withnall from comment #15)
> Review of attachment 366285 [details] [review] [review]:
> The `self.h.write('\n')` could be hoisted out of the if-block and executed
> after it.

Thank you! I've overlooked that!

This patch fixes that issue (but it can't be pushed yet as it depends on attachment 366555 [details] [review]).

Created attachment 366597 [details] [review]
gdbus-codegen: Add support for pragma inclusion guard

The patch has been extended including the information regarding the new `--pragma-once` option in `docs/reference/gio/gdbus-codegen.xml`.

Created attachment 366631 [details] [review]
gdbus-codegen: Split C header and code generation

(In reply to Philip Withnall from comment #18)
> Review of attachment 366294 [details] [review] [review]:
> 
> That would have been significantly easier to review had self.c and self.h
> not changed names. Reviewing with `git show --word-diff=color -b
> --word-diff-regex='[^[:space:]\(\)\.]' $commit_id` helped.

I'm sorry about this. It's nice to know that the command has made the revision work easier.

I thought it was worth making the change, since it would make it clearer. I was also thinking about moving from `%` to `format` to modernize a little bit the code base, but finally I didn't.

Although the `pragma once` patch didn't affect attachment 366293 [details] [review], it affects slightly this patch, so here goes an update.

Review of attachment 366555 [details] [review]:

This looks good, thanks. Could you include changes to the documentation for gdbus-codegen in this patch, so that the documentation mentions the positional argument and the deprecation of --xml-files?

Review of attachment 366597 [details] [review]:

This looks good to commit once the patches it depends on are pushed. Thanks.

Review of attachment 366631 [details] [review]:

Still looks OK.

(In reply to Iñigo Martínez from comment #26)
> Created attachment 366631 [details] [review] [review]
> gdbus-codegen: Split C header and code generation
> 
> (In reply to Philip Withnall from comment #18)
> > Review of attachment 366294 [details] [review] [review] [review]:
> > 
> > That would have been significantly easier to review had self.c and self.h
> > not changed names. Reviewing with `git show --word-diff=color -b
> > --word-diff-regex='[^[:space:]\(\)\.]' $commit_id` helped.
> 
> I'm sorry about this. It's nice to know that the command has made the
> revision work easier.

This kind of thing can be made easier by splitting up the changes: I would have done one commit to split the generation functions (the changes in codegen_main.py + the class-level changes in codegen.py), and then a second commit which mechanically did s/self.outfile/self.c/ and s/self.outfile/self.h/. It would then have been possible to mechanically verify the second commit.

> I thought it was worth making the change, since it would make it clearer. I
> was also thinking about moving from `%` to `format` to modernize a little
> bit the code base, but finally I didn't.

A patch to switch from `%` to `format` would be useful, but let’s do that as a follow-up once all these patches have landed. :-)

Created attachment 366703 [details] [review]
gdbus-codegen: Use Color's print_* methods

When writing the deprecation messages for the `--xml-files` parameter, I've taken a look at what `glib-genmarshal` and `glib-mkenum` do. They are using an utility class called Color which implements a number of print_* methos that print colored messages to the standard error output.

I have copied that same implementation to the `gdbus-codegen` utilities and replaced all the `RuntimeErrors` with the `print_error` method.

If this patch gets merged, my idea is to build the rest of the patches over this one, that would help for example writing deprecation warnings when using the `--xml-files` by using these same print functions. This way the output would remain consistent between the different programs.

Created attachment 366731 [details] [review]
gdbus-codegen: Move from optparse to argparse

I've had some spare time and I've updated the patches over the Color's print_* methods.

This patch for example has been updated to use the `print_warning` method to print the `--xml-files` deprecation message.

Created attachment 366734 [details] [review]
gdbus-codegen: Remove unnecessary parameters from the constructor

(In reply to Philip Withnall from comment #19)
> Review of attachment 366295 [details] [review] [review]:
> @@ +201,3 @@
> +    elif args.body:
> +        outdir = args.output_directory
> +        c_file = args.output
> 
> args.output_directory and args.output are mutually exclusive, so one of
> these is always going to be None.

When it arrives here, the `output_directory` parameter is set to its default value, which is just an empty string. I was setting the `outdir` parameter with `output_directory's` default value because it was needed by the `DocbookCodeGenerator's` constructor, even if the docbook was not generated.

I ended fixing `DocbookCodeGenerator` class properly, by removing the parameters that might not be used (`outdir` and `docbook`), and that are now parameters for the `generate` method.

This patch implements this fix.

Created attachment 366735 [details] [review]
gdbus-codegen: Support for separate C header and code generation

I have updated the patch with your comments:

- The `outfile` typo has been fixed.
- The documentation has been updated with your notes.
- If the `--output` parameter has not been set when using the `--header` or `--body` parameters, it will show an error message and exit.

Hope it's better now :)

Comment on attachment 366331 [details] [review]
docs: Add new options to gdbus-codegen

It has been merged in attachment 366735 [details] [review].

Review of attachment 366731 [details] [review]:

::: docs/reference/gio/gdbus-codegen.xml
@@ +67,2 @@
     <option>--generate-c-code</option>) and Docbook XML (via
+    <option>--generate-docbook</option>a).

Spurious ‘a’ after `</option>`.

@@ +158,3 @@
         <para>
+          This option is deprecated; use positional arguments instead.
+          The D-Bus introspection XML file. This option is deprecated;

Spurious additional ‘This option is deprecated;’.

Review of attachment 366734 [details] [review]:

Sure.

Review of attachment 366735 [details] [review]:

Sure. Emmanuele, how does this look to you?

Review of attachment 366703 [details] [review]:

OK.

Created attachment 366833 [details] [review]
gdbus-codegen: Move from optparse to argparse

(In reply to Philip Withnall from comment #35)
> Review of attachment 366731 [details] [review] [review]:
> Spurious ‘a’ after `</option>`.
> 
> Spurious additional ‘This option is deprecated;’.

These are fixed now.

Review of attachment 366833 [details] [review]:

OK, thanks.

Review of attachment 366735 [details] [review]:

Looks good to me

Review of attachment 366735 [details] [review]:

Let’s go!

Comment on attachment 366703 [details] [review]
gdbus-codegen: Use Color's print_* methods

Pushed as dcc1fe09d - gdbus-codegen: Use Color's print_* methods

Comment on attachment 366833 [details] [review]
gdbus-codegen: Move from optparse to argparse

Pushed as e59bce3c7 - gdbus-codegen: Move from optparse to argparse

Comment on attachment 366597 [details] [review]
gdbus-codegen: Add support for pragma inclusion guard

Pushed as c658d03b7 - gdbus-codegen: Add support for pragma inclusion guard

Comment on attachment 366293 [details] [review]
gdbus-codegen: Split C header and code generation functions

Pushed as a66f2f80e - gdbus-codegen: Split C header and code generation functions

Comment on attachment 366631 [details] [review]
gdbus-codegen: Split C header and code generation

Pushed as 22772acff - gdbus-codegen: Split C header and code generation

Comment on attachment 366734 [details] [review]
gdbus-codegen: Remove unnecessary parameters from the constructor

Pushed as 6c3af1cdd - gdbus-codegen: Remove unnecessary parameters from the constructor

Comment on attachment 366735 [details] [review]
gdbus-codegen: Support for separate C header and code generation

Pushed as e4d68c7b3 - gdbus-codegen: Support for separate C header and code
 generation

Everything is pushed now, so I'm closing the issue. Please, let me know if any issue arises from these changes and thank you for your time and effort reviewing them.

It seems that commit e4d68c7b3 breaks gnome-shell build when gtk-doc is enabled.

[1/3] gdbus-codegen '--interface-prefix=['"'"'org.gnome.Shell.Screenshot'"'"', '"'"'org.gnome.Shell.Screenshot.xml'"'"'].' --generate-docbook doc-gen --output-directory docs/reference/shell ../../source/gnome-shell/docs/reference/shell/../../../data/org.gnome.Shell.Screenshot.xml
FAILED: docs/reference/shell/doc-gen-org.gnome.Shell.Screenshot.xml 
gdbus-codegen '--interface-prefix=['"'"'org.gnome.Shell.Screenshot'"'"', '"'"'org.gnome.Shell.Screenshot.xml'"'"'].' --generate-docbook doc-gen --output-directory docs/reference/shell ../../source/gnome-shell/docs/reference/shell/../../../data/org.gnome.Shell.Screenshot.xml
ERROR: Using --header or --body requires --output
[2/3] gdbus-codegen '--interface-prefix=['"'"'org.gnome.ShellSearchProvider'"'"', '"'"'org.gnome.Shell.SearchProvider.xml'"'"'].' --generate-docbook doc-gen --output-directory docs/reference/shell ../../source/gnome-shell/docs/reference/shell/../../../data/org.gnome.ShellSearchProvider.xml
FAILED: docs/reference/shell/doc-gen-org.gnome.Shell.SearchProvider.xml 
gdbus-codegen '--interface-prefix=['"'"'org.gnome.ShellSearchProvider'"'"', '"'"'org.gnome.Shell.SearchProvider.xml'"'"'].' --generate-docbook doc-gen --output-directory docs/reference/shell ../../source/gnome-shell/docs/reference/shell/../../../data/org.gnome.ShellSearchProvider.xml
ERROR: Using --header or --body requires --output
[3/3] gdbus-codegen '--interface-prefix=['"'"'org.gnome.ShellSearchProvider2'"'"', '"'"'org.gnome.Shell.SearchProvider2.xml'"'"'].' --generate-docbook doc-gen --output-directory docs/reference/shell ../../source/gnome-shell/docs/reference/shell/../../../data/org.gnome.ShellSearchProvider2.xml
FAILED: docs/reference/shell/doc-gen-org.gnome.Shell.SearchProvider2.xml 
gdbus-codegen '--interface-prefix=['"'"'org.gnome.ShellSearchProvider2'"'"', '"'"'org.gnome.Shell.SearchProvider2.xml'"'"'].' --generate-docbook doc-gen --output-directory docs/reference/shell ../../source/gnome-shell/docs/reference/shell/../../../data/org.gnome.ShellSearchProvider2.xml
ERROR: Using --header or --body requires --output
ninja: build stopped: cannot make progress due to previous errors.

Neither '--header' nor '--body' is used, but gdbus-codegen still says 'Using --header or --body requires --output'.

Created attachment 367382 [details] [review]
gdbus-codegen: Fix issue with docbook generation

(In reply to Ting-Wei Lan from comment #51)
> It seems that commit e4d68c7b3 breaks gnome-shell build when gtk-doc is
> enabled.

Totally my fault. I assumed that, in addition to the docbook generation, the header or source code were always generated.

This patch fixes this issue, which makes header or source code generation optional. I have fully build and tested glib and all the tests were successful. I'm going to also build gnome-shell with buildstream, but the fact that libgit2-glib building has been like 3 hours in my computer, makes me wonder when I'll finish it.

Attachment 367382 [details] still doesn't work:

Traceback (most recent call last):

+ Trace 238365

I am not sure why you have to build libgit2-glib before building gnome-shell and why it can take 3 hours. It seems to me that the only thing in GNOME modulesets that can take more than one hour to build is WebKit.

Created attachment 367393 [details] [review]
gdbus-codegen: Fix issue with docbook generation

(In reply to Ting-Wei Lan from comment #53)
> Attachment 367382 [details] still doesn't work:

Another mistake. Yes, the docbook generation also needs to set the `outdir` directory, because it might generate more than one file.

I'm very sorry for the inconveniences :(

> I am not sure why you have to build libgit2-glib before building gnome-shell and why it can take 3 hours. It seems to me that the only thing in GNOME modulesets that can take more than one hour to build is WebKit.

Oh! you are right, they are totally unrelated things. It's just that I was working on `libgit2-glib` and I tried to build it using buildstream :)

It might be my computer (which is a bit old), but buildstream is terribly slow here.

Attachment 367393 [details] works for me. gnome-shell can be successfully built.

Review of attachment 367393 [details] [review]:

The documentation (gdbus-codegen.xml) needs to be updated to mention that --output-directory is mandatory when using --generate-docbook.

::: gio/gdbus-2.0/codegen/codegen_main.py
@@ +213,3 @@
+        header_name = os.path.splitext(c_file)[0] + '.h'
+    else:
+        outdir = args.output_directory

Is there anything which actually checks that args.output_directory is set here? As far as I can see, nothing stops you calling `gdbus-codegen --generate-docbook --output blah`, which will fail here.

It also seems like it would be simpler to get rid of the `outdir` variable and just use `args.output_directory` instead.

Created attachment 367420 [details] [review]
gdbus-codegen: Fix issue with docbook generation

(In reply to Philip Withnall from comment #56)
> Review of attachment 367393 [details] [review] [review]:
> 
> The documentation (gdbus-codegen.xml) needs to be updated to mention that
> --output-directory is mandatory when using --generate-docbook.
> 
> ::: gio/gdbus-2.0/codegen/codegen_main.py
> @@ +213,3 @@
> +        header_name = os.path.splitext(c_file)[0] + '.h'
> +    else:
> +        outdir = args.output_directory
> 
> Is there anything which actually checks that args.output_directory is set
> here? As far as I can see, nothing stops you calling `gdbus-codegen
> --generate-docbook --output blah`, which will fail here.

If `--generate-docbook` or `--generate-c-code` is used along `--output`, it will show an error[0]. `--outdir` will also have an empty string as default value[1], so if the option is not used, the files will be created on the working directory.

> It also seems like it would be simpler to get rid of the `outdir` variable
> and just use `args.output_directory` instead.

You are right, I've also removed the `outdir` variable when `--geenrate-c-code` is used, becuase it would only be used to avoid using `args.output_directory` just twice.

[0] https://git.gnome.org/browse/glib/tree/gio/gdbus-2.0/codegen/codegen_main.py#n190
[1] https://git.gnome.org/browse/glib/tree/gio/gdbus-2.0/codegen/codegen_main.py#n182

Review of attachment 367420 [details] [review]:

The documentation still needs to be updated for --generate-docbook. You’re right that --output-directory is not mandatory for --generate-docbook (my mistake, sorry), but it should still be mentioned in the documentation. Something like “Pass --output-directory to specify the directory to put the output files in. By default the current directory will be used.” (with appropriate DocBook markup). Thanks.

Created attachment 367512 [details] [review]
gdbus-codegen: Fix issue with docbook generation

I have changed the link, which was pointing to a comment instead of the bug itself.

Created attachment 367513 [details] [review]
gdbus-codegen: Improve documentation

(In reply to Philip Withnall from comment #58)
> Something like “Pass --output-directory to specify the directory to put the
> output files in. By default the current directory will be used.” (with
> appropriate DocBook markup). Thanks.

I have included your comment (thanks!), but I have finally made a different commit because I tried to improve also the documentation regarding `--header` and `--body` options.

Hope it's in better state now.

Review of attachment 367512 [details] [review]:

++

Review of attachment 367513 [details] [review]:

A few typos and then this one’s good, thanks.

::: docs/reference/gio/gdbus-codegen.xml
@@ +92,3 @@
+    For C code generation either <option>--body</option> that
+    generates source code, or <option>--header</option> that
+    generates header, can be used. These options must be used along

‘generates a header’

@@ +93,3 @@
+    generates source code, or <option>--header</option> that
+    generates header, can be used. These options must be used along
+    <option>--output</option>, which is used to pass the file used

‘along with’

@@ +94,3 @@
+    generates header, can be used. These options must be used along
+    <option>--output</option>, which is used to pass the file used
+    as output.

s/to pass the file used as output/to specify the file to output to/

Review of attachment 367513 [details] [review]:

I’ll just make these changes myself and push.

Both pushed to master, with the wording tweaks done. Thanks!

Attachment 367512 [details] pushed as 93042e0 - gdbus-codegen: Fix issue with docbook generation
Attachment 367513 [details] pushed as 3ef8361 - gdbus-codegen: Improve documentation