Code review comment for lp:~gary/z3c.recipe.filetemplate/support-system-python

On Jul 13, 2009, at 5:21 PM, Francis J. Lacoste wrote:

> On July 11, 2009, Gary Poster wrote:
>> Gary Poster has proposed merging
>> lp:~gary/z3c.recipe.filetemplate/support-system-python into
>> lp:~gary/z3c.recipe.filetemplate/trunk.
>>
>> Requested reviews:
>> Francis J. Lacoste (flacoste)
>>
>> Support the new zc.buildout 1.4.0+ include-site-packages option,
>> and more.
>>
>
> Hi Gary,
>
> This all looks good. My only concern is the new write_and_wait which
> I don't
> understand why it's necessary.

Cool. I'll discuss below.

>
>> === modified file 'z3c/recipe/filetemplate/__init__.py'
>
>> @@ -195,17 +202,19 @@
>> 'Destinations already exist: %s. Please make sure
>> that '
>> 'you really want to generate these automatically.
>> Then '
>> 'move them away.', ', '.join(already_exists))
>> + seen = [] # we throw this away right now, but could move
>> template
>
> Sentence should start with a capital.

Ack. See incremental diff.

>
>> + # processing up to __init__ if valuable. That would mean
>> that
> templates
>> + # would be rewritten even if a value in another section
>> had been
>> + # referenced; however, it would also mean that __init__
>> would do
>> + # virtually all of the work, with install only doing the
>> writing.
>> for rel_path, last_mod, st_mode in self.actions:
>> source = os.path.join(self.source_dir, rel_path)
>> dest = os.path.join(self.destination_dir, rel_path[:-3])
>> mode=stat.S_IMODE(st_mode)
>> - template=open(source).read()
>> - template=re.sub(r"\$\{([^:]+?)\}", r"${%s:\1}" %
>> self.name,
>> - template)
>> - self._create_paths(os.path.dirname(dest))
>> # we process the file first so that it won't be created
>> if
> there
>> # is a problem.
>> - processed = self.options._sub(template, [])
>> + processed = Template(source).substitute(self, seen)
>> + self._create_paths(os.path.dirname(dest))
>> result=open(dest, "wt")
>> result.write(processed)
>> result.close()
>> @@ -221,3 +230,74 @@
>>
>> def update(self):
>> pass
>> +
>> +
>> +class Template:
>> + # hacked from string.Template
>> + pattern = re.compile(r"""
>> + \$(?:
>> + \${(?P<escaped>[^}]*)} | # Escape sequence
>> of two
> delimiters.
>> + {(?P<braced_single>[-a-z0-9 ._]+)} |
>> + # Delimiter and a braced
>> local
> option
>> + {(?P<braced_double>[-a-z0-9 ._]+:[-a-z0-9 ._]+)} |
>> + # Delimiter and a braced
>> fully
>> + # qualified option (that
>> is, with
>> + # explicit section).
>> + {(?P<invalid>[^}]*}) # Other ill-formed
>> delimiter
> exprs.
>> + )
>> + """, re.IGNORECASE | re.VERBOSE)
>> +
>> + def __init__(self, source):
>> + self.source = source
>> + self.template = open(source).read()
>> +
>> + def _get_colno_lineno(self, i):
>> + lines = self.template[:i].splitlines(True)
>> + if not lines:
>> + colno = 1
>> + lineno = 1
>> + else:
>> + colno = i - len(''.join(lines[:-1]))
>> + lineno = len(lines)
>> + return colno, lineno
>> +
>> + def _get(self, options, section, option, seen, start):
>> + value = options.get(option, None, seen)
>> + if value is None:
>> + colno, lineno = self._get_colno_lineno(start)
>> + raise zc.buildout.buildout.MissingOption(
>> + "Option '%s:%s', referenced in line %d, col %d of
>> %s, "
>> + "does not exist." %
>> + (section, option, lineno, colno, self.source))
>> + return value
>> +
>> + def substitute(self, recipe, seen):
>> + # Helper function for .sub()
>> + def convert(mo):
>> + # Check the most common path first.
>> + option = mo.group('braced_single')
>> + if option is not None:
>> + val = self._get(recipe.options, recipe.name,
>> option, seen,
>> + mo.start('braced_single'))
>> + # We use this idiom instead of str() because the
>> latter
> will
>> + # fail if val is a Unicode containing non-ASCII
>> characters.
>> + return '%s' % (val,)
>> + double = mo.group('braced_double')
>> + if double is not None:
>> + section, option = double.split(':')
>> + val = self._get(recipe.buildout[section], section,
>> option,
> seen,
>> + mo.start('braced_double'))
>> + return '%s' % (val,)
>> + escaped = mo.group('escaped')
>> + if escaped is not None:
>> + return '${%s}' % (escaped,)
>> + invalid = mo.group('invalid')
>> + if invalid is not None:
>> + colno, lineno =
>> self._get_colno_lineno(mo.start('invalid'))
>> + raise ValueError(
>> + 'Invalid placeholder %r in line %d, col %d of
>> %s' %
>> + (mo.group('invalid'), lineno, colno,
>> self.source))
>> + raise ValueError('Unrecognized named group in pattern',
>> + self.pattern) # programmer error,
>> AFAICT
>> + return self.pattern.sub(convert, self.template)
>> +
>
> Trailing spaces.

