Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Landed with some cleanup and little fixes. Please test it and see if I broke anything in the process. I removed is_dir (and the po check which use it) because the return True was too much of an hack. Happy to consider a better fix for that as a separate patch. Thanks! Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
As a little note so I don't forget: In the latest version Sucrose 0.82.2 when you did a ./setup release the bundle number did not get updated. Not sure how much was refactored now. I can file a ticket as well. Simon Marco Pesenti Gritti wrote: Also we need to provide commands to generate the MANIFEST. Marco On Fri, Jun 13, 2008 at 12:42 PM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: Landed with some cleanup and little fixes. Please test it and see if I broke anything in the process. I removed is_dir (and the po check which use it) because the return True was too much of an hack. Happy to consider a better fix for that as a separate patch. Thanks! Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Please open a ticket yeah. This patch did not touch that code. Marco On Fri, Jun 13, 2008 at 12:56 PM, Simon Schampijer [EMAIL PROTECTED] wrote: As a little note so I don't forget: In the latest version Sucrose 0.82.2 when you did a ./setup release the bundle number did not get updated. Not sure how much was refactored now. I can file a ticket as well. Simon Marco Pesenti Gritti wrote: Also we need to provide commands to generate the MANIFEST. Marco On Fri, Jun 13, 2008 at 12:42 PM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: Landed with some cleanup and little fixes. Please test it and see if I broke anything in the process. I removed is_dir (and the po check which use it) because the return True was too much of an hack. Happy to consider a better fix for that as a separate patch. Thanks! Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
here you go. Sorry for the delay.
On Tue, Jun 10, 2008 at 2:51 PM, Marco Pesenti Gritti [EMAIL PROTECTED]
wrote:
On Mon, Jun 9, 2008 at 7:00 PM, Jameson Chema Quinn
[EMAIL PROTECTED] wrote:
Here is the new version of the patch. Tell me if there are any problems
applying it cleanly, and I will fix.
[EMAIL PROTECTED] sugar-toolkit]$ patch -p1
/home/marco/Download/bundleMANIFEST.formarco2.patch
patching file src/sugar/activity/bundlebuilder.py
Hunk #5 FAILED at 113.
1 out of 6 hunks FAILED -- saving rejects to file
src/sugar/activity/bundlebuilder.py.rej
patching file src/sugar/bundle/activitybundle.py
patching file src/sugar/bundle/bundle.py
Marco
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
From 1ce15e72cfa4a5c7aca081cdeb575cad74905520 Mon Sep 17 00:00:00 2001
From: chema [EMAIL PROTECTED](none)
Date: Thu, 12 Jun 2008 17:18:13 -0600
Subject: [PATCH] use MANIFEST. Deprecate bundle_name. fix_manifest().
bundlebuilder.config() cleanup.
---
src/sugar/activity/bundlebuilder.py | 122 ++-
src/sugar/bundle/activitybundle.py | 113
src/sugar/bundle/bundle.py | 32 -
3 files changed, 207 insertions(+), 60 deletions(-)
diff --git a/src/sugar/activity/bundlebuilder.py b/src/sugar/activity/bundlebuilder.py
index bf21085..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')
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 3:51 AM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: Sorry, Marco, but here is yet another version of the patch. I decided to refactor the Config class out of bundlebuilder - all it was carrying in the common start() case was bundle_name; and as for other cases, it is simpler to call Builder(...params...) than Builder(Config(...params...)). I'm using a config object because it's easier to carry around in the other Xer. I don't think having to create the Config is a big deal, calling it directly is not the primary use case anyway. But i you *really* feel that we need this bit of convenience, we can add a create_builder() which setups the config for you. Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 3:51 AM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: Sorry, Marco, but here is yet another version of the patch. I decided to refactor the Config class out of bundlebuilder - all it was carrying in the common start() case was bundle_name; and as for other cases, it is simpler to call Builder(...params...) than Builder(Config(...params...)). I'm not convinced that's a good idea. But anyway, let's not increase the scope of the patch, as I told you I already had problems to review it in his current form. I'll review the old versions of the patch and when that's done we can consider this change. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
-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
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Some quick responses, before I dig into this...
On Sun, Jun 8, 2008 at 4:08 AM, Marco Pesenti Gritti [EMAIL PROTECTED]
wrote:
-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?
Brain fart. I read pylint output, said OK, there are warnings about unused
imports, but which unused imports? :) Actually, this is fixed in later
patches I sent. Will fix.
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.
So I should duplicate the code of list_files? (This is my biggest question
with your response)
I think the MANIFEST definition is overkill and you are using it
inconsistently anyway. Just use the string.
As the number of metadata files in the activity format grows, it seems
likely that eventually we'll give them their own directory. Making this a
global now is the first step to being able to change it later. But if you
prefer, I'll use a string for now.
+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.
disagree a bit, but will do.
+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...
will do
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.
will do
+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.
will do
+def _is_dir(self, filename):
+def _is_file(self, filename):
As you did in your last patch remove the _
will do
+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
install was already exposed, I just refactored unpack out of it (and thus
also exposed unpack). This only showed up in the patch because I put unpack
first, and the body of unpack matched with that part of the body of install.
I'm pretty sure this function is used by Journal, we can't just drop it.
+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).
Will do.
+#strip trailing \n and other whitespace
I think this comment is unnecessary, it's implicit in the definition
of the strip function.
Will do.
I
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 1:59 PM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: 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. So I should duplicate the code of list_files? (This is my biggest question with your response) Unless I'm missing something you won't really duplicate much of it, since you are not using the ignore_* which are the big part of the code and the actual purpose of that function. You will basically just need the for... I think the MANIFEST definition is overkill and you are using it inconsistently anyway. Just use the string. As the number of metadata files in the activity format grows, it seems likely that eventually we'll give them their own directory. Making this a global now is the first step to being able to change it later. But if you prefer, I'll use a string for now. If you move it to a directory you will better construct to the path with os.path.join, so it would probably be a method of Bundle. Anyway I don't think it's worth to worry about that yet, it's easy enough to refactor if/when necessary. +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. disagree a bit, but will do. Just look at all the ugly None checks you need to do to support it... :) +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 install was already exposed, I just refactored unpack out of it (and thus also exposed unpack). This only showed up in the patch because I put unpack first, and the body of unpack matched with that part of the body of install. I'm pretty sure this function is used by Journal, we can't just drop it. You are right, That's ok then. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Here is the revised patch. It has your suggested changes, plus a couple more: - Check for existence of po directory in Builder - Config.__init__() is cleaned up. Now gets bundle name from activity.info. start() no longer needs a bundle name, and has deprecation warning. Also I put things in a more logical order. On Sun, Jun 8, 2008 at 7:11 AM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: On Sun, Jun 8, 2008 at 1:59 PM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: 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. So I should duplicate the code of list_files? (This is my biggest question with your response) Unless I'm missing something you won't really duplicate much of it, since you are not using the ignore_* which are the big part of the code and the actual purpose of that function. You will basically just need the for... I think the MANIFEST definition is overkill and you are using it inconsistently anyway. Just use the string. As the number of metadata files in the activity format grows, it seems likely that eventually we'll give them their own directory. Making this a global now is the first step to being able to change it later. But if you prefer, I'll use a string for now. If you move it to a directory you will better construct to the path with os.path.join, so it would probably be a method of Bundle. Anyway I don't think it's worth to worry about that yet, it's easy enough to refactor if/when necessary. +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. disagree a bit, but will do. Just look at all the ugly None checks you need to do to support it... :) +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 install was already exposed, I just refactored unpack out of it (and thus also exposed unpack). This only showed up in the patch because I put unpack first, and the body of unpack matched with that part of the body of install. I'm pretty sure this function is used by Journal, we can't just drop it. You are right, That's ok then. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar diff --git a/src/sugar/activity/bundlebuilder.py b/src/sugar/activity/bundlebuilder.py index 1063f72..c16845d 100644 --- a/src/sugar/activity/bundlebuilder.py +++ b/src/sugar/activity/bundlebuilder.py @@ -19,53 +19,52 @@ import os import zipfile import tarfile import shutil -import subprocess import re import gettext from optparse import OptionParser +import subprocess +import logging from sugar import env from sugar.bundle.activitybundle import ActivityBundle +IGNORE_DIRS=['dist', '.git'], +IGNORE_FILES=['.gitignore', 'MANIFEST', '*.pyc', '*~', '*.bak'] + def list_files(base_dir, ignore_dirs=None, ignore_files=None): result = [] - + for root, dirs, files in os.walk(base_dir): 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) rel_path = root[len(base_dir) + 1:] result.append(os.path.join(rel_path, f)) if ignore_dirs and root == base_dir: for ignore in ignore_dirs: if ignore in dirs: dirs.remove(ignore) - return result class Config(object): -def __init__(self, bundle_name, source_dir=None): -if source_dir: -self.source_dir = source_dir -else: -self.source_dir = os.getcwd() +def __init__(self, source_dir=None, dist_path = ): +self.source_dir = source_dir or 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) +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_name = reduce(lambda x, y:x+y, self.activity_name.split()) + +dist_dir, dist_name = os.path.split(dist_path) +self.dist_dir = dist_dir or os.path.join(self.source_dir, 'dist') +self.xo_name = dist_name or '%s-%d.xo' %
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 9:26 PM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: Here is the revised patch. It has your suggested changes, plus a couple more: - Check for existence of po directory in Builder - Config.__init__() is cleaned up. Now gets bundle name from activity.info. start() no longer needs a bundle name, and has deprecation warning. Also I put things in a more logical order. Jameson, *please* do *not* make any additional changes when submitting a new patch. Limit yourself to the changes requested by the reviewer. Additional fixes/improvements should go in a separate patch. I'll review it as is this time, but next time I'm going to ask you to take out the additional changes. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Also, you left this in. +print path Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
I'm still getting a few pylint errors, you don't?. Also can you please answer the question/notes I posted before I go ahead an do a full review? Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
I'm away from my machine, but quick response, I'll say more later. On Sat, Jun 7, 2008 at 6:51 AM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: I'm still getting a few pylint errors, you don't?. I was just getting defined outside of __init__ for ActivityBundle.manifest ... I don't know how I'm supposed to fix that, since it is defined from a subroutine of __init__ that is also useful elsewhere. Also can you please answer the question/notes I posted before I go ahead an do a full review? OK, here: * Please run pylint on the changed files. You can use sugar-jhbuild/scripts/data /pylintrc. That will catch several problems and nitpicks that I'd ask you to fix anyway. Done, I think, I will check again later. * I dont understand all the warnings.warn(MalformedBundleException(...)). What's the purpose of those. Shouldn't they be real exceptions? What's the advantage of using the warnings module over logging.warning? fixed in the patch I reposted * I'm not sure what you are using dest_dir for, but it looks like it should be part of Config. again, fixed. * Why are you moving the activity and env imports inline. I answered that in my previous email: to make it easier to use these files without having the rest of Sugar installed. ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
bundleManifest.all2.patch /*here*/ = (bundleMANIFEST.patch +
bundleManifest.subpatch + bundleMANIFEST.sub2.patch /*here*/)
bundleMANIFEST.sub2.patch makes bundle.Bundle.is_file public, makes version
an attribute of bundlebuilder.RawActivity (and all children), and fixes a
minor versioning bug in cmd_release.
On Sat, Jun 7, 2008 at 7:54 PM, Jameson Chema Quinn [EMAIL PROTECTED]
wrote:
oops, I left out the more complete patch. As I said,
bundleManifest.all.patch /*here*/ = (bundleMANIFEST.patch /*15K, included
in msg 3 of this thread*/ + bundleManifest.subpatch /*included in previous
message*/)
diff --git a/src/sugar/activity/bundlebuilder.py b/src/sugar/activity/bundlebuilder.py
index 1063f72..1b682a2 100644
--- a/src/sugar/activity/bundlebuilder.py
+++ b/src/sugar/activity/bundlebuilder.py
@@ -19,64 +19,45 @@ import os
import zipfile
import tarfile
import shutil
-import subprocess
+from subprocess import Popen, PIPE
import re
import gettext
from optparse import OptionParser
+import subprocess
+import logging
from sugar import env
-from sugar.bundle.activitybundle import ActivityBundle
-
-def list_files(base_dir, ignore_dirs=None, ignore_files=None):
-result = []
-
-for root, dirs, files in os.walk(base_dir):
-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))
-if ignore_dirs and root == base_dir:
-for ignore in ignore_dirs:
-if ignore in dirs:
-dirs.remove(ignore)
-
-return result
-
-class Config(object):
-def __init__(self, bundle_name, source_dir=None):
-if source_dir:
-self.source_dir = source_dir
-else:
-self.source_dir = os.getcwd()
-
-
-bundle = ActivityBundle(self.source_dir)
-version = bundle.get_activity_version()
+from sugar.bundle.activitybundle import ActivityBundle, MANIFEST, list_files
-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)
+class RawActivity(object):
+def __init__(self, source_dir=None, dist_path = ):
+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_name = reduce(lambda x, y:x+y, self.activity_name.split())
+
+dist_dir, dist_name = os.path.split(dist_path)
+self.dist_dir = dist_dir or os.path.join(self.source_dir, 'dist')
+self.xo_name = dist_name or '%s-%d.xo' % (self.bundle_name,
+ self.version)
+self.tarball_name = (dist_name or
+ '%s-%d.tar.bz2' % (self.bundle_name,
+self.version))
self.bundle_root_dir = self.bundle_name + '.activity'
-self.tarball_root_dir = '%s-%d' % (self.bundle_name, version)
-
-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)
-
-class Builder(object):
-def __init__(self, config):
-self.config = config
-
+self.tarball_root_dir = '%s-%d' % (self.bundle_name, self.version)
+
+class Builder(RawActivity):
def build(self):
self.build_locale()
def build_locale(self):
-po_dir = os.path.join(self.config.source_dir, 'po')
-
+if not self.bundle.is_dir('po'):
+logging.warn(Activity lacks a po directory for translations)
+return
+po_dir = os.path.join(self.source_dir, 'po')
for f in os.listdir(po_dir):
if not f.endswith('.po'):
continue
@@ -84,78 +65,92 @@ class Builder(object):
file_name = os.path.join(po_dir, f)
lang = f[:-3]
-localedir = os.path.join(self.config.source_dir, 'locale', lang)
+localedir = os.path.join(self.source_dir, 'locale', lang)
mo_path = os.path.join(localedir, 'LC_MESSAGES')
if not os.path.isdir(mo_path):
os.makedirs(mo_path)
-mo_file = os.path.join(mo_path, %s.mo % self.config.bundle_id)
+mo_file = os.path.join(mo_path, %s.mo % self.bundle_id)
args = [msgfmt, --output-file=%s % mo_file, file_name]
retcode = subprocess.call(args)
if retcode:
print 'ERROR - msgfmt failed with return code %i.' % retcode
cat = gettext.GNUTranslations(open(mo_file, 'r'))
-
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Changes: bundle.py Added zip-agnostic _is_file() method to bundle class; used in activitybundle for checking MANIFEST without unpacking activitybundle.py Moved basic logic for processing MANIFEST, including list_files, to activitybundle.py ; this is necessary for checking when installing a bundle. moved dependencies on activity registry and env to inline imports in relevant methods; some bundle tasks should be doable with just these three files. (This is doing only half of the job, the other half would be falling back to importing the other modules not as submodules). some simple globbing in list_files for ignored files, to allow ignoring *.pyc. reads manifest into a list of lines; then replaces invalid lines in place with blank lines. This allows fix_manifest in bundlebuilder to leave all existing files on same MANIFEST line number, which will help with diffs later. separate unpack from install (= unpack + register). Unpack checks for valid MANIFEST but is set to only complain, not throw exceptions, by default, to give folks time to clean up. bundlebuilder.py: fix_manifest function rewrites MANIFEST file. SourcePackager.get_files calls out to git to get file names, or falls back to MANIFEST otherwise. On Fri, Jun 6, 2008 at 1:24 PM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: On Fri, Jun 6, 2008 at 6:13 PM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: Note: this patch does not expose the fix_manifest function when running bundlebuilder as a script. That would be a 2 or 3 line patch, seperately. Some initial notes: * Please run pylint on the changed files. You can use sugar-jhbuild/scripts/data/pylintrc. That will catch several problems and nitpicks that I'd ask you to fix anyway. * I dont understand all the warnings.warn(MalformedBundleException(...)). What's the purpose of those. Shouldn't they be real exceptions? What's the advantage of using the warnings module over logging.warning? * I'm not sure what you are using dest_dir for, but it looks like it should be part of Config. * Why are you moving the activity and env imports inline. As we discussed in irc, it would be good to split up patches. But anyway in the case of big patches like this, containing unrelated changes is good to provide a list of the changes you made and the reason you made them (can you provide such list when you resubmit the patch with pylint fixed? Thanks!). Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar diff --git a/src/sugar/activity/bundlebuilder.py b/src/sugar/activity/bundlebuilder.py index 1063f72..3c8f7dc 100644 --- a/src/sugar/activity/bundlebuilder.py +++ b/src/sugar/activity/bundlebuilder.py @@ -19,46 +19,36 @@ import os import zipfile import tarfile import shutil -import subprocess +from subprocess import Popen, PIPE import re import gettext from optparse import OptionParser +import warnings +import subprocess from sugar import env -from sugar.bundle.activitybundle import ActivityBundle - -def list_files(base_dir, ignore_dirs=None, ignore_files=None): -result = [] +from sugar.bundle.activitybundle import ActivityBundle, MANIFEST, list_files -for root, dirs, files in os.walk(base_dir): -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)) -if ignore_dirs and root == base_dir: -for ignore in ignore_dirs: -if ignore in dirs: -dirs.remove(ignore) - -return result class Config(object): -def __init__(self, bundle_name, source_dir=None): -if source_dir: -self.source_dir = source_dir -else: -self.source_dir = os.getcwd() +def __init__(self, bundle_name, source_dir=None, dist_dir = None, + dist_name = None): +self.source_dir = source_dir or os.getcwd() +self.dist_dir = dist_dir or os.path.join(self.source_dir, 'dist') bundle = ActivityBundle(self.source_dir) version = bundle.get_activity_version() +self.bundle = bundle 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) +self.xo_name = dist_name or '%s-%d.xo' % (self.bundle_name, version) +self.tarball_name = (dist_name or + '%s-%d.tar.bz2' % (self.bundle_name, version)) 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) + info_path = os.path.join(self.source_dir, 'activity', 'activity.info') f = open(info_path,'r') @@ -105,48 +95,65 @@ class Builder(object): class Packager(object):
