Bug 70877 – [GTK] [WebKit] Replace the gtkdoc autools magic with something more flexible

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

The gtkdoc autotools magic a bit inflexible.

1. To make it build in the build directory (as opposed to in the source directory) we have to fork the generated makefile. See: http://trac.webkit.org/changeset/97901
2. it would be nice to be able to run the fast parts of gtkdoc during every build so that we would sniff out documentation warnings ealier. The slow part of gtkdoc is gtkdoc-mkhtml.
3. gtkdoc is yet another thing that ties us to autotools. While we don't plan to move away from autotools any time soon, it would be nice to avoid a hard dependency on it.
4. The gtkdoc makefile doesn't do any error reporting and it's hard to debug it when things go wrong.

This patch proposes introducing a python script to run the six (six!) gtkdoc steps intead of the standard makefile approach. I feel the maintenence burden of such a script will be very low.

[Martin Robinson]

Created attachment 112464 [details]
Patch

[Martin Robinson]

I've uploaded my first stab at this. It generates the WebKit2 documentaiton to <build_dir>/Documentation/WebKit2. The wrapper 'build-gtkdoc' script is a bit uglier than I'd hoped, but we can always push some of this into helpers if we do share this approach with WebKit1.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 112464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112464&action=review

> Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:83
> +generator = gtkdoc.PkgConfigGTKDoc(module_name="webkit2gtk",
> +                                   output_dir=output_dir,
> +                                   pkg_config_path=pkg_config_path,
> +                                   source_dirs=[src_dir],
> +                                   doc_dir=doc_dir,
> +                                   cflags=cflags,
> +                                   library_path=library_path,
> +                                   decorator="WEBKIT_API",
> +                                   deprecation_guard="WEBKIT_DISABLE_DEPRECATED",
> +                                   ignored_files=ignored_files)

All of this information is already available in the makefile, I think we could just call the other script from the makefile passing all this info as parameters.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:110
> +        raise_error_if_not_specified('output_dir')
> +        raise_error_if_not_specified('source_dirs')
> +        raise_error_if_not_specified('module_name')

Wouldn't it be better to use OptionParser instead?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:139
> +        for path in os.environ.get('PATH', '').split(':'):

Use could use os.pathsep instead of : here.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:180
> +        process_output = process.communicate()

I always have to think what process_out[0] and [1] are, I prefer to use something like

stdout, stderr = process.communicate()

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:181
> +        process.wait()

communicate already does the wait, so you don't need to call wait here.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:186
> +        if process_output[0]:
> +            print process_output[0]
> +        if process_output[1]:
> +            print process_output[1]

I think stdout output should go to stdout and stderr output to stderr, so you could do something like

if stdout:
    sys.stdout.write(stdout)
if stderr:
    sys.stderr.write(stderr)

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:192
> +        if self._output_has_warnings(process_output):

I think warnings are always printed on stdout, so this could be simply 

if 'warning:` in stdout:
    ....

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:222
> +        if not(os.path.exists(self.output_dir)):

not isn't a function, you don't need to use parentheses there.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:223
> +            os.makedirs(self.output_dir)

instead of checking wheter it exists and then create it, you can just call makedirs and handle the exception

try:
    os.makedirs()
except OSError as e:
    if e.errno != errno.EEXIST:
        raise

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:235
> +        if not os.path.exists(html_dest_dir):
> +            os.mkdir(html_dest_dir)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:253
> +        open(version_xml_path, 'w').write(self.version)

I think you should close the file too

[Martin Robinson]

