Bug 78177 - [GTK] doc rebasing does not respect DESTDIR
Summary: [GTK] doc rebasing does not respect DESTDIR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-08 16:27 PST by Gustavo Noronha (kov)
Modified: 2012-02-08 18:41 PST (History)
1 user (show)

See Also:


Attachments
Patch (2.52 KB, patch)
2012-02-08 16:30 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (3.31 KB, patch)
2012-02-08 17:27 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2012-02-08 16:27:51 PST
[GTK] doc rebasing does not respect DESTDIR
Comment 1 Gustavo Noronha (kov) 2012-02-08 16:30:35 PST
Created attachment 126183 [details]
Patch
Comment 2 Martin Robinson 2012-02-08 16:35:53 PST
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?
Comment 3 Gustavo Noronha (kov) 2012-02-08 17:27:58 PST
Created attachment 126200 [details]
Patch
Comment 4 Gustavo Noronha (kov) 2012-02-08 17:38:09 PST
> > 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 =).
Comment 5 Martin Robinson 2012-02-08 17:51:01 PST
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.
Comment 6 Gustavo Noronha (kov) 2012-02-08 17:59:24 PST
(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.
Comment 7 Martin Robinson 2012-02-08 18:05:58 PST
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.
Comment 8 Gustavo Noronha (kov) 2012-02-08 18:40:52 PST
Comment on attachment 126200 [details]
Patch

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