Merge lp:~jbaudoux/anybox.recipe.openerp/20130908_relative_paths into lp:anybox.recipe.openerp

[Jacques-Etienne Baudoux]

Improvement to be compliant with buildout option "relative_paths = true"

[lp:~jbaudoux/anybox.recipe.openerp/20130908_relative_paths]

423. By Jacques-Etienne Baudoux on 2013-09-08

Fix format string

[Georges Racinet (gracinet)]

Really interesting if I understand it correctly, this the forever delayed "TODO investigate these options" ! Thank you for the contribution.

I'd need to read the details of _relativitize(), and especially understand why the one from zc.buildout.easy_install is not enough, but one thing is for sure : this will need dedicated unit tests and documentation : if OpenERP buildouts can be made relocatable, then the doc must state it explicitely, along with limitations (simply pointing out that it behaves exactly as zc.recipe.egg in that respect, it that is the case).

Not yet decided whether this can go to 1.7 or trunk. There's nothing that seems to be so dangerous (a breakage would not stay unnoticed for long).

[Georges Racinet (gracinet)]

inclusion in trunk could be done faster, if that matters to you.
Also, with the new Sphinx documentation, that seems to be the most sensible choice for a new feature (even if we should have supported this from the beginning). Mention of relocability can be done in the non-specific parts of doc/configuration.rst, I guess.

[Jacques-Etienne Baudoux (jbaudoux)]

It allows me to deploy in a chroot subdirectory.
It's now compliant with the relative_paths option of buildout for the python scripts.
For your information, it's, however, still missing some hack to take care the the addons path in the openerp.cfg. I didn't found a nice way to solve that. For now, I'm solving it by creating a symling this: mybuildoutdirfullpath/chroot/mybuildoutdirfullpath -> /

[Georges Racinet (gracinet)]

OpenERP has a tendency to interpret paths relative to what if feels to be its home directory, namely the parent dir of the 'openerp' package.

In our case that's self.openerp_dir, so maybe you could work out something with os.path.relpath based on it ?

I don't know if that's true for addons, but it is for log files : bin/start_openerp --log-file=my.log will put the log file at <buildout_dir>/parts/<server part location>/my.log

One of the many reasons we wanted to normalize stuff in upgrade scripts (easy to forget to write down in maintainance procedures).

[Jacques-Etienne Baudoux (jbaudoux)]

As the openerp.cfg is generated by importing openerp.tools.config from buildout, and the path is generated by openerp with os.path.abspath(os.path.expanduser(os.path.expandvars(os.path.dirname(openerp.__file__))))
I don't see anyway to have relative path here.
And as having full path in openerp.cfg is not blocking, so I'm gonna leave it like this.

Nevertheless, for python, relative path is required for the python module loader if you deploy in chroot jail. So, I suggest you merge this MP.

[Georges Racinet (gracinet)]

> As the openerp.cfg is generated by importing openerp.tools.config from
> buildout, and the path is generated by openerp with os.path.abspath(os.path.ex
> panduser(os.path.expandvars(os.path.dirname(openerp.__file__))))
> I don't see anyway to have relative path here.

This I don't really understand: that'd be the initial generation, but the recipe overwrites openerp.cfg afterwards by writing directly to it with a ConfigParser instance, IIRC

> And as having full path in openerp.cfg is not blocking, so I'm gonna leave it
> like this.
>
> Nevertheless, for python, relative path is required for the python module
> loader if you deploy in chroot jail. So, I suggest you merge this MP.

I see, for the chroot use case, you *need* paths to be relative.
But for simple relocability without rerunning buildout, it'd be neat to have a full relative setup.
I'll take a look at the merge this afternoon. Meanwhile, if you have opportunity to add tests in anybox/recipe/openerp/test_server.py or test_base.py, that'd be great (tests are needed anyway).

[Georges Racinet (gracinet)]

