[sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function

[Marco Pesenti Gritti]

-import subprocess
+from subprocess import Popen, PIPE
 import re
 import gettext
 from optparse import OptionParser
+import warnings
+import subprocess

warnings seem unused, doesn't pylint warn you about it?
Since you are importing subprocess just use that for Popen and PIPE.

+from sugar.bundle.activitybundle import ActivityBundle, MANIFEST, list_files

It doesn't make sense for activitybundle to expose something a generic
as list_files, especially since you are just using it to walk files
inside that module.

I think the MANIFEST definition is overkill and you are using it
inconsistently anyway. Just use the string.

+    def __init__(self, bundle_name, source_dir=None, dist_dir = None,
+                 dist_name = None):

As I explained in my previous mail, I want to keep passing in the
config here. Please remove the extra params.

+    def fix_manifest(self):
+        allfiles =  list_files(self.config.source_dir,
+                          ignore_dirs=['dist', '.git'],
+                          ignore_files=['.gitignore', 'MANIFEST',
+                                        '*.pyc', '*~', '*.bak'])
+        for afile in allfiles:
+            if afile not in self.config.bundle.manifest:
+                self.config.bundle.manifest.append(afile)
+        manifestfile = open(os.path.join(self.config.source_dir,
+                                         MANIFEST),
+                            "wb")
+        for line in self.config.bundle.manifest:
+            manifestfile.write(line + "\n")

This is all really hard to read. Some suggestions:

* Split the ignore lists out of the list_files call.
* We are never using aX for list iteration in the code. Just use f or
filename there.
* Do a manifest = self.config.bundle.manifest, you are repeating it 3 times
* s/manifestfile/manifest so that it feet in one line and you don't
need the crazy split up.
* Add a \n after "manifestfile = open..."

     def get_files(self):
-        return list_files(self.config.source_dir,
-                          ignore_dirs=['locale', 'dist', '.git'],
-                          ignore_files=['.gitignore'])
+        git_ls = Popen('git-ls-files', stdout=PIPE, cwd=self.config.source_dir)
+        if git_ls.wait():
+            #non-0 return code - failed
+            return BuildPackager.get_files(self)
+        f = git_ls.stdout
+        files = []
+        for line in f.readlines():
+            filename = line.strip()
+            if not filename.startswith('.'):
+                files.append(filename)
+        f.close()
+        return files

Please separate groups of code with \n to make it more readable. I
don't think this comment is necessary "#non-0 return code - failed",
it's obvious if you ever used Popen.

+        from sugar import activity
+        from sugar import env

Unless you have a concrete need for this please drop it from the
patch. To fix it properly I think we need to really cleanup the
dependencies. But that's another patch.

+    def _is_dir(self, filename):
+    def _is_file(self, filename):

As you did in your last patch remove the _

+    def install(self):

I don't think installation should be expose by ActivityBundle, we need
to cleanup the dependencies. Let's remove it from this patch and we
can discuss how to do it properly

+        try:
+            f = self._get_file(MANIFEST)
+        except IOError:
+            f = None
+        if not f:
+            logging.warning("Activity directory lacks a MANIFEST file.")
+            return []

All the other code which uses _get_file, assumes it will just return
None if there is an IOError. Seems easier to just change the method to
really do so (if it doesn't already).

+        #strip trailing \n and other whitespace

I think this comment is unnecessary, it's implicit in the definition
of the strip function.

I have some more comments for the read_manifest implementation (as a
start some \n will help readability), but I have to run out now. We
can address those in the next iteration.

Thanks!
Marco