Issue 5336 - Allow additional include, page, and template paths using CMS

cms/sources.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 285 matching lines @@
                 path = os.path.join(dir, filename)
                 if os.path.isfile(path):
                     result.append(relpath + filename)
                 elif os.path.isdir(path):
                     do_list(path, relpath + filename + '/')
         do_list(self.get_path(subdir), '')
         return result
 
     def get_cache_dir(self):
         return os.path.join(self._dir, 'cache')
+
+
+class MultiSource(Source):
+    """A source that combines the contents of multiple other sources."""
+
+    def __init__(self, base_sources):
+        self._bases = base_sources
+
+    @property
+    def version(self):
+        return self._bases[0].version
+
+    def get_cache_dir(self):
+        return self._bases[0].get_cache_dir()
+
+    def __enter__(self):
+        for base in self._bases:
+            base.__enter__()
+        return self
+
+    def __exit__(self, exc_type, exc_value, tb):
+        return any(base.__exit__(exc_type, exc_value, tb)
+                   for base in self._bases)
+
+    def close(self):
+        for base in self._bases:
+            base.close()
+
+    def has_file(self, filename):
+        return any(base.has_file(filename) for base in self._bases)
+
+    def read_file(self, filename, binary=False):
+        for base in self._bases:
+            if base.has_file(filename):
+                return base.read_file(filename, binary)
+        raise KeyError('File not found {}'.format(filename))
+
+    def list_files(self, subdir):
+        return {f for base in self._bases for f in base.list_files(subdir)}
+
+
+def _memoize(func):
+    """Cache results of functions calls."""
+    memoized = {}
+
+    def wrapper(*args):
+        try:
+            return memoized[args]
+        except KeyError:
+            return memoized.setdefault(args, func(*args))
+    wrapper.cache_clear = memoized.clear
+    return wrapper
+
+
+def create_source(path, static=False, revision='default'):

[Wladimir Palant, 2017/09/29 08:50:44]

The default revision should be 'master', not 'default'.

[Vasily Kuznetsov, 2017/09/29 10:49:09]

Changed to `None`, following your next comment. Done

+    """Create a source from path and optional revision.
+
+    `static` flag indicates that the website source under `path` can be assumed
+    to not change after the source was created and any changes can be safely
+    ignored.
+    - If `static` is `True`, no interim changes are expected and caching will
+      be used to optimize performance (source directory is also assumed to be a
+      Mercurial repository, and `revision` parameter can be used to select a
+      source revision from which the files will be read).
+    - If `static` is `False`, changes on the filesystem are expected and will
+      be reflected in the outputs of the calls. This mode can be used for live
+      preview.
+
+    If `settings.ini` in the source contains `[paths]` section with an
+    `additional-paths` key that contains the list of additional root folders,
+    `MultiSource` will be instantiated and its bases will be the original
+    source plus an additional source for each additional root folder.
+    `MultiSource` looks up files in its base sources in the order they are
+    provided, so the files in the additional folders will only be used if the
+    original source doesn't contain that file.
+    """
+    if static:

[Wladimir Palant, 2017/09/29 08:50:44]

Should this flag really determine which type of source we choose? I'd expect
this to be dependent on the `revision` parameter - the default of this parameter
should be None, indicating a FileSource. And if some value is passed in, we
should use MercurialSource.

[Vasily Kuznetsov, 2017/09/29 10:49:10]

Yeah, you're right. `static` parameter should control the caching and `revision`
parameter (or its absence) should select the source type.

Done.

+        source = MercurialSource(path, revision)
+    else:
+        source = FileSource(path)
+
+    config = source.read_config()
+    try:
+        ap = config.get('paths', 'additional-paths').strip()
+        additional_paths = filter(None, ap.split())
+    except ConfigParser.NoSectionError:

[Wladimir Palant, 2017/09/29 08:50:44]

What about NoOptionError?

[Vasily Kuznetsov, 2017/09/29 10:49:09]

Thank you. Done.

