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)
-
+