Bug 116378 – [GTK] [CMake] Add a "make dist" target

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

[Martin Robinson]

We just need to ensure that CPack includes all the necessary files.

[Martin Robinson]

CPack doesn't look like a great option for WebKitGTK+:

1. It doesn't have support for LZMA.
2. It doesn't easily handle including files from the build directory into the tarball.
3. I don't see a way to rename files for the tarball.

Unfortunately, because of these issues, it probably makes sense to create our own 'make-dist' script.

[Martin Robinson]

Created attachment 220678 [details]
Patch

[Peter Gal]

Comment on attachment 220678 [details]
Patch

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

Just a few nits.

> Tools/gtk/make-dist.py:42
> +        if not(self.pattern.search(file)):

no need for the parentheses after the 'not'. Simply: 'if not self.patt...'

> Tools/gtk/make-dist.py:129
> +        if self.tarball_root[-1] != '/':
> +            self.tarball_root = self.tarball_root + '/'
> +        if self.tarball_root[0] != '/':
> +            self.tarball_root = '/' + self.tarball_root

Why is this needed? We could use os.path.join(..) to correctly build paths so there is no need to add the '/'. Still if we really need to do this a (imho) a better way is to check the start/end '/' character with the .startswith/.endswith methods.

> Tools/gtk/make-dist.py:154
> +        elif string.find('$build') != -1:

A more pythonic way would be to use the 'in' operator: elif '$build' not in string

> Tools/gtk/make-dist.py:168
> +        return self.tarball_root + path

The os.path.join could be used here to build the path.

> Tools/gtk/make-dist.py:206
> +            for file in self.get_files():
> +                files_tarred = files_tarred + 1

