GNOME Bugzilla – Bug 778801
gdbus-codegen: Add --outdir flag
Last modified: 2017-02-27 11:44:41 UTC
Created attachment 346035 [details] [review] gdbus-codegen: Add --outdir flag This is useful with Meson where files are generated in subdirs. https://github.com/mesonbuild/meson/issues/1387
Review of attachment 346035 [details] [review]: ::: gio/gdbus-2.0/codegen/codegen.py @@ +37,3 @@ self.h = h self.c = c + self.header_name = header_name Couldn’t you drop the `header_name` parameter and have it equivalent to `os.path.basename(self.h.name)`?
Review of attachment 346035 [details] [review]: ::: gio/gdbus-2.0/codegen/codegen_main.py @@ +165,3 @@ help='Add annotation (may be used several times)') + arg_parser.add_option('', '--outdir', metavar='OUTDIR', default='', + help='Location to output generated files') Also, I think the abbreviation is unnecessary, and it should be `--output-directory` (you can always add `-o` as a short version). Secondly, the new option should be documented in docs/reference/gio/gdbus-codegen.xml, which becomes the man page.
(In reply to Philip Withnall from comment #1) > Review of attachment 346035 [details] [review] [review]: > > ::: gio/gdbus-2.0/codegen/codegen.py > @@ +37,3 @@ > self.h = h > self.c = c > + self.header_name = header_name > > Couldn’t you drop the `header_name` parameter and have it equivalent to > `os.path.basename(self.h.name)`? I didn't want to break existing build scripts and it is valid to have a subdir as part of the headers path.
Created attachment 346116 [details] [review] gdbus-codegen: Add --output-directory flag
Review of attachment 346116 [details] [review]: With the following this this would be fine as far as I’m concerned, but it still needs to be acked by a GLib maintainer. Matthias? ::: docs/reference/gio/gdbus-codegen.xml @@ +236,3 @@ + <listitem> + <para> + Directory to output generated source to. I think it would be worthwhile pointing out that: • this is essentially the equivalent of using `cd $OUTDIR` before running `gdbus-codegen`; • a path fragment may be specified in `--generate-c-code`, which will be appended to $OUTDIR; and • the path fragment in `--generate-c-code` is what ends up in the #includes. Otherwise it’s not clear from the documentation how the two paths interact, or even that `--generate-c-code` can include path fragments.
Created attachment 346159 [details] [review] gdbus-codegen: Add --output-directory flag Add further clarification to docs.
Created attachment 346160 [details] [review] gdbus-codegen: Add --output-directory flag Fix typo...
Review of attachment 346160 [details] [review]: ::: docs/reference/gio/gdbus-codegen.xml @@ +192,3 @@ + This may include sub-directories but note that this path will be used to reference + the header file. If you want the files output in another location see + <option>--output-directory</option>. To me this paragraph implies that the path components in $OUTFILES will be ignored when choosing the output file, and will *only* be used for #includes. I think it would help to explicitly mention #include paths = $(dirname $OUTFILES) + $(basename $OUTFILES) + '.h'; and output path = $OUTDIR + $(dirname $OUTFILES).
Created attachment 346417 [details] [review] gdbus-codegen: Add --output-directory flag More docs clarification.
Review of attachment 346417 [details] [review]: Fundamentally sound. ::: docs/reference/gio/gdbus-codegen.xml @@ +190,3 @@ <filename>OUTFILES.c</filename> and + <filename>OUTFILES.h</filename> including any sub-directories. If you want the files to + be output in a different location use <option>--output-directory</option> as OUTFILE.h s/OUTFILE.h/<filename>OUTFILES.h</filename>/ @@ +191,3 @@ + <filename>OUTFILES.h</filename> including any sub-directories. If you want the files to + be output in a different location use <option>--output-directory</option> as OUTFILE.h + including sub-directories will be referenced from the c file. s/the c file/<filename>OUTFILES.c</filename>/ @@ +194,3 @@ + </para> + <para> + The full paths would then be <literal>"$(OUTDIR)/$(dirname $OUTFILES)/$(basename $OUTFILES).{c,h}"</literal> No need for the double quotes here. Missing a trailing period.
Created attachment 346495 [details] [review] gdbus-codegen: Add --output-directory flag More doc changes.
Review of attachment 346495 [details] [review]: ::: docs/reference/gio/gdbus-codegen.xml @@ +191,3 @@ + <filename>OUTFILES.h</filename> including any sub-directories. If you want the files to + be output in a different location use <option>--output-directory</option> as <filename>OUTFILE.h</filename> + including sub-directories will be referenced from <filename>OUTFILE.c</filename>. <filename>OUTFILES.h</filename> and <filename>OUTFILES.c</filename> (note the ‘S’), since that’s what you’ve defined the replaceable variable as above.
Created attachment 346602 [details] [review] gdbus-codegen: Add --output-directory flag Final doc changes?
Review of attachment 346602 [details] [review]: Yup, that looks good to me. Just need approval from someone like Matthias or Allison who are actually GLib maintainers.
Review of attachment 346602 [details] [review]: thanks for all the review help, Philip
Comment on attachment 346602 [details] [review] gdbus-codegen: Add --output-directory flag pushed as ee09bb7 - gdbus-codegen: Add --output-directory flag