Ack, done.

>
>
>> === modified file 'z3c/recipe/filetemplate/tests.py'
>> --- z3c/recipe/filetemplate/tests.py 2009-04-30 17:56:10 +0000
>> +++ z3c/recipe/filetemplate/tests.py 2009-07-09 18:20:56 +0000
>> @@ -12,12 +12,24 @@
>> #
>>
> ##############################################################################
>>
>> +import os
>> import zc.buildout.testing
>> import zc.buildout.tests
>> from zope.testing import doctest
>>
>> +def write_and_wait(dir, *args):
>> + path = os.path.join(dir, *(args[:-1]))
>> + original = os.stat(path).st_mtime
>> + while os.stat(path).st_mtime == original:
>> + f = open(path, 'w')
>> + f.write(args[-1])
>> + f.flush()
>> + os.fsync(f.fileno())
>> + f.close()
>> +
>
> Why is this needed? Your CHANGES.txt file mentioned making tests
> more robust
> against timing issues, but I don't see how that makes thing more
> robust?
>
> I mean the mtime of path should be modified right after open(path,
> 'w'). I
> don't see the rationale behind the loop, nor the necessity of
> f.flush() nor
> fsysnc. Unless you are running things on a NFS-mounted volume... and
> even
> there, I'm not sure it would help.

In general, the necessity is caused by the mtime precision. On my
Mac, the precision is 1 second. Here's an example run of a slightly
modified version of the helper (adding a print), in the interactive
prompt.

 >>> def write_and_wait(dir, *args):
... path = os.path.join(dir, *(args[:-1]))
... original = os.stat(path).st_mtime
... while os.stat(path).st_mtime == original:
... print original
... f = open(path, 'w')
... f.write(args[-1])
... f.flush()
... os.fsync(f.fileno())
... f.close()
...
 >>> f = open('/Users/gary/dev/deleteme', 'w')
 >>> f.write('test')
 >>> f.close()
 >>> write_and_wait('/Users/gary/dev/deleteme', 'test')
1247536625.0
 >>> write_and_wait('/Users/gary/dev/deleteme', 'test')
1247536627.0
 >>> write_and_wait('/Users/gary/dev/deleteme', 'test')
1247536628.0
 >>> write_and_wait('/Users/gary/dev/deleteme', 'test')
1247536629.0
1247536629.0
1247536629.0
[...there are 299 of these...]
 >>>

So, the precision is the point. I tried a variety of things to
trigger the mtime, but writing an empty string didn't work, and
directly writing the mtime is OS-specific/dependent, and I didn't
really want to go there. This is a test harness. Good enough, was my
thinking.

As far as all the flushing and so on, that's what Jim had for his
``write`` helper function, so I suspect there's a point there too. It
could probably be somewhat refactored but...it's a test harness.

Happy to change if you want, though.

Incremental diff follows.

Thanks

Gary

Index: z3c/recipe/filetemplate/__init__.py
===================================================================
--- z3c/recipe/filetemplate/__init__.py (revision 101788)
+++ z3c/recipe/filetemplate/__init__.py (working copy)
@@ -202,7 +202,7 @@
                  'Destinations already exist: %s. Please make sure
that '
                  'you really want to generate these automatically.
Then '
                  'move them away.', ', '.join(already_exists))
- seen = [] # we throw this away right now, but could move
template
+ seen = [] # We throw this away right now, but could move
template
          # processing up to __init__ if valuable. That would mean
that templates
          # would be rewritten even if a value in another section had
been
          # referenced; however, it would also mean that __init__
would do
@@ -300,4 +300,4 @@
              raise ValueError('Unrecognized named group in pattern',
                               self.pattern) # programmer error, AFAICT
          return self.pattern.sub(convert, self.template)
-
+