Took a look at the merge today (sorry, I'm late) and a few more question arise:

 - the invocation of Session in the generated interpreter now uses the buildout dir (to read the expected VERSION file)
 - is there a specific reason not to pass a relative path in the scripts generation, too ?

That makes too much questionning for merging before 1.8.0, I'll take a look at these again rigth after the release of 1.8.0 (should be first thing tomorrow morning CEST).

[Georges Racinet (gracinet)]

But I'm confident that it can make it before 1.8.1

[Jacques-Etienne Baudoux (jbaudoux)]

> This I don't really understand: that'd be the initial generation, but the recipe overwrites openerp.cfg afterwards by writing directly to it with a ConfigParser instance, IIRC
ok, I see what you mean. This has to be investigated a bit more.

> - is there a specific reason not to pass a relative path in the scripts generation, too ?
Of what scripts are you talking? All the python scripts in bin seems to be with relative path

Anyway, thanks for the time you spend reviewing this. I need to look at your latest developments

[Jacques-Etienne Baudoux (jbaudoux)]

Hi Georges,

I'm coming back on this MP as I just found some time to investigate that openerp.cfg addons_path that is not relative to the root of the chroot jail.

So, it's coming from server.py:
|- def _create_default_config(self):
|| """Have OpenERP generate its default config file.
|| """
|| self.options.setdefault('options.admin_passwd', '')
|| if self.major_version <= (6, 0):
|| # root-path not available as command-line option
|| os.chdir(join(self.openerp_dir, 'bin'))
|| subprocess.check_call([self.script_path, '--stop-after-init', '-s',
|| ])
|| else:
|| sys.path.extend([self.openerp_dir])
|| sys.path.extend([egg.location for egg in self.ws])
|| from openerp.tools.config import configmanager
|| configmanager(self.config_path).save()

I see 2 options:

1/ no improvement to this MP. Only python scripts are relative. However, each item in the addons path is prefixed by the path of the jail.
addons_path = /home/jbaudoux/chroot/opt/server/openerp/addons,....
and, inside the jail, workaround by creating a symlink /home/jbaudoux/chroot -> /

2/ don't use
|| from openerp.tools.config import configmanager
|| configmanager(self.config_path).save()
but run the jail start script like it was previously
|| subprocess.check_call([self.script_path, '--stop-after-init', '-s',
|| ])
where self.script_path has to be replaced by a new buildout variable specifying the script to run
This also has the advantage that if buildout is not run by the user that will run openerp, the openerp.cfg file gets the right ownership which is currently not the case.

What do you prefer? Option 1 is working fine (so I keep this MP as it is) but if you'd like to have option 2 implemented, I can make a MP to this MP.

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Jacques Etienne,

thank you for your work! I'm suggesting some changes to streamline the preparation of running the buildout in a jail root here: https://code.launchpad.net/~therp-nl/anybox.recipe.openerp/jbaudoux-relative_paths_explicit_jailroot_support/+merge/205099. This includes mangling of the addons paths.

[Jacques-Etienne Baudoux (jbaudoux)]

This MP is now obsolete and replaced by:
https://code.launchpad.net/~jbaudoux/anybox.recipe.openerp/1.8.5_relative_paths_jailroot/+merge/237238

Keeping this MP for history until latest gets merged.

Unmerged revisions

423. By Jacques-Etienne Baudoux on 2013-09-08

Fix format string

422. By Jacques-Etienne Baudoux on 2013-09-08

pep8

421. By Jacques-Etienne Baudoux on 2013-09-08

Support buildout option "relative_paths = true"

=== modified file 'anybox/recipe/openerp/base.py'
--- anybox/recipe/openerp/base.py	2013-09-05 22:45:15 +0000
+++ anybox/recipe/openerp/base.py	2013-09-08 17:19:36 +0000
@@ -111,6 +111,15 @@
         self.clear_retry = clear_retry == 'true'
 
         # same as in zc.recipe.eggs
+        relative_paths = options.get(
+            'relative-paths',
+            buildout['buildout'].get('relative-paths', 'false')
+            )
+        if relative_paths == 'true':
+            self._relative_paths = self.buildout_dir
+        else:
+            self._relative_paths = ''
+            assert relative_paths == 'false'
         self.extra_paths = [
             join(self.buildout_dir, p.strip())
             for p in self.options.get('extra-paths', '').split(os.linesep)
 
=== modified file 'anybox/recipe/openerp/server.py'
--- anybox/recipe/openerp/server.py	2013-09-07 11:43:18 +0000
+++ anybox/recipe/openerp/server.py	2013-09-08 17:19:36 +0000
@@ -13,6 +13,21 @@
 SERVER_COMMA_LIST_OPTIONS = ('log_handler', )
 
 
+def _relative_path(common, path):
+    """Copied from easy_install"""
+    r = []
+    while 1:
+        dirname, basename = os.path.split(path)
+        r.append(basename)
+        if dirname == common:
+            break
+        if dirname == path:
+            raise AssertionError("dirname of %s is the same" % dirname)
+        path = dirname
+    r.reverse()
+    return os.path.join(*r)
+
+
 class ServerRecipe(BaseRecipe):
     """Recipe for server install and config
     """
@@ -266,15 +281,28 @@
             desc = self.openerp_scripts[name] = dict(entry=entry)
             return name, desc
 
+    def _relativitize(self, path):
+        """Inspired from easy_install"""
+        if self._relative_paths:
+            eggs = os.path.normcase(os.path.abspath(
+                self.b_options['eggs-directory']))
+            common = os.path.dirname(os.path.commonprefix([path, eggs]))
+            if (common == self._relative_paths or
+                common.startswith(os.path.join(self._relative_paths, ''))
+                ):
+                return "join(base, %r)" % _relative_path(common, path)
+        return path
+
     def _register_main_startup_script(self, qualified_name):
         """Register main startup script, usually ``start_openerp`` for install.
         """
         desc = self._get_or_create_script('openerp_starter',
                                           name=qualified_name)[1]
         desc.update(
-            arguments='%r, %r, version=%r' % (self._get_server_command(),
-                                              self.config_path,
-                                              self.major_version),
+            arguments='%s, %s, version=%r' % (
+                self._relativitize(self._get_server_command()),
+                self._relativitize(self.config_path),
+                self.major_version),
         )
 
         startup_delay = float(self.options.get('startup_delay', 0))
@@ -305,9 +333,9 @@
                 "from anybox.recipe.openerp import devtools",
                 "devtools.load(for_tests=True)",
                 "")),