(In reply to comment #4)
> (From update of attachment 112464 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112464&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:83
> > +generator = gtkdoc.PkgConfigGTKDoc(module_name="webkit2gtk",
> > +                                   output_dir=output_dir,
> > +                                   pkg_config_path=pkg_config_path,
> > +                                   source_dirs=[src_dir],
> > +                                   doc_dir=doc_dir,
> > +                                   cflags=cflags,
> > +                                   library_path=library_path,
> > +                                   decorator="WEBKIT_API",
> > +                                   deprecation_guard="WEBKIT_DISABLE_DEPRECATED",
> > +                                   ignored_files=ignored_files)
> 
> All of this information is already available in the makefile, I think we could just call the other script from the makefile passing all this info as parameters.

I discussed this with Carlos over Jabber. Essentially I want to make it simple to generate the documentation outside of make. This will simplify generation. There's a pretty big startup lag for make.

> 
> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:110
> > +        raise_error_if_not_specified('output_dir')
> > +        raise_error_if_not_specified('source_dirs')
> > +        raise_error_if_not_specified('module_name')
> 
> Wouldn't it be better to use OptionParser instead?

Isn't optparse only for parsing command-line arguments?

> 
> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:139
> > +        for path in os.environ.get('PATH', '').split(':'):
> 
> Use could use os.pathsep instead of : here.

Done.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:180
> > +        process_output = process.communicate()
> 
> I always have to think what process_out[0] and [1] are, I prefer to use something like
> 
> stdout, stderr = process.communicate()

Sure. It's indeed quite a bit clearer.

> 
> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:181
> > +        process.wait()
> 
> communicate already does the wait, so you don't need to call wait here.

Okay.

> if stdout:
>     sys.stdout.write(stdout)
> if stderr:
>     sys.stderr.write(stderr) 

Done.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:192
> > +        if self._output_has_warnings(process_output):
> I think warnings are always printed on stdout, so this could be simply 
> if 'warning:` in stdout:
>     ....

I'm using this pattern, but I'm still checking both. I think it's good for completeness sake. The warnings should be on stdout.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:222
> > +        if not(os.path.exists(self.output_dir)):
> 
> not isn't a function, you don't need to use parentheses there.

Removed this. Thought I had fixed all these before uploading.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:223
> > +            os.makedirs(self.output_dir)
> 
> instead of checking wheter it exists and then create it, you can just call makedirs and handle the exception

I think checking first is more compact, so I kept it. There's a slight race condition, but I'm not too worried. :)

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:253
> > +        open(version_xml_path, 'w').write(self.version)
> I think you should close the file too

Nice catch! Fixed.

[Martin Robinson]

Created attachment 112638 [details]
Patch

[Martin Robinson]

(In reply to comment #6)
> Created an attachment (id=112638) [details]
> Patch

The new version of the patch also looks for the build in WebKitBuild.

[Carlos Garcia Campos]

(In reply to comment #5)
> > 
> > > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:110
> > > +        raise_error_if_not_specified('output_dir')
> > > +        raise_error_if_not_specified('source_dirs')
> > > +        raise_error_if_not_specified('module_name')
> > 
> > Wouldn't it be better to use OptionParser instead?
> 
> Isn't optparse only for parsing command-line arguments?

Yes, and these are command line options after all, you can give the args to the parser as an argument and the parser doesn't look at sys.argv.

> 
> > > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:223
> > > +            os.makedirs(self.output_dir)
> > 
> > instead of checking wheter it exists and then create it, you can just call makedirs and handle the exception
> 
> I think checking first is more compact, so I kept it. There's a slight race condition, but I'm not too worried. :)

Yes, main problem is the race condition, but I don't think it's that important in this case, since this script is not supposed to be called in parallel

[Carlos Garcia Campos]

Comment on attachment 112638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112638&action=review

> Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:91
> +generator = gtkdoc.PkgConfigGTKDoc(module_name="webkit2gtk",
> +                                   output_dir=output_dir,
> +                                   pkg_config_path=pkg_config_path,
> +                                   source_dirs=[src_dir],
> +                                   doc_dir=doc_dir,
> +                                   cflags=cflags,
> +                                   library_path=library_path,
> +                                   decorator="WEBKIT_API",
> +                                   deprecation_guard="WEBKIT_DISABLE_DEPRECATED",
> +                                   ignored_files=ignored_files)

I still think we could move all of this to the makefile, or turn it into a config file.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:102
> +        for (key, value) in args.iteritems():

You don't need parentheses here, it's the comma what makes the tuple.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:182
> +        (stdout, stderr) = process.communicate()

Ditto.

[Martin Robinson]

(In reply to comment #9)

> > Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:91
> > +generator = gtkdoc.PkgConfigGTKDoc(module_name="webkit2gtk",
> > +                                   output_dir=output_dir,
> > +                                   pkg_config_path=pkg_config_path,
> > +                                   source_dirs=[src_dir],
> > +                                   doc_dir=doc_dir,
> > +                                   cflags=cflags,
> > +                                   library_path=library_path,
> > +                                   decorator="WEBKIT_API",
> > +                                   deprecation_guard="WEBKIT_DISABLE_DEPRECATED",
> > +                                   ignored_files=ignored_files)
> 
> I still think we could move all of this to the makefile, or turn it into a config file.

For now, why don't we have build-gtkdoc be the config file? I'm not sure it makes sense to add a bunch of configuration file parsing code now when Python is more expressive than any scheme we could come up with.

I don't prefer to have this in a makefile, because that means you'll need to run make to build the docs. On my system (which is quite speedy) make churns for 6 seconds before doing anything. This will make fixing the docs pretty painful as you need to continously run the tools as you are fixing errors.

I'd like gtkdoc.py to be a module and not a standalone script as well. A module allows easier integration with other WebKit tooling such as webkit-patch, the EWS, and run-webkit-tests. I also am considering releasing gtkdoc.py as a standalone module for mucking with gtkdoc. Admittedly, I'm not sure if that's useful yet.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:102
> > +        for (key, value) in args.iteritems():
> 
> You don't need parentheses here, it's the comma what makes the tuple.

Indeed, a comma is the tuple constructor, but I prefer to wrap my tuples in parenthesis the same way the interpreter does. I'll gladly change it if you feel strongly though.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > > Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:91
> > > +generator = gtkdoc.PkgConfigGTKDoc(module_name="webkit2gtk",
> > > +                                   output_dir=output_dir,
> > > +                                   pkg_config_path=pkg_config_path,
> > > +                                   source_dirs=[src_dir],
> > > +                                   doc_dir=doc_dir,
> > > +                                   cflags=cflags,
> > > +                                   library_path=library_path,
> > > +                                   decorator="WEBKIT_API",
> > > +                                   deprecation_guard="WEBKIT_DISABLE_DEPRECATED",
> > > +                                   ignored_files=ignored_files)
> > 
> > I still think we could move all of this to the makefile, or turn it into a config file.
> 
> For now, why don't we have build-gtkdoc be the config file? I'm not sure it makes sense to add a bunch of configuration file parsing code now when Python is more expressive than any scheme we could come up with.

Indeed, the config file would be just a python script that you can just import, it's pretty common, see the jhbuildrc file for example.

> I don't prefer to have this in a makefile, because that means you'll need to run make to build the docs. On my system (which is quite speedy) make churns for 6 seconds before doing anything. This will make fixing the docs pretty painful as you need to continously run the tools as you are fixing errors.

But you will have to call it from the makefile anyway to generate the docs while building, no? Anyway, I'm not saying you can't call it from the command line too, but when calling from the makefile you don't use the config file, you pass everything as command line arguments.

> I'd like gtkdoc.py to be a module and not a standalone script as well. A module allows easier integration with other WebKit tooling such as webkit-patch, the EWS, and run-webkit-tests. I also am considering releasing gtkdoc.py as a standalone module for mucking with gtkdoc. Admittedly, I'm not sure if that's useful yet.

You can have both, just add:

if __name__ == '__main__':
    main()

that won't be executed when the script is imported as a module.

> > > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:102
> > > +        for (key, value) in args.iteritems():
> > 
> > You don't need parentheses here, it's the comma what makes the tuple.
> 
> Indeed, a comma is the tuple constructor, but I prefer to wrap my tuples in parenthesis the same way the interpreter does. I'll gladly change it if you feel strongly though.

Not strongly, just that I'm used to avoid paretheses when they are not needed, the same way we don't add parentheses in the ifs, but if you prefer to keep them for tuples, it's ok to me.

[Philippe Normand]

Comment on attachment 112638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112638&action=review

> Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:37
> +def find_build_directory(top_level):

What they do in webkitpy is calling the perl webkit-build-directory script. I think it'd be better to call it here instead of reinventing the wheel :)

[Martin Robinson]

(In reply to comment #11)

> But you will have to call it from the makefile anyway to generate the docs while building, no? Anyway, I'm not saying you can't call it from the command line too, but when calling from the makefile you don't use the config file, you pass everything as command line arguments.

At that point when you have to change a parameter you'll need to change it in both places. It's better to keep it in once place.

> > I'd like gtkdoc.py to be a module and not a standalone script as well. A module allows easier integration with other WebKit tooling such as webkit-patch, the EWS, and run-webkit-tests. I also am considering releasing gtkdoc.py as a standalone module for mucking with gtkdoc. Admittedly, I'm not sure if that's useful yet.

> > I'd like gtkdoc.py to be a module and not a standalone script as well. 

> You can have both, just add:
> 
> if __name__ == '__main__':
>     main()
> 
> that won't be executed when the script is imported as a module.

I should have been clearer. I'd like to keep gtkdoc.py independent of WebKit. By making gtkdoc.py the main script I'll need to introduce WebKit specific stuff into it.

> Not strongly, just that I'm used to avoid paretheses when they are not needed, the same way we don't add parentheses in the ifs, but if you prefer to keep them for tuples, it's ok to me.

I guess my inclination is unusual here, so I'll change it.

[Martin Robinson]

(In reply to comment #12)

> What they do in webkitpy is calling the perl webkit-build-directory script. I think it'd be better to call it here instead of reinventing the wheel :)

Hrm. Looking at the code it doesn't look like it handles the non build-webkit builds. This is the kind of build Gustavo does while packaging and Carlos does in general.

[Martin Robinson]

Created attachment 112797 [details]
Patch

[Martin Robinson]

(In reply to comment #15)
> Created an attachment (id=112797) [details]
> Patch

I've removed the parenthesis around the tuples and the race condition when creating directories.

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=112797) [details] [details]
> > Patch
> 
> I've removed the parenthesis around the tuples and the race condition when creating directories.

The patch still doesn't include the interactiion with the build system, are you going to do that in a following patch? We should still generate the docs when building with --enable-gtk-doc, if just checking the warnings is fast we can always do it by default when building or add a new option --enable-gtk-doc-warnings or something like that, enabled by default. If it's really fast I would avoid adding a new option.

[Martin Robinson]

(In reply to comment #17)

> The patch still doesn't include the interactiion with the build system, are you going to do that in a following patch? We should still generate the docs when building with --enable-gtk-doc, if just checking the warnings is fast we can always do it by default when building or add a new option --enable-gtk-doc-warnings or something like that, enabled by default. If it's really fast I would avoid adding a new option.

Yeah. I planned to add that in a followup patch -- after fixing the rest of the gtkdoc errors for WebKit1 and WebKit2. Checking warnings by default should be easy to do. We should just skip the mkhtml and fixxref steps. I think we can actually get rid of --enable-gtk-doc altogether and just have "make docs" do the right thing always.

[Philippe Normand]

Comment on attachment 112797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112797&action=review

Looks great Martin, I suggested some small changes, no big deal I think.

> Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc:93
> +sys.exit(1 if generator.saw_warnings else 0)

sys.exit(generator.saw_warnings)

should be enough I think.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:112
> +        raise_error_if_not_specified('output_dir')
> +        raise_error_if_not_specified('source_dirs')
> +        raise_error_if_not_specified('module_name')

If these arguments are not optional they should be made explicit in the constructor prototype.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:189
> +        if not self._has_executable_on_path(args[0]):
> +            raise Exception('%s not found on path', args[0])
> +
> +        self.logger.info("Running %s", args[0])
> +        self.logger.debug("Full command args: %s", str(args))
> +        process = subprocess.Popen(args, env=env, cwd=cwd,
> +                                   stdout=subprocess.PIPE,
> +                                   stderr=subprocess.PIPE)

No need for self._has_executable_on_path I think. Just wrap the Popen call in a try/except block and catch OSError

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:200
> +        if 'warning' in stderr or 'warning' in stdout:

if 'warning' in stderr + stdout
would work as well. As you prefer :)

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:205
> +

What about returning the stdout contents? So you could use this method instead of Popen in the sub-class

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:232
> +        self._create_directory_if_nonexistent(self.output_dir)
> +        copy_all_files_in_directory(self.doc_dir, self.output_dir)

Maybe use http://docs.python.org/library/shutil.html#shutil.copytree ?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:243
> +        self._create_directory_if_nonexistent(html_dest_dir)
> +
> +        if os.path.exists(html_src_dir):
> +            copy_all_files_in_directory(html_src_dir, html_dest_dir)

Ditto

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:269
> +        for source_dir in self.source_dirs:
> +            args.append('--source-dir=%s' % source_dir)

Just a suggestion, maybe use a list comprehension. They are faster than normal loops.

args.extend(['--source-dir=%s' % source_dir for source_dir in self.source_dirs])

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:295
> +        if self.ldflags:
> +            env['LDFLAGS'] = ldflags

Shouldn't the test be "if ldflags" ?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:304
> +        if 'CFLAGS' in env:
> +            self.logger.debug('CFLAGS=%s', env['CFLAGS'])
> +        if 'LDFLAGS' in env:
> +            self.logger.debug('LDFLAGS %s', env['LDFLAGS'])
> +        if 'RUN' in env:
> +            self.logger.debug('RUN=%s', env['RUN'])

Move these debug calls up to avoid the additional ifs?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:325
> +        for source_dir in self.source_dirs:
> +            args.append('--source-dir=%s' % source_dir)

Same remark as in the gtkdoc_scan method ;) Maybe it would make sense to store the result as an instance attribute?

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:369
> +        if not 'pkg_config_path' in args:
> +            raise Exception('pkg_config_path not given')

pkg_config_path should be made explicit.
Now I understand why you used a dictionary in the parent class. I don't have a strong opinion about it anymore but my personal preference is to make arguments explicit, even in this case. But up to you, really.

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:389
> +        self.version = process.communicate()[0].strip()

It'd be nice to use _run_command instead of those Popen calls. Additional bonus points include logging support

[Martin Robinson]

(In reply to comment #19)

Thanks for the review.

> sys.exit(generator.saw_warnings)
> should be enough I think.

Done.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:112
> > +        raise_error_if_not_specified('output_dir')
> > +        raise_error_if_not_specified('source_dirs')
> > +        raise_error_if_not_specified('module_name')
> 
> If these arguments are not optional they should be made explicit in the constructor prototype.

I left them as kwargs. Making them explicit will remove the ability to specify them in any order, I think.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:189
> > +        if not self._has_executable_on_path(args[0]):
> > +            raise Exception('%s not found on path', args[0])
> > +
> > +        self.logger.info("Running %s", args[0])
> > +        self.logger.debug("Full command args: %s", str(args))
> > +        process = subprocess.Popen(args, env=env, cwd=cwd,
> > +                                   stdout=subprocess.PIPE,
> > +                                   stderr=subprocess.PIPE)
> 
> No need for self._has_executable_on_path I think. Just wrap the Popen call in a try/except block and catch OSError

Okay. I just let the exception bubble up now.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:200
> > +        if 'warning' in stderr or 'warning' in stdout:
> if 'warning' in stderr + stdout
> would work as well. As you prefer :)

I just left the previous style, because I like how explicit it is and that it doesn't create a temporary string.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:205
> > +
> What about returning the stdout contents? So you could use this method instead of Popen in the sub-class

Great idea! I've done this.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:232
> > +        self._create_directory_if_nonexistent(self.output_dir)
> > +        copy_all_files_in_directory(self.doc_dir, self.output_dir)
> 
> Maybe use http://docs.python.org/library/shutil.html#shutil.copytree ?

With copytree I'd have to delete the parent directory first and any files placed by the user would disappear. copytree is also recursive, so I'd need to hack the ignore callable to avoid copying too much. I stuck with the manual approach here. Hope that's okay.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:269
> > +        for source_dir in self.source_dirs:
> > +            args.append('--source-dir=%s' % source_dir)
> 
> Just a suggestion, maybe use a list comprehension. They are faster than normal loops.
> args.extend(['--source-dir=%s' % source_dir for source_dir in self.source_dirs])

Did this!

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:295
> > +        if self.ldflags:
> > +            env['LDFLAGS'] = ldflags
> 
> Shouldn't the test be "if ldflags" ?

Yes! Fixed.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:304
> > +        if 'CFLAGS' in env:
> > +            self.logger.debug('CFLAGS=%s', env['CFLAGS'])
> > +        if 'LDFLAGS' in env:
> > +            self.logger.debug('LDFLAGS %s', env['LDFLAGS'])
> > +        if 'RUN' in env:
> > +            self.logger.debug('RUN=%s', env['RUN'])
> 
> Move these debug calls up to avoid the additional ifs?

The idea was that if they already had something in their environment that was interfering with the run, they could see it when they printed out debugging information. I think that's a good feature to keep.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:325
> > +        for source_dir in self.source_dirs:
> > +            args.append('--source-dir=%s' % source_dir)
> 
> Same remark as in the gtkdoc_scan method ;) Maybe it would make sense to store the result as an instance attribute?

Fixed, but regenerated it. It should be a very cheap operation.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:369
> > +        if not 'pkg_config_path' in args:
> > +            raise Exception('pkg_config_path not given')
> 
> pkg_config_path should be made explicit.
> Now I understand why you used a dictionary in the parent class. I don't have a strong opinion about it anymore but my personal preference is to make arguments explicit, even in this case. But up to you, really.

Made pkg_config_path explicit, but kept the rest as kwargs.

> > Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:389
> > +        self.version = process.communicate()[0].strip()
> 
> It'd be nice to use _run_command instead of those Popen calls. Additional bonus points include logging support

Fixed.

[Martin Robinson]

Created attachment 114294 [details]
Patch

[Philippe Normand]

Comment on attachment 114294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114294&action=review

Ok! Just a last little remark. Thanks again for the efforts on this!

> Source/WebKit2/UIProcess/API/gtk/docs/gtkdoc.py:361
> +        self._read_pkg_config_file(pkg_config_path)
> +
> +    def _read_pkg_config_file(self, pkg_config_path):

Oh sorry I missed that in previous review. Maybe you can do everything in the constructor? Well a separate method is ok as well :)

[Martin Robinson]

Committed r99802: <http://trac.webkit.org/changeset/99802>

[Martin Robinson]

(In reply to comment #22)

> Oh sorry I missed that in previous review. Maybe you can do everything in the constructor? Well a separate method is ok as well :)

Sure, that's a reasonable change. I did that before landing.