[sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function

[Jameson "Chema" Quinn]

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