Merge lp:~florent.x/openobject-server/trunk-tools-file_open into lp:openobject-server

[Florent]

Rewrite the file_open tool to manage better all kind of paths:
 - relative paths 'hr/report/timesheer.xsl'
 - absolute paths '/srv/openerp/addons/hr/report/timesheer.xsl'

If the path name is below "root_path" or below one of the "ad_paths", the zipfile lookup stops at the root of the folder managed by OpenERP.
If the path name is not below an OpenERP folder, the utility does not try to find a zip file.

It should fix the issues, until the next release, when the support for zip modules will be dropped completely.

[Olivier Dony (Odoo) (odo-openerp)]

Ouch, thanks for taking a stab at that [awful] legacy code (dating back from 2007[1]).

As I mentioned on the bug, I have a work in progress to change the file location mechanism to give higher priority to extracted/directory modules rather than zip files, for the various reasons exposed in comment #3.

This might cause a bit of conflict in your branch, but not too much normally.
I should be done very soon, and will notify you, so we can then proceed to review/merge the second part of the fix for bug 928376 - your patch.

Thanks!

[1] rev.632 revid:ced-96fb1d71e2b7282f46324766dd733aa1181e9e00 and related ones

[lp:~florent.x/openobject-server/trunk-tools-file_open]

4018. By Florent on 2012-02-08

[MERGE] file_open: give precedence to directory before zip.

[Florent (florent.x)]

>
> As I mentioned on the bug, I have a work in progress to change the file
> location mechanism to give higher priority to extracted/directory modules
> rather than zip files, for the various reasons exposed in comment #3.
>
> This might cause a bit of conflict in your branch, but not too much normally.
> I should be done very soon, and will notify you, so we can then proceed to
> review/merge the second part of the fix for bug 928376 - your patch.
>

I did some tests, and I have merged your changes in this branch.

[Olivier Dony (Odoo) (odo-openerp)]

For the record I merged this branch in an ~openerp-dev branch to have it executed by our Continuous Integration system, and the total time needed to install all standard modules and execute all the tests was not noticeably different.
I imagine this is because the .zip files that are looked up are almost always the same, and are cached as negative dentries by the kernel - causing no disk access after the first.

As the performance difference was the most important reason for this change, I would be in favor of merging it in trunk right after the release of 6.1, because it is a change of behavior and could still cause some unforeseen side-effect - and we're very late in the release process of 6.1.

Thanks!

review: Approve

[Florent (florent.x)]

AFAIK 6.1 is released now...
and this patch is still candidate for merge in trunk.

review: Approve

=== modified file 'openerp/modules/module.py'
--- openerp/modules/module.py	2012-02-08 10:59:13 +0000
+++ openerp/modules/module.py	2012-02-08 22:32:18 +0000
@@ -280,25 +280,28 @@
 def get_module_resource(module, *args):
     """Return the full path of a resource of the given module.
 
-    @param module: the module
-    @param args: the resource path components
+    :param module: module name
+    :param list(str) args: resource path components within module
 
-    @return: absolute path to the resource
+    :rtype: str
+    :return: absolute path to the resource
 
     TODO name it get_resource_path
     TODO make it available inside on osv object (self.get_resource_path)
     """
-    a = get_module_path(module)
-    if not a: return False
-    resource_path = opj(a, *args)
-    if zipfile.is_zipfile( a +'.zip') :
-        zip = zipfile.ZipFile( a + ".zip")
+    mod_path = get_module_path(module)
+    if not mod_path: return False
+    resource_path = opj(mod_path, *args)
+    if os.path.isdir(mod_path):
+        # the module is a directory - ignore zip behavior
+        if os.path.exists(resource_path):
+            return resource_path
+    elif zipfile.is_zipfile(mod_path + '.zip'):
+        zip = zipfile.ZipFile( mod_path + ".zip")
         files = ['/'.join(f.split('/')[1:]) for f in zip.namelist()]
         resource_path = '/'.join(args)
         if resource_path in files:
-            return opj(a, resource_path)
-    elif os.path.exists(resource_path):
-        return resource_path
+            return opj(mod_path, resource_path)
     return False
 
 def get_module_icon(module):

=== modified file 'openerp/tools/misc.py'
--- openerp/tools/misc.py	2012-01-25 13:24:07 +0000
+++ openerp/tools/misc.py	2012-02-08 22:32:18 +0000
@@ -134,7 +134,7 @@
     @param name name of the file
     @param mode file open mode
     @param subdir subdirectory
