Do not reprocess already extended files

extends can be interpreted as inheritance in OOP, but the original
behaviour was against what is commonly (always?) seen everywhere.

It is however good practice a file extends all files it needs directly
(and only them). Then if two files A & B (possibly unrelated) extends
the same third C, A was unable to overrides C values. It was even
error-prone because someone who don't use B yet could override C values
in A and later extending B would break A.

For some of our common use cases, this new algorithm is also 9x faster
(time to annotate: ~2.3s with -> ~.29s).

Other changes:
- ~/ is now expanded for non-url extends.
- An absolute (non-url) path is not longer treated like a local path
  if the base is a url.
- Better path/url normalization.

Rebase instructions:
- squash with "Chomp ../ from beginging of filenames"
- split and apply "Support ${:_profile_base_location_}." after

src/zc/buildout/buildout.py

@@ -32,6 +32,11 @@ try:
 except ImportError:
     from io import StringIO
 
+try:
+    from urllib.parse import urljoin
+except ImportError:
+    from urlparse import urljoin
+
 import zc.buildout.configparser
 import copy
 import datetime
@@ -286,7 +291,7 @@ class Buildout(DictMixin):
             for (section, v) in itertools.groupby(sorted(cloptions),
                                                   lambda v: v[0])
             )
-        override = cloptions.get('buildout', {}).copy()
+        override = cloptions.get('buildout', {})
 
         # load user defaults, which override defaults
         if user_defaults:
@@ -298,13 +303,12 @@ class Buildout(DictMixin):
             user_config = os.path.join(buildout_home, 'default.cfg')
             if os.path.exists(user_config):
                 _update(data, _open(os.path.dirname(user_config), user_config,
-                                    [], data['buildout'].copy(), override,
-                                    set()))
+                                    data['buildout'], override))
 
         # load configuration files
         if config_file:
-            _update(data, _open(os.path.dirname(config_file), config_file, [],
-                                data['buildout'].copy(), override, set()))
+            _update(data, _open(os.path.dirname(config_file), config_file,
+                                data['buildout'], override))
 
         # apply command-line options
         _update(data, cloptions)
@@ -1813,63 +1817,55 @@ def _default_globals():
 
     return globals_defs
 
-def _open(base, filename, seen, dl_options, override, downloaded):
+def _open(base, filename, dl_options, override, seen=None, processing=None):
     """Open a configuration file and return the result as a dictionary,
 
     Recursively open other files based on buildout options found.
     """
-    counter = 0
-    while filename.startswith('../'):
-      filename = filename.replace('../', '', 1)
-      counter += 1
-    base = base.rsplit('/', counter)[0]
-    _update_section(dl_options, override)
-    _dl_options = _unannotate_section(dl_options.copy())
-    newest = bool_option(_dl_options, 'newest', 'false')
-    fallback = newest and not (filename in downloaded)
-    download = zc.buildout.download.Download(
-        _dl_options, cache=_dl_options.get('extends-cache'),
-        fallback=fallback, hash_name=True)
-    is_temp = False
-    downloaded_filename = None
     if _isurl(filename):
-        downloaded_filename, is_temp = download(filename)
-        fp = open(downloaded_filename)
-        base = filename[:filename.rfind('/')]
-    elif _isurl(base):
-        if os.path.isabs(filename):
-            fp = open(filename)
-            base = os.path.dirname(filename)
+        download = True
+    else:
+        download = _isurl(base)
+        if download:
+            filename = urljoin(base + '/', filename)
         else:
-            filename = base + '/' + filename
-            downloaded_filename, is_temp = download(filename)
-            fp = open(downloaded_filename)
-            base = filename[:filename.rfind('/')]
+            filename = os.path.realpath(os.path.join(
+                base, os.path.expanduser(filename)))
+
+    if seen:
+        if filename in seen:
+            if filename in processing:
+                raise zc.buildout.UserError("circular extends: %s" % filename)
+            return {}
+        seen.add(filename)
     else:
-        filename = os.path.join(base, filename)
-        fp = open(filename)
-        base = os.path.dirname(filename)
-    downloaded.add(filename)
+        seen = {filename}
+
+    is_temp = False
+    try:
+        if download:
+            if override is None:
+                _dl_options = dl_options
+            else:
+                _dl_options = _update_section(dl_options.copy(), override)
+                _unannotate_section(_dl_options)
+            downloaded_filename, is_temp = zc.buildout.download.Download(
+                _dl_options, cache=_dl_options.get('extends-cache'),
+                fallback=bool_option(_dl_options, 'newest', 'false'),
+                hash_name=True)(filename)
+            filename_for_logging = '%s (downloaded as %s)' % (
+                filename, downloaded_filename)
+            base = filename[:filename.rfind('/')]
+        else:
+            downloaded_filename = filename_for_logging = filename
+            base = os.path.dirname(filename)
 
-    if filename in seen:
+        with open(downloaded_filename) as fp:
+            result = zc.buildout.configparser.parse(
+                fp, filename_for_logging, _default_globals)
+    finally:
         if is_temp:
-            fp.close()
             os.remove(downloaded_filename)
-        raise zc.buildout.UserError("Recursive file include", seen, filename)
-
-    root_config_file = not seen
-    seen.append(filename)
-
-    filename_for_logging = filename
-    if downloaded_filename:
-        filename_for_logging = '%s (downloaded as %s)' % (
-            filename, downloaded_filename)
-    result = zc.buildout.configparser.parse(
-        fp, filename_for_logging, _default_globals)
-
-    fp.close()
-    if is_temp:
-        os.remove(downloaded_filename)
 
     options = result.get('buildout', {})
     extends = options.pop('extends', None)
@@ -1885,21 +1881,22 @@ def _open(base, filename, seen, dl_options, override, downloaded):
                 section['_profile_base_location_'] = base
                 break
 
-    result = _annotate(result, filename)
-
-    if root_config_file and 'buildout' in result:
-        dl_options = _update_section(dl_options, result['buildout'])
-
+    _annotate(result, filename)
     if extends:
-        extends = extends.split()
-        eresult = _open(base, extends.pop(0), seen, dl_options, override,
-                        downloaded)
-        for fname in extends:
-            _update(eresult, _open(base, fname, seen, dl_options, override,
-                    downloaded))
-        result = _update(eresult, result)
-
-    seen.pop()
+        if processing:
+            processing.append(filename)
+        else:
+            processing = [filename]
+            # From now on, it won't change.
+            dl_options = _unannotate_section(_update_section(_update_section(
+                dl_options.copy(), options), override))
+            override = None
+        eresult = {}
+        for extends in extends.split():
+            _update(eresult, _open(base, extends, dl_options, override,
+                                   seen, processing))
+        del processing[-1]
+        return _update(eresult, result)
     return result
 
 

src/zc/buildout/extends-cache.txt

@@ -499,7 +499,6 @@ The URL http://localhost/baseA.cfg was downloaded.
 The URL http://localhost/base.cfg was downloaded.
 The URL http://localhost/baseB.cfg was downloaded.
 The URL http://localhost/baseC.cfg was downloaded.
-The URL http://localhost/baseB.cfg was downloaded.
 The URL http://localhost/deeper/base.cfg was downloaded.
 The URL http://localhost/baseD.cfg was downloaded.
 Not upgrading because not running a local buildout command.

[Julien Muchembled]

For slapos!967 (merged), @lpgeneau could not test that wendelin-scalability/test-fluentd-* SR still build because these SR are already broken for a while, and the bug seems to be in this commit. What really looks wrong is that while both software/fluentd/software.cfg and software/wendelin-scalability/test-common.cfg compute versions:zc.buildout=2.7.1+slapos010 when annotated separately, extending these 2 files in this order (software/wendelin-scalability/test-fluentd-common.cfg) results in 2.3.1.

/cc @jerome

[Jérome Perrin]

@Nicolas and I also discussed "something surprising" depending on the order of extends. I don't remember the details, I think we did not investigate deeply, but this might be the same thing.

[Nicolas Wavrant]

What we discussed is that in stack/erp5 we define versions of eggs like :

https://lab.nexedi.com/nexedi/slapos/blob/master/stack/erp5/ztk-versions.cfg#L35

but some of them are duplicated in stack/slapos.cfg :

https://lab.nexedi.com/nexedi/slapos/blob/master/stack/slapos.cfg#L354

I forgot the detail, but the surprise was the used version depends on the order of which these 2 files are extended, and sometimes the older version is picked.

In my case, I wanted to build a SR based on ERP5, but it failed because it was using setuptools = 12.2 (https://lab.nexedi.com/nexedi/slapos/blob/master/stack/erp5/ztk-versions.cfg#L87)