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

[Marco Pesenti Gritti]

Some quic

* The patch does not apply cleanly to sugar-toolkit master.
* Do you actually need to move the imports in activitybundle for
Develop or is it just a cleanup?

+IGNORE_DIRS=['dist', '.git'],
+IGNORE_FILES=['.gitignore', 'MANIFEST', '*.pyc', '*~', '*.bak']
+

Keep these in the class they are used by.

-
+

You are adding spaces there, please remove.

         for f in files:
-            if ignore_files and f not in ignore_files:
+            if not (ignore_files and
+                    [True for pat in ignore_files if fnmatch(f,pat)]):
+                #not (result matches a pattern in ignore_files, ignore it)

This was really hard to parse for me. It should be simpler if you just
build the list of files with something like "files = [n for n in names
if fnmatch(n, pattern)]" and then iterate over it.

                     dirs.remove(ignore)
-
     return result

No need to remove the \n  there.

+    def __init__(self, source_dir=None, dist_path = ""):

The dist_path split is really confusing, better to pass in both a
dist_dir and xo_name.
In the implementation please use an if instead of the or (like I did
for source_dir). The or is more compact but imo makes the thing harder
to read.

+        #list files
+        allfiles = list_files(source_dir, ignore_dirs=IGNORE_DIRS,
+                              ignore_files=IGNORE_FILES)
+
+        #add them to internal copy of MANIFEST
+        #note: duplicates/invalids already removed when creating bundle object
+        for path in allfiles:
+            if path not in manifest:
+                manifest.append(path)
+
+        #write internal copy to disk
+        manifestfile = open(os.path.join(source_dir,"MANIFEST"),"wb")
+        for line in manifest:
+            manifestfile.write(line + "\n")

All the comments in this block are redundant, the code is clear enough.

+        if git_ls.wait():
+            #fall back to MANIFEST
+            return BuildPackager.get_files(self)

Fallback to MANIFEST doesn't make sense. We should fallback to the
list_files which you are removing instead.

+        #cleanup and return

Please avoid this kind of comments, they don't add anything.

+        return [file.strip() for file in files if not file.startswith('.')]

What's the reason to skip  .* ?

+        #we can either do
+        #pyXXXXlint: disable-msg=W0201
+        #disables "Attribute %r defined outside __init__" msg.
+        #(remove XXXX to enable), or we can do:
+        self.manifest = None #which is meaningless but shuts pylint up.
+        self.read_manifest()

Setting it to None is fine, remove the comments.

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

This should be a None check... see previous review for the reasoning.

+        for num, line in enumerate(manifestlines):
+            if line:

s/manifestlines/lines, it's clear from the context.

Can line ever be None there? I haven't checked by I'd not expect
readlines() to return Nones.

+                #check...
+                if line.endswith("/"):
+                    #...dirs...
+                    if not self.is_dir(line):
+                        manifestlines[num] = ""
+                        logging.warning("Bundle %s: invalid dir "
+                                        "entry in MANIFEST: %s"
+                                        %(self._name,line))

Do we require a trailing / for directories?

+        #remove trailing newlines - unlike internal newlines,
+        #  they do not help keep absolute position
+        while manifestlines and manifestlines[-1] == "":
+            manifestlines = manifestlines[:-1]
+        self.manifest = manifestlines

Can you explain the absolute position comment?

+    def unpack(self, install_dir, strict_manifest=False):

What is the use case of strict_manifest?

+        #check installed files against the MANIFEST
+        manifestfiles = self.get_files(self._raw_manifest())
+        paths  = []
+        for root, dirs, files in os.walk(install_path):
+            for f in files:
+                rel_path = root[len(base_dir) + 1:]
+                paths.append(os.path.join(rel_path, f))
+        for path in paths:
+            if path in manifestfiles:
+                manifestfiles.remove(path)
+            elif path != "MANIFEST":
+                logging.warning("Bundle %s: %s not in MANIFEST"%
+                                (self._name,path))
+                if strict_manifest:
+                    os.remove(os.path.join(install_path, path))

Some \n please :)

+        #create empty directories
+        for adir in self.get_dirs():
+            dirpath = os.path.join(install_path, adir)
+            if os.path.isdir(dirpath):
+                logging.warning("Bunldle %s: non-empty dir %s in MANIFEST"%
+                                                       (self._name,dirpath))
+            else:
+                os.makedirs(dirpath)

Can you explain the reason of this?

Thanks,
Marco