Bug 77551

Summary: [GTK] Fix xrefs after installing API documentation
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, hayato, mrobinson, pnormand, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 77094, 77542    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
The correct patch
none
Updated patch according to review mrobinson: review+

Description Carlos Garcia Campos 2012-02-01 06:49:05 PST
gtk-doc does the right thing when you have the api documentation of the dependencies (glib, soup, gtk, etc.) installed in the same prefix you are installing webkit, but we need to provide the additional directories to scan for api docs installed in a different prefix. We can get the prefix of every package from the pkg-config file, pass the directories where the documentation might be installed for every dependency to gtkdoc-fixxref. That's enough to fix gtkdoc-fixxref warnings (assuming you have the docs intalled), but links are created with path relative to the current one, assuming all deps will be in the same prefix. So, after install the documentation we need to run gtkdoc-rebase to fix all possible invalid links (valid links are just kept of course).
Comment 1 Carlos Garcia Campos 2012-02-01 06:59:19 PST
Created attachment 124947 [details]
Patch
Comment 2 Martin Robinson 2012-02-01 08:43:44 PST
I believe this is the patch for bug 77094.
Comment 3 Carlos Garcia Campos 2012-02-01 08:49:51 PST
Created attachment 124957 [details]
The correct patch

Sorry, I uploaded the wrong patch :-P
Comment 4 Hayato Ito 2012-02-01 20:37:59 PST
It seems I wrongly canceled the review flag for the patch in 77551. I intended 77511.
Could you set review flag again if it was canceled? I am sorry.
Comment 5 Carlos Garcia Campos 2012-02-01 23:32:31 PST
Comment on attachment 124957 [details]
The correct patch

No problem.
Comment 6 Carlos Garcia Campos 2012-02-01 23:38:28 PST
Btw, Martin suggested to get the deps automatically from the pkg-config files, starting with the packages required by webkitgtk and walking recursively. That might work to find the packages, but every package can have more than one module and it can be installed with a name it's not possible to get automatically. For example: glib-2.0 installs gio, glib and gobject, gtk+2-0 installs gdk and gtk, but gtk-3.0 instal gtk3 and gdk3, libsoup-2.4 installs libsoup-2.4 with the same name than the package. The deps are just a few, so I think it's not a problem to add them manually. If we introduce a new one, gtkdoc-fixxref will show new warnings.
Comment 7 Martin Robinson 2012-02-06 00:37:00 PST
Comment on attachment 124957 [details]
The correct patch

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

Nice stuff! I have just a few suggestions for moving things around.

> Tools/GNUmakefile.am:328
> +	$(srcdir)/Tools/gtk/generate-gtkdoc --rebase

I think this should happen automatically when --skip-html is not passed to generate-gtkdoc. Either that or you should add --skip-rebase to generate-gtkdoc and make sure to pass it in the build scripts.

> Tools/gtk/common.py:90
> +def xref_deps(xref_dep_packages):
> +    def package_prefix(package):
> +        process = subprocess.Popen(['pkg-config', '--variable=prefix', package],
> +                                   stdout=subprocess.PIPE)
> +        stdout = process.communicate()[0]
> +        if process.returncode != 0:
> +            return None
> +        return stdout.strip()
> +
> +    deps = []

I think odds are low that we are ever going to use xref_deps in any other file. I would keep package_prefix here and just call it something like prefix_of_pkg_config_file. I'd move xref_deps to generate-gtkdoc and call it get_gtkdoc_module_paths.

> Tools/gtk/common.py:103
> +def gtk_version(pkg_config_path):

Might want to name this to gtk_version_of_pkg_config_file.

> Tools/gtk/generate-gtkdoc:106
> +        'xref_deps' : common.xref_deps(xref_deps),

Do you mind changing this argument name to cross_reference_deps? I know it's a bit obtuse, but cross_reference seems a little clearer than xref here.

> Tools/gtk/generate-gtkdoc:140
> +    if '--rebase' not in sys.argv:
> +        print "Generating WebKit1 documentation..."
> +        saw_webkit1_warnings = generate_doc(pkg_config_path, get_webkit1_options(common.gtk_version(pkg_config_path)))
> +    else:
> +        print "Rebasing WebKit1 documentation..."
> +        rebase_doc(pkg_config_path, get_webkit1_options(common.gtk_version(pkg_config_path)))

