Bug 128417 – [GTK] generate-gtkdoc should not generate documentation for source files for unbuilt source files

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This causes serious headaches for people who work on the API layer.

[Martin Robinson]

Created attachment 224547 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 224547 [details]
Patch

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

Thanks for working on this. I have a just a few comments and questions.

> Source/WebCore/bindings/gobject/GNUmakefile.am:606
> +gtkdoc-webkitdom.cfg: $(WebCore)/bindings/gobject/GNUmakefile.am $(srcdir)/Tools/gtk/GNUmakefile.am
> +	$(AM_V_GEN)echo "[webkitdomgtk]" > $@ && \
> +	echo "pkgconfig_file=$(webkitdom_pkgconfig_file)" >> $@ && \
> +	echo "namespace=webkit_dom" >> $@ && \
> +	echo "doc_dir=DerivedSources/webkitdom/docs" >> $@ && \
> +	echo -e "cflags=-I$(srcdir)/Source $(webkitgtk_gdom_include_dirs)" >> $@ && \
> +	echo "source_dirs=$(top_builddir)/DerivedSources/webkitdom $(srcdir)/Source/WebCore/bindings/gobject" >> $@ && \
> +	echo "headers=$(webkitgtk_gdom_built_h_api) DerivedSources/webkitdom/WebKitDOMDeprecated.h" >> $@
> +BUILT_SOURCES += gtkdoc-webkitdom.cfg

Since we are generating different config files for wk1, wk2 and wkdom, I wonder if it would be easier to make these config files being python files that can be just imported. We don't really need to have sections, since the file is gtkdoc-webkitdom.cfg and the only section it has is webkitdom.

> Tools/gtk/GNUmakefile.am:47
> -docs-build.stamp: $(docs_build_stamp_list)
> +docs-build.stamp: $(docs_build_stamp_list) gtkdoc-webkitdom.cfg gtkdoc-webkitgtk.cfg gtkdoc-webkit2gtk.cfg

I guess this is a problem if wk1 or wk2 is disabled in build.

> Tools/gtk/generate-gtkdoc:-50
> -def get_gtkdoc_module_paths(xref_dep_packages):
> -    deps = []
> -    html_dir = os.path.join('share', 'gtk-doc', 'html')
> -
> -    for package in xref_dep_packages:
> -        prefix = common.prefix_of_pkg_config_file(package)
> -        if prefix is None:
> -            continue
> -        for module in xref_dep_packages[package]:
> -            deps.append(os.path.join(prefix, html_dir, module))
> -
> -    return deps

What is this change exactly? It's not easy to see why we need to change this looking at the diff.

> Tools/gtk/generate-gtkdoc:86
> +            if os.path.exists(file) and os.path.isfile(file):

os.path.isfile() returns False if the fiel doesn't exist, so you don't need to call os.path.exists() too.

> Tools/gtk/generate-gtkdoc:89
> +        add_file_if_exists(os.path.splitext(header)[0] + ".cpp")
> +        add_file_if_exists(os.path.splitext(header)[0] + ".c")

You could avoid doing the splitext twice here

> Tools/gtk/generate-gtkdoc:95
> +        if not os.path.splitext(file)[1] in ['.h', '.c', '.cpp', '.cc']:
> +            return False # These files are ignored anyway.

I think this is easier to read as if os.path.splitext(file)[1] not in ['.h', '.c', '.cpp', '.cc']:

You don't need the absolute path for this check, so you can probably move the previous line after this.

> Tools/gtk/generate-gtkdoc:97
> +        if not os.path.isfile(file):
> +            return False

Not sure if you need the abs patch for this check either.

> Tools/gtk/generate-gtkdoc:100
> +        if file in implementation_files:
> +            return False
> +        return True

This could be 

return file not in implementation_files

> Tools/gtk/generate-gtkdoc:108
> +    config = SafeConfigParser()
> +    config.read(config_file)
> +    module_name = config.sections()[0]

