jinja2: rendered -> output, template -> url/inline

slapos/recipe/template/README.jinja2.txt

@@ -5,15 +5,17 @@
 Similar to the default recipe but the template syntax is Jinja2 instead of
 buildout. Other significant differences are:
 
-- For historical reasons, the generated file is specified with ``rendered``
-  instead of ``output``.
-- For historical reasons, the template is specified with ``template`` only
-  and an inline template is prefixed with ``inline:`` + an optional newline.
 - Rendering, and download if requested, is done during the install phase.
 - Dependencies are explicit (see ``context`` option) instead of deduced from
   the template.
 - Some extra features (options detailed below).
 
+For backward compatibility, the following old options are still supported:
+
+- The generated file can be specified with ``rendered`` instead of ``output``.
+- The template can be specified with ``template`` instead of ``url``/``inline``.
+  An inline template is prefixed with ``inline:`` + an optional newline.
+
 Example demonstrating some types::
 
     >>> write('buildout.cfg',

slapos/recipe/template/__init__.py

@@ -71,24 +71,31 @@ class Recipe(object):
         self.mode = int(mode, 8) if mode else None
         self._init(name, options)
 
-    def _init(self, name, options):
-        self.output = options['output']
-        rendered = options.get('inline')
+    def _template(self, options):
+        inline = options.get('inline')
         try:
             url = options['url']
         except KeyError:
-            if rendered is None:
+            if inline is None:
                 raise
             if self.md5sum:
                 raise UserError("options 'inline' & 'md5sum' conflict")
             self.md5sum = True # tell update() to do nothing
-            self.rendered = rendered
+            return True, inline
         else:
-            if rendered:
+            if inline:
                 raise UserError("options 'inline' & 'url' conflict")
+            return False, url
+
+    def _init(self, name, options):
+        self.output = options['output']
+        inline, template = self._template(options)
+        if inline:
+            self.rendered = template
+        else:
             options_sub = options._sub
             self.rendered = '$'.join(options_sub(s, None)
-                for s in self._read(url).split('$$'))
+                for s in self._read(template).split('$$'))
 
     def _read(self, url, *args):
         path, is_temp = zc.buildout.download.Download(

slapos/recipe/template/jinja2_template.py

@@ -165,8 +165,17 @@ class Recipe(Recipe):
                 delimiter=import_delimiter)
         else:
             loader = None
-        self.output = options['rendered']
-        self.template = options['template']
+        try:
+            self.output = options['output']
+        except KeyError: # BBB
+            self.output = options['rendered']
+            template = options['template']
+            if template.startswith('inline:'):
+                self.template = True, template[7:].lstrip('\r\n')
+            else:
+                self.template = False, template
+        else:
+            self.template = self._template(options)
         extension_list = [x for x in (y.strip()
             for y in options.get('extensions', '').split()) if x]
         self.context = context = DEFAULT_CONTEXT.copy()
@@ -187,11 +196,10 @@ class Recipe(Recipe):
             loader=loader)
 
     def _render(self):
-        template = self.template
         env = self.env
-        if template.startswith('inline:'):
-            source = template[7:].lstrip('\r\n')
-            compiled_source = env.compile(source, filename='<inline>')
+        inline, template = self.template
+        if inline:
+            compiled_source = env.compile(template, filename='<inline>')
         else:
             try:
                 compiled_source = compiled_source_cache[template]

[Vincent Pelletier]

Because of this try..except..else pattern, it is not possible to override a section which specifies output to instead replace it with an template=inline:..., breaking backward compatibility. Please fix.

[Julien Muchembled]

I don't understand. Can you clarify or give an real example ?

Also note that I want to encourage people to use the new options and there's just enough backward compatibility to make a transition possible.

[Vincent Pelletier]

Initial (non-working) situation after updating foo.cfg from a version before the template recipe key scheme change to a version after it:

foo.cfg:

[foo]
output = hey
url = ...

bar.cfg:

[buildout]
extends = foo.cfg

[foo]
template = inline:blablah

Obviously, this will ignore template because output is present. Fine, I can live with such backward compatibility breakage.

Here is how I expect a minimal fix to look like:

bar.cfg:

[buildout]
extends = foo.cfg

[foo]
url =
template = inline:blablah

But of course it does not work because output is still fully set and template/url are not even checked in such case. So here is the next level fix, which seems to be the kind of direction the above code could work with:

bar.cfg:

[buildout]
extends = foo.cfg

[foo]
output =
rendered = hey # duplicating the "output" value from the original [foo] section
template = inline:blablah

But of course KeyError does not happen on options['output'] access, so template is still ignored.

So this backward compatibility code can only be triggered if no layer at all switched to the new parameter scheme.

Then if I try to go one step further in following the renaming (and here we leave the strict perimeter of my original comment on the present commit) and define the new property to provide the inline template:

bar.cfg:

[buildout]
extends = foo.cfg

[foo]
url =
inline = blablah

This does not work because the exact same try..except pattern is used in fc8dcbb8, and the recipe complains that url and template are mutually exclusive. So because of both these simultaneous bugs (on the present commit and on fc8dcbb8), there is no way to specify an inline template when overriding a section which includes a template by url.

