On Sun, Jun 8, 2008 at 6:27 PM, Marco Pesenti Gritti <[EMAIL PROTECTED]>
wrote:
> Some quic
>
> * The patch does not apply cleanly to sugar-toolkit master.
I'll be more careful with my git diff.
>
> * Do you actually need to move the imports in activitybundle for
> Develop or is it just a cleanup?
No, removed.
>
>
> +IGNORE_DIRS=['dist', '.git'],
> +IGNORE_FILES=['.gitignore', 'MANIFEST', '*.pyc', '*~', '*.bak']
> +
>
> Keep these in the class they are used by.
>
check
>
> -
> +
>
> 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.
check. I could also change the "for ..:.. append" to a "..+= [..for..] if
you want
>
>
> 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.
check.
>
>
> + #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.
The thing about duplicate/invalid actually matters, leaving just that.
>
>
> + 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.
check
>
>
> + #cleanup and return
>
> Please avoid this kind of comments, they don't add anything.
check.
>
>
> + return [file.strip() for file in files if not
> file.startswith('.')]
>
> What's the reason to skip .* ?
This is the logic from the old version of bundlebuilder. No preference,
myself. I suspect that it is for .gitignore .
>
>
> + #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.
I left just one comment.
>
>
> + 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.
check.
>
>
> + for num, line in enumerate(manifestlines):
> + if line:
>
> s/manifestlines/lines, it's clear from the context.
check.
>
>
> Can line ever be None there? I haven't checked by I'd not expect
> readlines() to return Nones.
Not None; "", which is also false.
>
>
> + #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?
It is for version control.
Version x: ["activity/activity.info", "main.py", "extra.py", "sillyname.py"]
...
rm extra.py; mv sillyname.py bettername.py
...
version x+1: ["activity/activity.info", "main.py", "", "bettername.py"]
thus the "" in the middle is useful. "" on the end, however, is not.
>
>
> + def unpack(self, install_dir, strict_manifest=False):
>
> What is the use case of strict_manifest?
Intended to be turned on by default later, once current activities are
fixed. It will encourages proper use of MANIFEST. The principle is SPOT -
single point of truth - and it will reduce the amount of fix_manifest I have
to do in Develop to avoid data loss. fix_manifest too often defeats the
purpose of MANIFEST, we might as well be including implicitly.
>
>
> + #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?
On second thought, there is no good reason. Removed.
> Also, you left this in.
>
> + print path
fixed
Here is the new version of the patch. Tell me if there are any problems
applying it cleanly, and I will fix.
Jameson
diff --git a/src/sugar/activity/bundlebuilder.py b/src/sugar/activity/bundlebuilder.py
index 7a89ab4..d7e4bfd 100644
--- a/src/sugar/activity/bundlebuilder.py
+++ b/src/sugar/activity/bundlebuilder.py
@@ -23,6 +23,8 @@ import subprocess
import re
import gettext
from optparse import OptionParser
+import logging
+from fnmatch import fnmatch
from sugar import env
from sugar.bundle.activitybundle import ActivityBundle
@@ -31,10 +33,14 @@ def list_files(base_dir, ignore_dirs=None, ignore_files=None):
result = []
for root, dirs, files in os.walk(base_dir):
+ if ignore_files:
+ for pattern in ignore_files:
+ files = [f for f in files if not fnmatch(f, pattern)]
+
+ rel_path = root[len(base_dir) + 1:]
for f in files:
- if ignore_files and f not in ignore_files:
- rel_path = root[len(base_dir) + 1:]
- result.append(os.path.join(rel_path, f))
+ result.append(os.path.join(rel_path, f))
+
if ignore_dirs and root == base_dir:
for ignore in ignore_dirs:
if ignore in dirs:
@@ -43,25 +49,27 @@ def list_files(base_dir, ignore_dirs=None, ignore_files=None):
return result
class Config(object):
- def __init__(self, bundle_name):
- self.source_dir = os.getcwd()
-
- bundle = ActivityBundle(self.source_dir)
- version = bundle.get_activity_version()
-
- self.bundle_name = bundle_name
- self.xo_name = '%s-%d.xo' % (self.bundle_name, version)
- self.tarball_name = '%s-%d.tar.bz2' % (self.bundle_name, version)
+ def __init__(self, source_dir=None, dist_dir = None, dist_name = None):
+ self.source_dir = source_dir or os.getcwd()
+
+ self.bundle = bundle = ActivityBundle(self.source_dir)
+ self.version = bundle.get_activity_version()
+ self.activity_name = bundle.get_name()
self.bundle_id = bundle.get_bundle_id()
- self.bundle_root_dir = self.bundle_name + '.activity'
- self.tarball_root_dir = '%s-%d' % (self.bundle_name, version)
+ self.bundle_name = reduce(lambda x, y:x+y, self.activity_name.split())
- info_path = os.path.join(self.source_dir, 'activity', 'activity.info')
- f = open(info_path,'r')
- info = f.read()
- f.close()
- match = re.search('^name\s*=\s*(.*)$', info, flags = re.MULTILINE)
- self.activity_name = match.group(1)
+ if dist_dir:
+ self.dist_dir = dist_dir
+ else:
+ self.dist_dir = os.path.join(self.source_dir, 'dist')
+
+ if dist_name:
+ self.xo_name = self.tar_name = dist_name
+ else:
+ self.xo_name = '%s-%d.xo' % (self.bundle_name, self.version)
+ self.tar_name = '%s-%d.tar.bz2' % (self.bundle_name, self.version)
+ self.bundle_root_dir = self.bundle_name + '.activity'
+ self.tar_root_dir = '%s-%d' % (self.bundle_name, self.version)
class Builder(object):
def __init__(self, config):
@@ -71,6 +79,10 @@ class Builder(object):
self.build_locale()
def build_locale(self):
+ if not self.config.bundle.is_dir('po'):
+ logging.warn("Activity lacks a po directory for translations")
+ return
+
po_dir = os.path.join(self.config.source_dir, 'po')
for f in os.listdir(po_dir):
@@ -101,54 +113,74 @@ class Builder(object):
class Packager(object):
def __init__(self, config):
self.config = config
- self.dist_dir = os.path.join(self.config.source_dir, 'dist')
self.package_path = None
- if not os.path.exists(self.dist_dir):
- os.mkdir(self.dist_dir)
+ if not os.path.exists(self.config.dist_dir):
+ os.mkdir(self.config.dist_dir)
class BuildPackager(Packager):
- def __init__(self, config):
- Packager.__init__(self, config)
- self.build_dir = self.config.source_dir
-
def get_files(self):
- return list_files(self.build_dir,
- ignore_dirs=['po', 'dist', '.git'],
- ignore_files=['.gitignore'])
+ return self.config.bundle.get_files()
+
+ def _list_useful_files(self):
+ ignore_dirs = ['dist', '.git'],
+ ignore_files = ['.gitignore', 'MANIFEST', '*.pyc', '*~', '*.bak']
+
+ return list_files(self.config.source_dir, ignore_dirs, ignore_files)
+
+ def fix_manifest(self):
+ manifest = self.config.bundle.manifest
+
+ allfiles = self._list_useful_files()
+
+ for path in allfiles:
+ if path not in manifest:
+ manifest.append(path)
+ #note: duplicate/invalid entries already gone from bundle.manifest
+
+ f = open(os.path.join(self.config.source_dir, "MANIFEST"), "wb")
+ for line in manifest:
+ f.write(line + "\n")
class XOPackager(BuildPackager):
def __init__(self, config):
BuildPackager.__init__(self, config)
- self.package_path = os.path.join(self.dist_dir, self.config.xo_name)
+ self.package_path = os.path.join(self.config.dist_dir,
+ self.config.xo_name)
def package(self):
bundle_zip = zipfile.ZipFile(self.package_path, 'w',
zipfile.ZIP_DEFLATED)
for f in self.get_files():
- bundle_zip.write(os.path.join(self.build_dir, f),
+ bundle_zip.write(os.path.join(self.config.source_dir, f),
os.path.join(self.config.bundle_root_dir, f))
bundle_zip.close()
-class SourcePackager(Packager):
+class SourcePackager(BuildPackager):
def __init__(self, config):
- Packager.__init__(self, config)
- self.package_path = os.path.join(self.dist_dir,
- self.config.tarball_name)
+ BuildPackager.__init__(self, config)
+ self.package_path = os.path.join(self.config.dist_dir,
+ self.config.tar_name)
def get_files(self):
- return list_files(self.config.source_dir,
- ignore_dirs=['locale', 'dist', '.git'],
- ignore_files=['.gitignore'])
+ #list files in git
+ git_ls = subprocess.Popen('git-ls-files', stdout=subprocess.PIPE,
+ cwd=self.config.source_dir)
+ if git_ls.wait():
+ #fall back to filtered list
+ return self._list_useful_files()
+ paths = git_ls.stdout.readlines()
+
+ return [path.strip() for path in paths if not path.startswith('.')]
def package(self):
tar = tarfile.open(self.package_path, "w")
for f in self.get_files():
tar.add(os.path.join(self.config.source_dir, f),
- os.path.join(self.config.tarball_root_dir, f))
+ os.path.join(self.config.tar_root_dir, f))
tar.close()
def cmd_help(config, options, args):
@@ -331,11 +363,13 @@ def cmd_build(config, options, args):
builder = Builder(config)
builder.build()
-def start(bundle_name):
+def start(bundle_name=None):
+ if bundle_name:
+ logging.warn("bundle_name deprecated, now comes from activity.info")
parser = OptionParser()
(options, args) = parser.parse_args()
- config = Config(bundle_name)
+ config = Config()
try:
globals()['cmd_' + args[0]](config, options, args[1:])
diff --git a/src/sugar/bundle/activitybundle.py b/src/sugar/bundle/activitybundle.py
index db30555..8dba9ba 100644
--- a/src/sugar/bundle/activitybundle.py
+++ b/src/sugar/bundle/activitybundle.py
@@ -56,7 +56,7 @@ class ActivityBundle(Bundle):
self._show_launcher = True
self._activity_version = 0
- info_file = self._get_file('activity/activity.info')
+ info_file = self.get_file('activity/activity.info')
if info_file is None:
raise MalformedBundleException('No activity.info file')
self._parse_info(info_file)
@@ -65,6 +65,62 @@ class ActivityBundle(Bundle):
if linfo_file:
self._parse_linfo(linfo_file)
+ self.manifest = None #This should be replaced by following function
+ self.read_manifest()
+
+ def _raw_manifest(self):
+ f = self.get_file("MANIFEST")
+ if not f:
+ logging.warning("Activity directory lacks a MANIFEST file.")
+ return []
+
+ ret = [line.strip() for line in f.readlines()]
+ f.close()
+ return ret
+
+ def read_manifest(self):
+ """read_manifest: sets self.manifest to list of lines in MANIFEST,
+ with invalid lines replaced by empty lines.
+
+ Since absolute order carries information on file history, it should
+ be preserved.
+ For instance, when renaming a file, you should leave the new name
+ on the same line as the old one.
+ """
+ lines = self._raw_manifest()
+ for num, line in enumerate(lines):
+ if line:
+ #remove duplicates
+ if line in lines[0:num]:
+ lines[num] = ""
+ logging.warning("Bundle %s: duplicate entry in MANIFEST: %s"
+ %(self._name,line))
+ continue
+
+ #remove MANIFEST
+ if line == "MANIFEST":
+ lines[num] = ""
+ logging.warning("Bundle %s: MANIFEST includes itself: %s"
+ %(self._name,line))
+
+ #check files
+ if not self.is_file(line):
+ lines[num] = ""
+ logging.warning("Bundle %s: "
+ "invalid entry in MANIFEST: %s"
+ %(self._name,line))
+
+ #remove trailing newlines - unlike internal newlines,
+ # they do not help keep absolute position
+ while lines and lines[-1] == "":
+ lines = lines[:-1]
+ self.manifest = lines
+
+ def get_files(self, manifest = None):
+ manifestpath = ["MANIFEST"] if self.is_file("MANIFEST") else []
+ return manifestpath + [line for line in (manifest or self.manifest)
+ if line]
+
def _parse_info(self, info_file):
cp = ConfigParser()
cp.readfp(info_file)
@@ -123,12 +179,12 @@ class ActivityBundle(Bundle):
return None
linfo_path = os.path.join('locale', lang, 'activity.linfo')
- linfo_file = self._get_file(linfo_path)
+ linfo_file = self.get_file(linfo_path)
if linfo_file is not None:
return linfo_file
linfo_path = os.path.join('locale', lang[:2], 'activity.linfo')
- linfo_file = self._get_file(linfo_path)
+ linfo_file = self.get_file(linfo_path)
if linfo_file is not None:
return linfo_file
@@ -180,7 +236,7 @@ class ActivityBundle(Bundle):
if self._unpacked:
return os.path.join(self._path, icon_path)
else:
- icon_data = self._get_file(icon_path).read()
+ icon_data = self.get_file(icon_path).read()
temp_file, temp_file_path = tempfile.mkstemp(self._icon)
os.write(temp_file, icon_data)
os.close(temp_file)
@@ -220,17 +276,38 @@ class ActivityBundle(Bundle):
return True
else:
return False
-
- def install(self):
- activities_path = env.get_user_activities_path()
- act = activity.get_registry().get_activity(self._bundle_id)
- if act is not None and act.path.startswith(activities_path):
- raise AlreadyInstalledException
-
- install_dir = env.get_user_activities_path()
+
+ def unpack(self, install_dir, strict_manifest=False):
self._unzip(install_dir)
install_path = os.path.join(install_dir, self._zip_root_dir)
+
+ #list installed files
+ manifestfiles = self.get_files(self._raw_manifest())
+ paths = []
+ for root, dirs, files in os.walk(install_path):
+ rel_path = root[len(install_path) + 1:]
+ for f in files:
+ paths.append(os.path.join(rel_path, f))
+
+ #check list against the MANIFEST
+ 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))
+
+ #Is anything in MANIFEST left over after accounting for all files?
+ if manifestfiles:
+ err = ("Bundle %s: files in MANIFEST not included: %s"%
+ (self._name,str(manifestfiles)))
+ if strict_manifest:
+ raise MalformedBundleException(err)
+ else:
+ logging.warning(err)
xdg_data_home = os.getenv('XDG_DATA_HOME',
os.path.expanduser('~/.local/share'))
@@ -267,11 +344,21 @@ class ActivityBundle(Bundle):
os.symlink(info_file,
os.path.join(installed_icons_dir,
os.path.basename(info_file)))
+ return install_path
+ def install(self):
+ activities_path = env.get_user_activities_path()
+ act = activity.get_registry().get_activity(self._bundle_id)
+ if act is not None and act.path.startswith(activities_path):
+ raise AlreadyInstalledException
+
+ install_dir = env.get_user_activities_path()
+ install_path = self.unpack(install_dir)
+
if not activity.get_registry().add_bundle(install_path):
raise RegistrationException
- def uninstall(self, force=False):
+ def uninstall(self, force=False):
if self._unpacked:
install_path = self._path
else:
diff --git a/src/sugar/bundle/bundle.py b/src/sugar/bundle/bundle.py
index 47d08b2..90b511b 100644
--- a/src/sugar/bundle/bundle.py
+++ b/src/sugar/bundle/bundle.py
@@ -96,13 +96,15 @@ class Bundle:
'All files in the bundle must be inside a single ' +
'top-level directory')
- def _get_file(self, filename):
+ def get_file(self, filename):
f = None
if self._unpacked:
path = os.path.join(self._path, filename)
- if os.path.isfile(path):
- f = open(path)
+ try:
+ f = open(path,"rb")
+ except IOError:
+ return None
else:
zip_file = zipfile.ZipFile(self._path)
path = os.path.join(self._zip_root_dir, filename)
@@ -114,6 +116,30 @@ class Bundle:
zip_file.close()
return f
+
+ def is_dir(self, filename):
+ if self._unpacked:
+ path = os.path.join(self._path, filename)
+ return os.path.isdir(path)
+ else:
+ return True #zip files contain all dirs you care about!
+
+ def is_file(self, filename):
+ if self._unpacked:
+ path = os.path.join(self._path, filename)
+ return os.path.isfile(path)
+ else:
+ zip_file = zipfile.ZipFile(self._path)
+ path = os.path.join(self._zip_root_dir, filename)
+ try:
+ zip_file.getinfo(path)
+ #no exception above, so:
+ return True
+ except KeyError:
+ return False
+ finally:
+ zip_file.close()
+
def get_path(self):
"""Get the bundle path."""
_______________________________________________
Sugar mailing list
[email protected]
http://lists.laptop.org/listinfo/sugar
