[sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Marco Pesenti Gritti
Sun Jun 8 20:27:09 EDT 2008
* 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_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.
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
+ #list files
+ allfiles = list_files(source_dir, ignore_dirs=IGNORE_DIRS,
+ #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:
+ #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.
Setting it to None is fine, remove the comments.
+ 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.
+ if line.endswith("/"):
+ if not self.is_dir(line):
+ manifestlines[num] = ""
+ logging.warning("Bundle %s: invalid dir "
+ "entry in MANIFEST: %s"
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:
+ elif path != "MANIFEST":
+ logging.warning("Bundle %s: %s not in MANIFEST"%
+ 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"%
Can you explain the reason of this?
More information about the Sugar-devel