What happen if generate-gtkdoc is called and the config files don't exist?

> Tools/gtk/generate-gtkdoc:189
> +    saw_webkit1_warnings = generate_documentation_for_config(common.build_path('gtkdoc-webkitgtk.cfg'))
> +    saw_webkit2_warnings = generate_documentation_for_config(common.build_path('gtkdoc-webkit2gtk.cfg'))

I think we should check if the config files actually exist and exit with an error message if they don't

> Tools/gtk/gtkdoc.py:42
> +                          Required if source_files is not specified.

What is source_files? you mean headers? I think both options should always be required. Since the config files are now required and they always contain both, I guess there's no problem and it makes the whole thing easier and safer.

> Tools/gtk/gtkdoc.py:122
> -        def raise_error_if_not_specified(key):
> -            if not getattr(self, key):
> -                raise Exception('%s not specified.' % key)
> -
> -        raise_error_if_not_specified('output_dir')
> -        raise_error_if_not_specified('source_dirs')
> -        raise_error_if_not_specified('module_name')
> +        if not getattr(self, 'output_dir'):
> +            raise Exception('output_dir not specified.')
> +        if not getattr(self, 'module_name'):
> +            raise Exception('module_name not specified.')

Why?

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 224547 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224547&action=review
> 
> Thanks for working on this. I have a just a few comments and questions.

Thanks for the timely review! I'll post a new patch, but I'll answer a few of your questions first.


> Since we are generating different config files for wk1, wk2 and wkdom, I wonder if it would be easier to make these config files being python files that can be just imported. We don't really need to have sections, since the file is gtkdoc-webkitdom.cfg and the only section it has is webkitdom.

The nice thing about the config file format is that we don't have to worry about quotes and commas as much, as the config file format syntax is much more flexible than Python syntax. This makes the makefiles much more readable. I think we can consider Python files, but we should do it once we've switched fully to CMake, which has more expressive looping constructs and string operations, but right now, I think it would make everything a lot more complicated.

> > Tools/gtk/generate-gtkdoc:-50
> > -def get_gtkdoc_module_paths(xref_dep_packages):
> > -    deps = []
> > -    html_dir = os.path.join('share', 'gtk-doc', 'html')
> > -
> > -    for package in xref_dep_packages:
> > -        prefix = common.prefix_of_pkg_config_file(package)
> > -        if prefix is None:
> > -            continue
> > -        for module in xref_dep_packages[package]:
> > -            deps.append(os.path.join(prefix, html_dir, module))
> > -
> > -    return deps
> 
> What is this change exactly? It's not easy to see why we need to change this looking at the diff.

Before each module had it's own dictionary of cross-reference dependencies. Since they can be the same for all modules without harm, it was easier to just simplify this into a single method and not have to store anything in the configuration files.