-            arguments='%r, %r, version=%r, just_test=True' % (
-                self._get_server_command(),
-                self.config_path,
+            arguments='%s, %s, version=%r, just_test=True' % (
+                self._relativitize(self._get_server_command()),
+                self._relativitize(self.config_path),
                 self.major_version),
         )
 
@@ -340,7 +368,8 @@
         # to resort on hacking sys.argv
         desc['initialization'] = (
             "from sys import argv; argv[1:] = ['%s', '-c', '%s.conf.py']" % (
-                gunicorn_entry_point, join(self.etc, qualified_name)))
+                gunicorn_entry_point,
+                self._relativitize(join(self.etc, qualified_name))))
 
     def _register_openerp_command(self, qualified_name):
         """Register https://launchpad.net/openerp-command for install.
@@ -403,7 +432,9 @@
         desc = self._get_or_create_script('openerp_cron_worker',
                                           name=qualified_name)[1]
         desc.update(entry='openerp_cron_worker',
-                    arguments='%r, %r' % (script_src, self.config_path),
+                    arguments='%s, %s' % (
+                        self._relativitize(script_src),
+                        self._relativitize(self.config_path)),
                     initialization='',
                     )
 
@@ -418,7 +449,7 @@
         initialization = os.linesep.join((
             "",
             "from anybox.recipe.openerp.startup import Session",
-            "session = Session(%r)" % self.config_path,
+            "session = Session(%s)" % self._relativitize(self.config_path),
             "if len(sys.argv) <= 1:",
             "    print('To start the OpenERP working session, just do:')",
             "    print('    session.open(db=DATABASE_NAME)')",
@@ -441,8 +472,7 @@
             initialization=initialization,
             arguments=self.options.get('arguments', ''),
             extra_paths=self.extra_paths,
-            # TODO investigate these options:
-            # relative_paths=self._relative_paths,
+            relative_paths=self._relative_paths,
         )
 
     def _install_openerp_scripts(self):
@@ -476,9 +506,8 @@
                 interpreter='',
                 initialization=initialization,
                 arguments=desc.get('arguments', ''),
-                # TODO investigate these options:
                 extra_paths=self.extra_paths,
-                # relative_paths=self._relative_paths,
+                relative_paths=self._relative_paths,
             )
             self.openerp_installed.append(join(self.bin_dir, script_name))