-    @param pathinfo if True returns tupple (fileobject, filepath)
+    @param pathinfo if True returns tuple (fileobject, filepath)
 
     @return fileobject if pathinfo is False else (fileobject, filepath)
     """
@@ -142,44 +142,60 @@
     adps = addons.module.ad_paths
     rtp = os.path.normcase(os.path.abspath(config['root_path']))
 
-    if name.replace(os.path.sep, '/').startswith('addons/'):
+    if os.path.isabs(name):
+        # It is an absolute path
+        # Is it below 'addons_path' or 'root_path'?
+        name = os.path.normcase(os.path.normpath(name))
+        for root in adps + [rtp]:
+            if name.startswith(root):
+                base = root.rstrip(os.sep)
+                name = name[len(base) + 1:]
+                break
+        else:
+            # It is outside the OpenERP root: skip zipfile lookup.
+            base, name = os.path.split(name)
+        return _fileopen(name, mode=mode, basedir=base, pathinfo=pathinfo)
+
+    if name.replace(os.sep, '/').startswith('addons/'):
         subdir = 'addons'
-        name = name[7:]
+        name2 = name[7:]
+    elif subdir:
+        name = os.path.join(subdir, name)
+        if name.replace(os.sep, '/').startswith('addons/'):
+            subdir = 'addons'
+            name2 = name[7:]
+        else:
+            name2 = name
 
-    # First try to locate in addons_path
+    # First, try to locate in addons_path
     if subdir:
-        subdir2 = subdir
-        if subdir2.replace(os.path.sep, '/').startswith('addons/'):
-            subdir2 = subdir2[7:]
-
-        subdir2 = (subdir2 != 'addons' or None) and subdir2
-
         for adp in adps:
             try:
-                if subdir2:
-                    fn = os.path.join(adp, subdir2, name)
-                else:
-                    fn = os.path.join(adp, name)
-                fn = os.path.normpath(fn)
-                fo = file_open(fn, mode=mode, subdir=None, pathinfo=pathinfo)
-                if pathinfo:
-                    return fo, fn
-                return fo
+                return _fileopen(name2, mode=mode, basedir=adp,
+                                 pathinfo=pathinfo)
             except IOError:
                 pass
 
-    if subdir:
-        name = os.path.join(rtp, subdir, name)
-    else:
-        name = os.path.join(rtp, name)
-
-    name = os.path.normpath(name)
-
-    # Check for a zipfile in the path
-    head = name
+    # Second, try to locate in root_path
+    return _fileopen(name, mode=mode, basedir=rtp, pathinfo=pathinfo)
+
+
+def _fileopen(path, mode, basedir, pathinfo):
+    name = os.path.normpath(os.path.join(basedir, path))
+    # Give higher priority to module directories, which is
+    # a more common case than zipped modules.
+    if os.path.isfile(name):
+        fo = open(name, mode)
+        if pathinfo:
+            return (fo, name)
+        return fo
+
+    # Support for loading modules in zipped form.
+    # This will not work for zipped modules that are sitting
+    # outside of known addons paths.
+    head = os.path.normpath(path)
     zipname = False
-    name2 = False
-    while True:
+    while os.sep in head:
         head, tail = os.path.split(head)
         if not tail:
             break
@@ -187,9 +203,10 @@
             zipname = os.path.join(tail, zipname)
         else:
             zipname = tail
-        if zipfile.is_zipfile(head+'.zip'):
+        zpath = os.path.join(basedir, head + '.zip')
+        if zipfile.is_zipfile(zpath):
             from cStringIO import StringIO
-            zfile = zipfile.ZipFile(head+'.zip')
+            zfile = zipfile.ZipFile(zpath)
             try:
                 fo = StringIO()
                 fo.write(zfile.read(os.path.join(
@@ -197,20 +214,14 @@
                         os.sep, '/')))
                 fo.seek(0)
                 if pathinfo:
-                    return fo, name
+                    return (fo, name)
                 return fo
             except Exception:
-                name2 = os.path.normpath(os.path.join(head + '.zip', zipname))
                 pass
-    for i in (name2, name):
-        if i and os.path.isfile(i):
-            fo = file(i, mode)
-            if pathinfo:
-                return fo, i
-            return fo
-    if os.path.splitext(name)[1] == '.rml':
-        raise IOError, 'Report %s doesn\'t exist or deleted : ' %str(name)
-    raise IOError, 'File not found : %s' % name
+    # Not found
+    if name.endswith('.rml'):
+        raise IOError('Report %r doesn\'t exist or deleted' % name)
+    raise IOError('File not found: %s' % name)
 
 
 #----------------------------------------------------------