> 
> > Tools/gtk/generate-gtkdoc:86
> > +            if os.path.exists(file) and os.path.isfile(file):
> 
> os.path.isfile() returns False if the fiel doesn't exist, so you don't need to call os.path.exists() too.
> 
> > Tools/gtk/generate-gtkdoc:89
> > +        add_file_if_exists(os.path.splitext(header)[0] + ".cpp")
> > +        add_file_if_exists(os.path.splitext(header)[0] + ".c")
> 
> You could avoid doing the splitext twice here
> 
> > Tools/gtk/generate-gtkdoc:95
> > +        if not os.path.splitext(file)[1] in ['.h', '.c', '.cpp', '.cc']:
> > +            return False # These files are ignored anyway.
> 
> I think this is easier to read as if os.path.splitext(file)[1] not in ['.h', '.c', '.cpp', '.cc']:
> 
> You don't need the absolute path for this check, so you can probably move the previous line after this.
> 
> > Tools/gtk/generate-gtkdoc:97
> > +        if not os.path.isfile(file):
> > +            return False
> 
> Not sure if you need the abs patch for this check either.
> 
> > Tools/gtk/generate-gtkdoc:100
> > +        if file in implementation_files:
> > +            return False
> > +        return True
> 
> This could be 
> 
> return file not in implementation_files
> 
> > Tools/gtk/generate-gtkdoc:108
> > +    config = SafeConfigParser()
> > +    config.read(config_file)
> > +    module_name = config.sections()[0]
> 
> What happen if generate-gtkdoc is called and the config files don't exist?
> 
> > Tools/gtk/generate-gtkdoc:189
> > +    saw_webkit1_warnings = generate_documentation_for_config(common.build_path('gtkdoc-webkitgtk.cfg'))
> > +    saw_webkit2_warnings = generate_documentation_for_config(common.build_path('gtkdoc-webkit2gtk.cfg'))
> 
> I think we should check if the config files actually exist and exit with an error message if they don't
> 
> > Tools/gtk/gtkdoc.py:42
> > +                          Required if source_files is not specified.
> 
> What is source_files? you mean headers? I think both options should always be required. Since the config files are now required and they always contain both, I guess there's no problem and it makes the whole thing easier and safer.

I think of gtkdoc.py as independent of the front-end, generate-gtkdoc. I'm writing it in hopes that it could be useful to other projects. It may not turn out to be the case, but it isn't something I want to change in this patch. Only one is truly required.

> > Tools/gtk/gtkdoc.py:122
> > -        def raise_error_if_not_specified(key):
> > -            if not getattr(self, key):
> > -                raise Exception('%s not specified.' % key)
> > -
> > -        raise_error_if_not_specified('output_dir')
> > -        raise_error_if_not_specified('source_dirs')
> > -        raise_error_if_not_specified('module_name')
> > +        if not getattr(self, 'output_dir'):
> > +            raise Exception('output_dir not specified.')
> > +        if not getattr(self, 'module_name'):
> > +            raise Exception('module_name not specified.')
> 
> Why?

Since it cannot be used for all three, the method is of limited use now. The third one now has to check two arguments. Both approaches have the number of lines of code.

[Martin Robinson]

Created attachment 224801 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 224801 [details]
Patch

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

> Tools/gtk/GNUmakefile.am:13
> -docs_build_stamp_list = \
> +docs_build_dependencies = \

Why renaming this? looks unrelated

> Tools/gtk/generate-gtkdoc:88
> +        header_parts = os.path.splitext(header)
> +        add_file_if_exists(header_parts[0] + ".cpp")
> +        add_file_if_exists(header_parts[0] + ".c")

You could get the basename without the extension directly, and avoid using [0]. 

basename = os.path.splitext(header)[0]

> Tools/gtk/generate-gtkdoc:123
> +        'cflags': " ".join(config.get(module_name, 'cflags').split()),

Why do you split and then join?

[Martin Robinson]

(In reply to comment #5)
> (From update of attachment 224801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224801&action=review
> 
> > Tools/gtk/GNUmakefile.am:13
> > -docs_build_stamp_list = \
> > +docs_build_dependencies = \
> 
> Why renaming this? looks unrelated

The name doesn't really make sense any more since none of the files on the list are stamp files.

> 
> > Tools/gtk/generate-gtkdoc:88
> > +        header_parts = os.path.splitext(header)
> > +        add_file_if_exists(header_parts[0] + ".cpp")
> > +        add_file_if_exists(header_parts[0] + ".c")
> 
> You could get the basename without the extension directly, and avoid using [0]. 
> 
> basename = os.path.splitext(header)[0]

Okay. Makes sense.

[Martin Robinson]

Committed r164632: <http://trac.webkit.org/changeset/164632>

[Martin Robinson]

Reopening to attach new patch.

[Martin Robinson]

Created attachment 225166 [details]
Patch

[Martin Robinson]

Comment on attachment 225166 [details]
Patch

Patch landed as a diff for the old patch.