Bug 78177 – [GTK] doc rebasing does not respect DESTDIR

• Component: New Bugs
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

[GTK] doc rebasing does not respect DESTDIR

[Gustavo Noronha (kov)]

Created attachment 126183 [details]
Patch

[Martin Robinson]

Comment on attachment 126183 [details]
Patch

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

> Tools/gtk/generate-gtkdoc:113
> +        'destdir' : '',

You aren't actually passing the command-line argument here...is that on purpose?

[Gustavo Noronha (kov)]

Created attachment 126200 [details]
Patch

[Gustavo Noronha (kov)]

> > Tools/gtk/generate-gtkdoc:113
> > +        'destdir' : '',
> 
> You aren't actually passing the command-line argument here...is that on purpose?

FWIW, I thought that was a default. I thought I had verified it worked, but apparently I did something stupid and my verification was flawed, this couldn't possibly work. The new patch works for real, though =).

[Martin Robinson]

Comment on attachment 126200 [details]
Patch

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

> Tools/gtk/generate-gtkdoc:54
> +    # We should consider using an arguments parsing library if
> +    # we need more of these complex ones.

Better make this a TODO:

> Tools/gtk/generate-gtkdoc:68
> +    destdir = ''
> +    use_next = False
> +    for arg in sys.argv:
> +        if use_next:
> +            destdir = arg
> +            break
> +
> +        if arg.startswith('--destdir'):
> +            if '=' in arg:
> +                destdir = arg.split('=')[1]
> +                break
> +            else:
> +                use_next = True
> +

I think it's fine to be less permissive here for now and then come back later and add real argument handling:

for argument in sys.argv[i]:
    if argument.startswith('--destdir='):
        destdir = argument.split('=')[1]
        break

> Tools/gtk/generate-gtkdoc:73
> +        'destdir' : destdir,

You should add some documentation for this new parameter to gtkdoc.py.

> Tools/gtk/gtkdoc.py:359
> -        html_dir = os.path.join(self.prefix, 'share', 'gtk-doc', 'html', self.module_name)
> +        html_dir = os.path.join(self.destdir + self.prefix, 'share', 'gtk-doc', 'html', self.module_name)

Hrm. Something seems amiss here. This the only place where you concatenat self.destdir and self.prefix so how could the direcdtory exist already? Perhaps I simply am not understanding this bit of code.

[Gustavo Noronha (kov)]

(In reply to comment #5)
> (From update of attachment 126200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126200&action=review
> 
> > Tools/gtk/generate-gtkdoc:54
> > +    # We should consider using an arguments parsing library if
> > +    # we need more of these complex ones.
> 
> Better make this a TODO:
[...]
> I think it's fine to be less permissive here for now and then come back later and add real argument handling:
> 
> for argument in sys.argv[i]:
>     if argument.startswith('--destdir='):
>         destdir = argument.split('=')[1]
>         break

Yeah, I agree.


> > Tools/gtk/generate-gtkdoc:73
> > +        'destdir' : destdir,
> 
> You should add some documentation for this new parameter to gtkdoc.py.

Will do.
 
> > Tools/gtk/gtkdoc.py:359
> > -        html_dir = os.path.join(self.prefix, 'share', 'gtk-doc', 'html', self.module_name)
> > +        html_dir = os.path.join(self.destdir + self.prefix, 'share', 'gtk-doc', 'html', self.module_name)
> 
> Hrm. Something seems amiss here. This the only place where you concatenat self.destdir and self.prefix so how could the direcdtory exist already? Perhaps I simply am not understanding this bit of code.

The directory is created by the make install hook, right here:

http://trac.webkit.org/browser/trunk/Tools/GNUmakefile.am#L297

We only run generate-gtkdoc with --rebase after make install has run, so the directory is guaranteed to exist.

[Martin Robinson]

Comment on attachment 126200 [details]
Patch

Looks good, though I think I managed to convince you about that --destdir should be renamed to something like --virtual-root or --staging-dir.

[Gustavo Noronha (kov)]

Comment on attachment 126200 [details]
Patch

Landed http://trac.webkit.org/changeset/107164