launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #29074
[Merge] ~cjwatson/launchpad:rework-zcml-overrides into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:rework-zcml-overrides into launchpad:master.
Commit message:
Avoid writing to code tree for config-specific ZCML overrides
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/428905
Launchpad configs often include some ZCML overrides (for example, `configs/testrunner/yuitest.zcml` hooks up a fixture for JavaScript testing). These were previously loaded by way of service startup code that generated `zcml/+config-overrides.zcml`, which was then loaded by `zcml/webapp.zcml` and `zcml/ftesting.zcml`.
However, this arrangement required that service startup code be able to write to the code tree. This didn't work well in the context of Juju charms based on the `ols` layer (which arrange for the code tree to be writable only by root), and it essentially amounts to self-modifying code which seems like dubious design in general.
To avoid this problem, implement a new `lp:includeLaunchpadOverrides` ZCML directive which includes all the `*.zcml` files in the current instance's configuration. This is equivalent to the old arrangement, but without requiring the service to modify its own code tree at startup.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:rework-zcml-overrides into launchpad:master.
diff --git a/lib/lp/scripts/runlaunchpad.py b/lib/lp/scripts/runlaunchpad.py
index 1336ec3..904fe57 100644
--- a/lib/lp/scripts/runlaunchpad.py
+++ b/lib/lp/scripts/runlaunchpad.py
@@ -341,8 +341,6 @@ def start_launchpad(argv=list(sys.argv), setup=None):
services, argv = split_out_runlaunchpad_arguments(argv[1:])
argv = process_config_arguments(argv)
services = get_services_to_run(services)
- # Create the ZCML override file based on the instance.
- config.generate_overrides()
# Many things rely on a directory called 'logs' existing in the current
# working directory.
ensure_directory_exists("logs")
@@ -385,8 +383,6 @@ def start_launchpad(argv=list(sys.argv), setup=None):
def start_librarian():
"""Start the Librarian in the background."""
- # Create the ZCML override file based on the instance.
- config.generate_overrides()
# Create the Librarian storage directory if it doesn't already exist.
prepare_for_librarian()
pidfile = pidfile_path("librarian")
diff --git a/lib/lp/scripts/utilities/killtestservices.py b/lib/lp/scripts/utilities/killtestservices.py
index 1fc4281..228881c 100755
--- a/lib/lp/scripts/utilities/killtestservices.py
+++ b/lib/lp/scripts/utilities/killtestservices.py
@@ -19,7 +19,6 @@ def main():
# Tell lp.services.config to use the testrunner config instance, so that
# we don't kill the real services.
config.setInstance("testrunner")
- config.generate_overrides()
print("Killing Memcached....", end="")
kill_by_pidfile(MemcachedLayer.getPidFile())
print("done.")
diff --git a/lib/lp/scripts/utilities/test.py b/lib/lp/scripts/utilities/test.py
index bb852f0..d997a8a 100755
--- a/lib/lp/scripts/utilities/test.py
+++ b/lib/lp/scripts/utilities/test.py
@@ -71,7 +71,6 @@ def configure_environment():
# Tell lp.services.config to use the testrunner config instance.
config.setInstance("testrunner")
- config.generate_overrides()
# Remove this module's directory from path, so that zope.testbrowser
# can import pystone from test:
diff --git a/lib/lp/services/config/__init__.py b/lib/lp/services/config/__init__.py
index 3924ca1..859a38a 100644
--- a/lib/lp/services/config/__init__.py
+++ b/lib/lp/services/config/__init__.py
@@ -16,7 +16,6 @@ import sys
from lazr.config import ImplicitTypeSchema
from lazr.config.interfaces import ConfigErrors
-from lp.services.osutils import open_for_writing
from lp.services.propertycache import cachedproperty, get_property_cache
__all__ = [
@@ -232,27 +231,6 @@ class LaunchpadConfig:
text = f.read()
self._config.push(path, text)
- def generate_overrides(self):
- """Ensure correct config.zcml overrides will be called.
-
- Call this method before letting any ZCML processing occur.
- """
- loader_file = os.path.join(self.root, "zcml/+config-overrides.zcml")
- loader = open_for_writing(loader_file, "w")
-
- print(
- """
- <configure xmlns="http://namespaces.zope.org/zope";>
- <!-- This file automatically generated using
- lp.services.config.LaunchpadConfig.generate_overrides.
- DO NOT EDIT. -->
- <include files="%s/*.zcml" />
- </configure>"""
- % self.config_dir,
- file=loader,
- )
- loader.close()
-
def appserver_root_url(self, facet="mainsite", ensureSlash=False):
"""Return the correct app server root url for the given facet."""
root_url = str(getattr(self.vhost, facet).rooturl)
diff --git a/lib/lp/services/config/tests/test_config_lookup.py b/lib/lp/services/config/tests/test_config_lookup.py
index 4f04561..277883b 100644
--- a/lib/lp/services/config/tests/test_config_lookup.py
+++ b/lib/lp/services/config/tests/test_config_lookup.py
@@ -121,30 +121,3 @@ class TestInstanceConfigDirLookup(ConfigTestCase):
config_file.close()
self.assertEqual(2323, cfg.launchpad.default_batch_size)
-
-
-class TestGenerateOverrides(ConfigTestCase):
- """Test the generate_overrides method of LaunchpadConfig."""
-
- def test_generate_overrides(self):
- instance_dir = self.setUpInstanceConfig("zcmltest")
- cfg = config.LaunchpadConfig("zcmltest")
- # The ZCML override file is generated in the root of the tree.
- # Set that root to the temporary directory.
- cfg.root = self.temp_config_root_dir
- cfg.generate_overrides()
- override_file = os.path.join(cfg.root, "zcml/+config-overrides.zcml")
- self.assertTrue(
- os.path.isfile(override_file), "Overrides file wasn't created."
- )
-
- fh = open(override_file)
- overrides = fh.read()
- fh.close()
-
- magic_line = '<include files="%s/*.zcml" />' % instance_dir
- self.assertTrue(
- magic_line in overrides,
- "Overrides doesn't contain the magic include line (%s):\n%s"
- % (magic_line, overrides),
- )
diff --git a/lib/lp/services/config/tests/test_zcml.py b/lib/lp/services/config/tests/test_zcml.py
new file mode 100644
index 0000000..475ee48
--- /dev/null
+++ b/lib/lp/services/config/tests/test_zcml.py
@@ -0,0 +1,54 @@
+# Copyright 2022 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for lp.services.config.zcml."""
+
+import io
+import os.path
+from textwrap import dedent
+
+from zope.configuration.config import ConfigurationMachine
+from zope.configuration.xmlconfig import (
+ processxmlfile,
+ registerCommonDirectives,
+)
+
+from lp.services.config.fixture import ConfigUseFixture
+from lp.services.config.tests.test_config_lookup import ConfigTestCase
+from lp.testing.layers import BaseLayer
+
+
+class TestIncludeLaunchpadOverrides(ConfigTestCase):
+
+ layer = BaseLayer
+
+ def test_includes_overrides(self):
+ instance_dir = self.setUpInstanceConfig("zcmltest")
+ self.useFixture(ConfigUseFixture("zcmltest"))
+ with open(os.path.join(instance_dir, "test.zcml"), "w") as f:
+ f.write(
+ dedent(
+ """
+ <configure xmlns="http://namespaces.zope.org/zope";
+ xmlns:meta="http://namespaces.zope.org/meta";>
+ <meta:provides feature="testfeature" />
+ </configure>
+ """
+ )
+ )
+ context = ConfigurationMachine()
+ self.assertFalse(context.hasFeature("testfeature"))
+ registerCommonDirectives(context)
+ topfile = io.StringIO(
+ dedent(
+ """
+ <configure xmlns="http://namespaces.zope.org/zope";
+ xmlns:lp="http://namespaces.canonical.com/lp";>
+ <include package="lp.services.config" file="meta.zcml" />
+ <lp:includeLaunchpadOverrides />
+ </configure>
+ """
+ )
+ )
+ processxmlfile(topfile, context, testing=True)
+ self.assertTrue(context.hasFeature("testfeature"))
diff --git a/lib/lp/services/config/zcml.py b/lib/lp/services/config/zcml.py
new file mode 100644
index 0000000..6ace438
--- /dev/null
+++ b/lib/lp/services/config/zcml.py
@@ -0,0 +1,19 @@
+# Copyright 2022 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""ZCML directives relating to Launchpad configuration."""
+
+__all__ = [
+ "include_launchpad_overrides",
+]
+
+import os.path
+
+from zope.configuration.xmlconfig import includeOverrides
+
+from lp.services.config import config
+
+
+def include_launchpad_overrides(context):
+ """Include overrides depending on the Launchpad instance name."""
+ includeOverrides(context, files=os.path.join(config.config_dir, "*.zcml"))
diff --git a/utilities/list-pages b/utilities/list-pages
index 0e64904..187f4e7 100755
--- a/utilities/list-pages
+++ b/utilities/list-pages
@@ -53,7 +53,6 @@ from zope.interface import directlyProvides, implementer
from zope.publisher.interfaces.browser import IDefaultBrowserLayer
import zcml
-from lp.services.config import config
from lp.services.scripts import execute_zcml_for_scripts
from lp.services.webapp.interfaces import ICanonicalUrlData
from lp.services.webapp.publisher import canonical_url
@@ -64,8 +63,6 @@ ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
def load_zcml(zopeless=False):
"""Load ZCML the way we do in the web site."""
- # We can't load ZCML without generating configuration.
- config.generate_overrides()
if zopeless:
execute_zcml_for_scripts()
else:
diff --git a/zcml/ftesting.zcml b/zcml/ftesting.zcml
index 9c684eb..35124c0 100644
--- a/zcml/ftesting.zcml
+++ b/zcml/ftesting.zcml
@@ -4,7 +4,8 @@
<!-- Used by the FunctionalLayer in lp.testing.layers. -->
-<configure xmlns="http://namespaces.zope.org/zope";>
+<configure xmlns="http://namespaces.zope.org/zope";
+ xmlns:lp="http://namespaces.canonical.com/lp";>
<include file="common.zcml" />
@@ -13,7 +14,9 @@
<includeOverrides files="override-includes/*-configure.zcml" />
<includeOverrides files="override-includes/*-configure-testing.zcml" />
- <includeOverrides file="+config-overrides.zcml" />
+
+ <include package="lp.services.config" file="meta.zcml" />
+ <lp:includeLaunchpadOverrides />
<include file="summarizerequests.zcml" />
diff --git a/zcml/script-testing.zcml b/zcml/script-testing.zcml
index 0d1c6c0..e2ccae8 100644
--- a/zcml/script-testing.zcml
+++ b/zcml/script-testing.zcml
@@ -12,7 +12,7 @@
<includeOverrides files="override-includes/*-configure.zcml" />
<includeOverrides files="override-includes/*-configure-testing.zcml" />
- <!-- No +config-overrides here, as the mail config can cause celery
- tests to fail. -->
+ <!-- No <includeLaunchpadOverrides /> here, as the mail config can cause
+ celery tests to fail. -->
</configure>
diff --git a/zcml/script.zcml b/zcml/script.zcml
index 01ae301..2e7b03e 100644
--- a/zcml/script.zcml
+++ b/zcml/script.zcml
@@ -11,8 +11,8 @@
<includeOverrides files="override-includes/*-configure.zcml" />
<includeOverrides files="override-includes/*-configure-normal.zcml" />
- <!-- No +config-overrides here, as the mail config can cause celery
- tests to fail. -->
+ <!-- No <includeLaunchpadOverrides /> here, as the mail config can cause
+ celery tests to fail. -->
<!-- Add a hook to configure the email stuff using ZCML stored outside
of the launchpad tree -->
diff --git a/zcml/webapp.zcml b/zcml/webapp.zcml
index f949d9f..929f06c 100644
--- a/zcml/webapp.zcml
+++ b/zcml/webapp.zcml
@@ -2,7 +2,8 @@
GNU Affero General Public License version 3 (see the file LICENSE).
-->
-<configure xmlns="http://namespaces.zope.org/zope";>
+<configure xmlns="http://namespaces.zope.org/zope";
+ xmlns:lp="http://namespaces.canonical.com/lp";>
<include file="common.zcml" />
@@ -11,7 +12,9 @@
<includeOverrides files="override-includes/*-configure.zcml" />
<includeOverrides files="override-includes/*-configure-normal.zcml" />
- <includeOverrides file="+config-overrides.zcml" />
+
+ <include package="lp.services.config" file="meta.zcml" />
+ <lp:includeLaunchpadOverrides />
<include file="summarizerequests.zcml" />