+        additional_paths = []
+
+    if additional_paths:
+        additional_sources = [
+            create_source(os.path.join(path, p))

[Sebastian Noack, 2017/09/28 20:48:21]

This seems to be equivalent to FileSource(os.path.join(path, p)), hence the
recursion would be unnecessary and just makes the code harder to follow.

Also I wonder whether this would even work in practice. So on the server where
MercurialSource is used since the repository isn't checked out these paths
wouldn't exist anyway, or do I miss something?

[Wladimir Palant, 2017/09/29 08:50:44]

Not quite equivalent. The directory we link to might have additional paths of
its own, so we might get a MultiSource instance here again. Whether such nested
dependencies make a lot of sense is questionable, but we should support them for
correctness.
It would work in practice, but in a rather suboptimal way. On the server we
would use MercurialSource for the base source but FileSource for its
dependencies. So the server would have to update the dependencies to the right
revision, exactly the situation that MercurialSource is meant to avoid.

Something else is a bigger concern IMHO. The source repository is no longer
self-contained with this change, so whenever you want to test you would have to
check out its dependencies (what revisions?) to the same location as what the
server uses. Testing will require a rather messy setup now. We've been there
with our source code, and it isn't really manageable. That's why we created
ensure_dependencies.py.

While reusing ensure_dependencies.py here might be complicated, we might want to
use a simplified version of the logic. Meaning not additional_paths but rather
inherit_from setting indicating repository location and revision. These
repositories should be automatically cloned (with --noupdate flag) as
subdirectories within a dependencies/ directory or pulled as necessary, we can
use MercurialSource for them then - both for server and test environment.

[Vasily Kuznetsov, 2017/09/29 10:49:09]

Yes, nested dependencies are the reason why I made `create_source` recursive.
This was one of the options in the original discussion in the ticket (see
https://issues.adblockplus.org/ticket/5336#comment:2). After all the
deliberations, we (Jon, Julian, Winsley, Matze, perhaps others from Website
infra team) decided to go for a solution that gives more control and
responsibility to the website developers and doesn't increase our dependency on
Mercurial.

I like your proposal (or my second option from the comment linked above) for
that it makes CMS more aware of the dependencies and more able to ensure that
they are put in place consistently and have right versions. What I don't like
about it is that it makes CMS and the websites more dependent on Mercurial. In
my experience the use of `MercurialSource` has only brought us awkwardness in
testing (with potential missed issues) and problems in production (albeit,
because people had wrong expectations about CMS behavior).

My preference would be to have a simpler CMS that is just a site generator that
knows nothing about version control and use standard tools to take care of
dependency management. However, my preference is not as important as what
website developers think and how they would prefer to work. So far, my
impression was that they share my preferences. Perhaps Julian and Winsley, who
are also CC'd on this review can comment on this.

If it matters, from where we are now, implementing Wladimir's proposal in
addition to, or instead of the current approach, would probably take me about a
day of work.

[Sebastian Noack, 2017/09/29 15:55:07]

I agree with Wladimir. His suggestion seems to be the simplest and most
consistent way to make sure the right dependencies are used, and also gives more
control to websites developers.

If we go with your intial approach, treating all dependencies as local file
path, this will defeat the whole purpose of mercurial integration in the first
place, and means that developers would have to locally setup the dependencies
manually, and on the server somebody would have to do the same along configuring
another cronjob that keeps the dependency up to date. Hence website developers
wouldn't be able to add/update dependencies on their own, and there is no
mechanism that makes sure the right version of a dependency is used.

As discussed before, adding support for Git would be trivial, if it is preferred
over Mercurial. But making the CMS agnostic of version control is probably a bad
idea, even more so if we want to deal with external resources now.

+            for p in additional_paths
+        ]
+        source = MultiSource([source] + additional_sources)
+
+    if static:
+        for fname in [
+            'resolve_link',
+            'read_config',
+            'read_template',
+            'read_locale',
+            'read_include',
+            'exec_file',
+        ]:
+            setattr(source, fname, _memoize(getattr(source, fname)))
+
+    return source