[sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Marco Pesenti Gritti
Sun Jun 8 06:08:30 EDT 2008
+from subprocess import Popen, PIPE
from optparse import OptionParser
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:
+ manifestfile = open(os.path.join(self.config.source_dir,
+ 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
* 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..."
- return list_files(self.config.source_dir,
- ignore_dirs=['locale', 'dist', '.git'],
+ 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('.'):
+ 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
+ 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.
More information about the Sugar-devel