So I have to convert what used to be a trivial inline template section into a separate template file included by URL, to pass it parameters and to manage its md5sum (...which could mean setting commit hooks and update-hash, but these details are beside my point: the section becomes more complex).

So the final situation to get stuff done despite these obstacles is:

blah.jinja2:

blahblah

bar.cfg:

[buildout]
extends = foo.cfg

[foo]
url = ${:_profile_base_location_}/blah.jinja2
md5sum = ...
context +=
  # ...some stuff needed in the template, which used to be expanded here when it was in-line but now has to be explicitly added

Fixing the fc8dcbb8 issue is enough to provide a clean way out of this issue in practice as it would fix support for an empty url and a specified inline.

But I think the above backward-compatibility codepath is still broken of its own: I think there should be independent backward-compatibility codepaths for template/url and rendered/output, which would each properly handle these being set to an empty value. Failing when both of either pair is provided is fine I guess.

[Julien Muchembled]

A long post with apparently typos (e.g. "recipe complains that url and template are mutually exclusive" instead of "recipe complains that url and inline are mutually exclusive" ?) and it's still unclear. I expected you to just give snippets from 2 files and list your constraints, then I should be able to read the code and understand the issue without more explanation.

Just by reading the beginning, you say that foo.cfg is written for the new version of slapos.recipe.template and then you seem to allow yourself to modify bar.cfg, so what's wrong with the following ?

foo.cfg:

[foo]
output = hey
url = ...

bar.cfg:

[buildout]
extends = foo.cfg

[foo]
url =
inline = blablah

Of course, it currently fails because of the other issue you reported but it's trivial to fix and that's not an argument to implement a workaround.

[Vincent Pelletier]

Just by reading the beginning, you say that foo.cfg is written for the new version of slapos.recipe.template and then you seem to allow yourself to modify bar.cfg

foo.cfg is some file from slapos.git that I cannot modify.

bar.cfg is the project-specific file I fully control and which (as visible in the above post) extends foo.cfg and overrides part of it.

Something I did not explicitly mention, but assumed was obvious in the context of a backward-compatibility issues, is that this extension in bar.cfg is being changed from a foo.cfg version before the slapos.recipe.template API change to one after the API change. So, yes, I "allow myself" to modify it because I am precisely repinning its extends, the question is how do I get it working again after the repin.

What is especially infuriating is that:

• The bugs in this recipe are the only thing which needed any workaround on a 1.5 years repin jump of the whole complex ERP5 SR.
• The cause is the section API changed, which looks like a cosmetic change which ends up causing gratuitous breakage pushing extra work to SR maintainers.

so what's wrong with the following ?

I precisely wrote that example in my post, followed by why this does not work (now with the typo fixed):

This does not work because the exact same try..except pattern is used in fc8dcbb8, and the recipe complains that url and inline are mutually exclusive. So because of both these simultaneous bugs (on the present commit and on fc8dcbb8), there is no way to specify an inline template when overriding a section which includes a template by url.

Of course, it currently fails because of the other issue you reported

Which is precisely what I wrote in the last 2 lines of my post, including why I think the present bug also needs to be fixed, and one way I suggest it would be fixed.

but it's trivial to fix and that's not an argument to implement a workaround.

Then please explain what trivial fix you are suggesting which would work with the currently-released recipe.

[Julien Muchembled]

It's not cosmetic changes. It unifies the 2 recipes in a way that they follow some convention, e.g. 'url' for source like in slapos.recipe.build:download*. Overall, the API is easier to remember and there are greater chance to be able to refactor some parts, i.e. several sections with different recipes sharing common options.

And now that most of the code in the wild now uses the new API, I'd rather drop backward compatibility if I spent any time on this recipe.

Then please explain what trivial fix you are suggesting which would work with the currently-released recipe.

I don't understand. You asked me to fix slapos.recipe.template so you'd have to use the new release I would do to include the fixes.

[Vincent Pelletier]

And now that most of the code in the wild now uses the new API, I'd rather drop backward compatibility if I spent any time on this recipe.

Fine, I guess.

Please also check any try: options[...] except KeyError: which may remain.

I don't understand. You asked me to fix slapos.recipe.template so you'd have to use the new release I would do to include the fixes.

There are two very distinct issues:

• how to get stuff to work now (...last week, actually)
• how to get stuff to work better in the future

The discussion of how to work around these two issues belongs to the former. To me, the claim that a trivial fix exists (in the present tense) also belongs to the former: in this context I cannot care less that a trivial fix will eventually exist (future tense), because I need stuff to work now (last week) with whatever is already released. So I need (and implemented) a workaround, which increases the complexity on my end.

The discussion of how to improve the code to fix these issues belong to the latter. In this context, the fact that the workaround will become unnecessary (whenever the SR is next re-pinned) is good news.

There is also a third discussion about how to avoid breaking compatibility the next time a recipe is reworked. I guess try: options[...] except KeyError: belongs in a "don't do" list there. The idea of using a separate recipe name (within the same egg, so both can share most of a common code) should probably be in a "consider doing" list there, but this thread has been going on for way too long already.