[sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Jameson "Chema" Quinn
jquinn at cs.oberlin.edu
Mon Jun 9 13:00:51 EDT 2008
On Sun, Jun 8, 2008 at 6:27 PM, Marco Pesenti Gritti <mpgritti at gmail.com>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.laptop.org/pipermail/sugar/attachments/20080609/d6671bc3/attachment-0001.htm
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bundleMANIFEST.formarco2.patch
Type: text/x-diff
Size: 16269 bytes
Desc: not available
Url : http://lists.laptop.org/pipermail/sugar/attachments/20080609/d6671bc3/attachment-0001.patch
More information about the Sugar
mailing list