I think it makes sense to just get the options once here.
Comment 8 Carlos Garcia Campos 2012-02-06 00:42:21 PST
(In reply to comment #7)
> (From update of attachment 124957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124957&action=review
> 
> Nice stuff! I have just a few suggestions for moving things around.
> 
> > Tools/GNUmakefile.am:328
> > +	$(srcdir)/Tools/gtk/generate-gtkdoc --rebase
> 
> I think this should happen automatically when --skip-html is not passed to generate-gtkdoc. Either that or you should add --skip-rebase to generate-gtkdoc and make sure to pass it in the build scripts.

I'm not sure I get what you mean, you only want to rebase installed docs, so rebase has to be called once the docs has been installed.

> > Tools/gtk/common.py:90
> > +def xref_deps(xref_dep_packages):
> > +    def package_prefix(package):
> > +        process = subprocess.Popen(['pkg-config', '--variable=prefix', package],
> > +                                   stdout=subprocess.PIPE)
> > +        stdout = process.communicate()[0]
> > +        if process.returncode != 0:
> > +            return None
> > +        return stdout.strip()
> > +
> > +    deps = []
> 
> I think odds are low that we are ever going to use xref_deps in any other file. I would keep package_prefix here and just call it something like prefix_of_pkg_config_file. I'd move xref_deps to generate-gtkdoc and call it get_gtkdoc_module_paths.

Ok

> > Tools/gtk/common.py:103
> > +def gtk_version(pkg_config_path):
> 
> Might want to name this to gtk_version_of_pkg_config_file.
> 
> > Tools/gtk/generate-gtkdoc:106
> > +        'xref_deps' : common.xref_deps(xref_deps),
> 
> Do you mind changing this argument name to cross_reference_deps? I know it's a bit obtuse, but cross_reference seems a little clearer than xref here.

xref is pretty standard name, but I don't mind to rename it

> > Tools/gtk/generate-gtkdoc:140
> > +    if '--rebase' not in sys.argv:
> > +        print "Generating WebKit1 documentation..."
> > +        saw_webkit1_warnings = generate_doc(pkg_config_path, get_webkit1_options(common.gtk_version(pkg_config_path)))
> > +    else:
> > +        print "Rebasing WebKit1 documentation..."
> > +        rebase_doc(pkg_config_path, get_webkit1_options(common.gtk_version(pkg_config_path)))
> 
> I think it makes sense to just get the options once here.

it happens once
Comment 9 Martin Robinson 2012-02-06 00:49:55 PST
Comment on attachment 124957 [details]
The correct patch

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

>>> Tools/GNUmakefile.am:328
>>> +	$(srcdir)/Tools/gtk/generate-gtkdoc --rebase
>> 
>> I think this should happen automatically when --skip-html is not passed to generate-gtkdoc. Either that or you should add --skip-rebase to generate-gtkdoc and make sure to pass it in the build scripts.
> 
> I'm not sure I get what you mean, you only want to rebase installed docs, so rebase has to be called once the docs has been installed.

Ah, I understand now. I suggested a small rename below then.

>>> Tools/gtk/generate-gtkdoc:140
>>> +        rebase_doc(pkg_config_path, get_webkit1_options(common.gtk_version(pkg_config_path)))
>> 
>> I think it makes sense to just get the options once here.
> 
> it happens once

Sorry, I meant just to factor out the code that gets the options to one place.
options = get_webkit1_options(common.gtk_version(pkg_config_path)
if '--rebase' not in sys.argv:
    print "Generating WebKit1 documentation..."
    saw_webkit1_warnings = generate_doc(pkg_config_path, options)
else:
    print "Rebasing WebKit1 documentation..."
    rebase_doc(pkg_config_path, options)

> Tools/gtk/gtkdoc.py:352
> +    def rebase(self):

Perhaps call this rebase_installed_docs, just to avoid confusing dumb people like me. :)
Comment 10 Carlos Garcia Campos 2012-02-06 00:52:14 PST
(In reply to comment #9)
> (From update of attachment 124957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124957&action=review
> 
> >>> Tools/GNUmakefile.am:328
> >>> +	$(srcdir)/Tools/gtk/generate-gtkdoc --rebase
> >> 
> >> I think this should happen automatically when --skip-html is not passed to generate-gtkdoc. Either that or you should add --skip-rebase to generate-gtkdoc and make sure to pass it in the build scripts.
> > 
> > I'm not sure I get what you mean, you only want to rebase installed docs, so rebase has to be called once the docs has been installed.
> 
> Ah, I understand now. I suggested a small rename below then.
> 
> >>> Tools/gtk/generate-gtkdoc:140
> >>> +        rebase_doc(pkg_config_path, get_webkit1_options(common.gtk_version(pkg_config_path)))
> >> 
> >> I think it makes sense to just get the options once here.
> > 
> > it happens once
> 
> Sorry, I meant just to factor out the code that gets the options to one place.
> options = get_webkit1_options(common.gtk_version(pkg_config_path)
> if '--rebase' not in sys.argv:
>     print "Generating WebKit1 documentation..."
>     saw_webkit1_warnings = generate_doc(pkg_config_path, options)
> else:
>     print "Rebasing WebKit1 documentation..."
>     rebase_doc(pkg_config_path, options)

Ok

> > Tools/gtk/gtkdoc.py:352
> > +    def rebase(self):
> 
> Perhaps call this rebase_installed_docs, just to avoid confusing dumb people like me. :)

Ok
Comment 11 Carlos Garcia Campos 2012-02-06 01:17:17 PST
Created attachment 125593 [details]
Updated patch according to review
Comment 12 Martin Robinson 2012-02-06 01:19:29 PST
Comment on attachment 125593 [details]
Updated patch according to review

Thanks!
Comment 13 Carlos Garcia Campos 2012-02-06 01:23:30 PST
Committed r106786: <http://trac.webkit.org/changeset/106786>