Instead of counting the number of files by 'hand' we could use the enumerate(..) function to count for us. (see http://docs.python.org/2/library/functions.html#enumerate) (this also applies to the code above in the line range 199-201)

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 220678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220678&action=review
> 
> Just a few nits.

Thanks for the pointers. 

> > Tools/gtk/make-dist.py:129
> > +        if self.tarball_root[-1] != '/':
> > +            self.tarball_root = self.tarball_root + '/'
> > +        if self.tarball_root[0] != '/':
> > +            self.tarball_root = '/' + self.tarball_root
> 
> Why is this needed? We could use os.path.join(..) to correctly build paths so there is no need to add the '/'. Still if we really need to do this a (imho) a better way is to check the start/end '/' character with the .startswith/.endswith methods.

While right now our CMake build doesn't work outside Cygwin on Windows, I don't see any reason to make our tools platform specific if we can avoid it. os.path.join will use the platform directory separator.
 
 
> > Tools/gtk/make-dist.py:206
> > +            for file in self.get_files():
> > +                files_tarred = files_tarred + 1
> 
> Instead of counting the number of files by 'hand' we could use the enumerate(..) function to count for us. (see http://docs.python.org/2/library/functions.html#enumerate) (this also applies to the code above in the line range 199-201)

It makes a lot of sense to user enumerate for this loop, though for the one before I don't think there's a lot of benefit. I could use sum(1 for i in files) but that would create a giant list, I believe.

[Martin Robinson]

Created attachment 221053 [details]
Make the script more pythonic

[Carlos Garcia Campos]

Comment on attachment 221053 [details]
Make the script more pythonic

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

> Tools/gtk/make-dist.py:23
> +import os.path

Why do you need this? aren't you using os.path.foo anyway?

> Tools/gtk/make-dist.py:33
> +class Rule(object):

I don't think you need to specify the parent when it's object, that's implicit.

> Tools/gtk/make-dist.py:54
> +        self.rules = []
> +
> +        # By default, accept all files.
> +        self.add_rule(Rule(Rule.Result.INCLUDE, '.*'))

I guess this could be just:

self.rules = [ Rule(Rule.Result.INCLUDE, '.*') ]

> Tools/gtk/make-dist.py:58
> +        if not(cls._global_rules):

do you need the parentheses?

> Tools/gtk/make-dist.py:108
> +            del dirs[:]
> +            dirs.extend(to_keep)

I don't understand this.

> Tools/gtk/make-dist.py:136
> +        if self.current_directory:

if self.current_directory is not None:

> Tools/gtk/make-dist.py:149
> +            string = string.replace('$source', '')
> +            string = string.replace('$build', '')
> +            return string

return string.replace('$source', '').replace('$build', '')

> Tools/gtk/make-dist.py:180
> +        if not len(parts) or not len(parts[0]):

This could be just if not parts. You don't need to use len, an empty list evaluates as false. Same for the empty string

> Tools/gtk/make-dist.py:182
> +        elif parts[0][0] == "#":

Do not use else after a return

> Tools/gtk/make-dist.py:184
> +        elif parts[0] == "directory" and len(parts) > 1:

Ditto.

> Tools/gtk/make-dist.py:196
> +            for file in directory.get_files():
> +                yield file

I would call this fileTuple or something like that, it confused me that this returns a file, but then you use file[0] and file[1] in create_tarfile

> Tools/gtk/make-dist.py:201
> +        count = 0
> +        for file in self.get_files():
> +            count = count + 1

Why not returning a list instead of a generator? so that you can do count = len(self.get_files()) and it will be generated only once instead of twice (once to count and then to enumerate it)

> Tools/gtk/make-dist.py:205
> +            for i, file in enumerate(self.get_files(), start=1):

You could do:

for i, (file, tarball_path) in enumerate(self.get_files(), start=1):

and use those names instead file[0] and file[1]

> Tools/gtk/make-dist.py:230
> +    manifest = Manifest(manifest_filename=arguments.manifest_filename,
> +                        source_root=arguments.source_directory,
> +                        build_root=arguments.build_directory,
> +                        tarball_root=arguments.tarball_root)

You don't need to specify the name of the paramas, when you are passing all of them and in the right order, I think it would be easier to read.

manifest = Manifest(arguments.manifest_filename, arguments.source_directory, arguments.build_directory, arguments.tarball_root);

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 221053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221053&action=review
> 
> > Tools/gtk/make-dist.py:23
> > +import os.path
> 
> Why do you need this? aren't you using os.path.foo anyway?

I guess not! Removed.

> 
> > Tools/gtk/make-dist.py:33
> > +class Rule(object):
> 
> I don't think you need to specify the parent when it's object, that's implicit.

Specifying object as the parent means this is a new-style class, versus old-style: http://stackoverflow.com/questions/13816849/old-style-and-new-style-classes-in-python-2-7
> 
> > Tools/gtk/make-dist.py:54
> > +        self.rules = []
> > +
> > +        # By default, accept all files.
> > +        self.add_rule(Rule(Rule.Result.INCLUDE, '.*'))
> 
> I guess this could be just:
> 
> self.rules = [ Rule(Rule.Result.INCLUDE, '.*') ]

It could be. My thought was that this would make the code safe if add_rule ever did anything extra, but it's not a big deal.

> 
> > Tools/gtk/make-dist.py:58
> > +        if not(cls._global_rules):
> 
> do you need the parentheses?

Nope!

> > Tools/gtk/make-dist.py:108
> > +            del dirs[:]
> > +            dirs.extend(to_keep)
> 
> I don't understand this.

I'm trying to modify dirs in place, because it seems that's what walk requires.

> 
> > Tools/gtk/make-dist.py:136
> > +        if self.current_directory:
> 
> if self.current_directory is not None:

Okay.

> > Tools/gtk/make-dist.py:149
> > +            string = string.replace('$source', '')
> > +            string = string.replace('$build', '')
> > +            return string
> 
> return string.replace('$source', '').replace('$build', '')

Okay.

> 
> > Tools/gtk/make-dist.py:180
> > +        if not len(parts) or not len(parts[0]):
> 
> This could be just if not parts. You don't need to use len, an empty list evaluates as false. Same for the empty string

Okay.

> 
> > Tools/gtk/make-dist.py:182
> > +        elif parts[0][0] == "#":
> 
> Do not use else after a return

Okay.
> 
> > Tools/gtk/make-dist.py:184
> > +        elif parts[0] == "directory" and len(parts) > 1:
> 
> Ditto.
> 
> > Tools/gtk/make-dist.py:196
> > +            for file in directory.get_files():
> > +                yield file
> 
> I would call this fileTuple or something like that, it confused me that this returns a file, but then you use file[0] and file[1] in create_tarfile

Sure.

> > Tools/gtk/make-dist.py:201
> > +        count = 0
> > +        for file in self.get_files():
> > +            count = count + 1
> 
> Why not returning a list instead of a generator? so that you can do count = len(self.get_files()) and it will be generated only once instead of twice (once to count and then to enumerate it)

I didn't want to keep the entire list in memory. The process of iterating the files isn't noticeable, so I hope this is okay.

> > Tools/gtk/make-dist.py:205
> > +            for i, file in enumerate(self.get_files(), start=1):
> 
> You could do:
> 
> for i, (file, tarball_path) in enumerate(self.get_files(), start=1):
> 
> and use those names instead file[0] and file[1]

This is a lot nicer!

> 
> > Tools/gtk/make-dist.py:230
> > +    manifest = Manifest(manifest_filename=arguments.manifest_filename,
> > +                        source_root=arguments.source_directory,
> > +                        build_root=arguments.build_directory,
> > +                        tarball_root=arguments.tarball_root)
> 
> You don't need to specify the name of the paramas, when you are passing all of them and in the right order, I think it would be easier to read.
> 
> manifest = Manifest(arguments.manifest_filename, arguments.source_directory, arguments.build_directory, arguments.tarball_root);

Sure. I personally prefer the more explicit style, but I don't really care in the end.

[Martin Robinson]

Created attachment 222393 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 222393 [details]
Patch

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

Great stuff!

> Tools/gtk/make-dist.py:82
> +        yield (self.source_root, self.tarball_root)

Why use yield here? Doesn't look like we're returning something different ever.

> Tools/gtk/make-dist.py:126
> +        if self.tarball_root[-1] != '/':
> +            self.tarball_root = self.tarball_root + '/'
> +        if self.tarball_root[0] != '/':
> +            self.tarball_root = '/' + self.tarball_root

It was suggested before that we should use .startswith/.endswith, I think that would be more readable indeed.

> Tools/gtk/make-dist.py:177
> +        if parts[0][0] == "#":

parts[0].startswith()?

> Tools/gtk/manifest.txt:74
> +directory $build/Documentation/webkit2gtk/html Documentation/webkit2gtk/html
> +directory $build/Documentation/webkit2gtk/html Documentation/webkit2gtk/html
> +directory $build/Documentation/webkitgtk/tmpl Documentation/webkitgtk/tmpl
> +directory $build/Documentation/webkitgtk/tmpl Documentation/webkitgtk/tmpl

Shipping generated files has been frowned upon by some people before - mainly because you can't make dist if you haven't made a full build with the appropriate configuration options before that. I don't think it's a problem, and I really like having pre-build documentation in the tarball, but just thought I'd share this here.

[Martin Robinson]

Committed r163114: <http://trac.webkit.org/changeset/163114>

[Martin Robinson]

Comment on attachment 222393 [details]
Patch

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

Thanks for the review!

>> Tools/gtk/make-dist.py:82
>> +        yield (self.source_root, self.tarball_root)
> 
> Why use yield here? Doesn't look like we're returning something different ever.

This is a generator so that it has the same interface as Directory and can just live in the list of directories as a member of manifest.

>> Tools/gtk/make-dist.py:126
>> +            self.tarball_root = '/' + self.tarball_root
> 
> It was suggested before that we should use .startswith/.endswith, I think that would be more readable indeed.

Fixed!

>> Tools/gtk/make-dist.py:177
>> +        if parts[0][0] == "#":
> 
> parts[0].startswith()?

Fixed!

>> Tools/gtk/manifest.txt:74
>> +directory $build/Documentation/webkitgtk/tmpl Documentation/webkitgtk/tmpl
> 
> Shipping generated files has been frowned upon by some people before - mainly because you can't make dist if you haven't made a full build with the appropriate configuration options before that. I don't think it's a problem, and I really like having pre-build documentation in the tarball, but just thought I'd share this here.

Yeah, I'm similarly conflicted.