Merge ~alexmurray/review-tools:dbus-app-activates-on-property-checking into review-tools:master

[Alex Murray]

This is required to allow things like https://dashboard.snapcraft.io/snaps/ubuntu-desktop-session/revisions/3/ to pass automated review.

78e4a06... by Alex Murray on 2022-10-11

Update check-names.list and tests/test.sh.expected for new tests

Signed-off-by: Alex Murray <email address hidden>

[Alex Murray (alexmurray)]

Will likely abandon this in favour of https://code.launchpad.net/~jamesh/review-tools/+git/review-tools/+merge/403932 which I had completely forgotten about.

[Alex Murray (alexmurray)]

Yep, decided to go with jamesh original proposal as that is a bit cleaner than this.

review: Disapprove

Unmerged commits

78e4a06... by Alex Murray on 2022-10-11

Update check-names.list and tests/test.sh.expected for new tests

Signed-off-by: Alex Murray <email address hidden>

2a4797c... by Alex Murray on 2022-10-11

sr_lint: Add support for dbus apps/activates-on property checking

The app bus-name property is essentially deprecated by upstream and instead the
activates-on property is used instead to specify a list of dbus slot names which
the app should activate on. Add support for checking this property is valid and
remove the old activatable dbus slot property which is not actually used by
snapd.

Signed-off-by: Alex Murray <email address hidden>

diff --git a/check-names.list b/check-names.list
index 1a977d9..623dca1 100644
--- a/check-names.list
+++ b/check-names.list
@@ -59,6 +59,7 @@ lint-snap-v2:daemon|
 lint-snap-v2:daemon_required|
 lint-snap-v2:daemon-scope|
 lint-snap-v2:dbus_activatable|
+lint-snap-v2:dbus_activates-on|
 lint-snap-v2:dbus_bus-name_required|
 lint-snap-v2:dbus_mixed_slots|
 lint-snap-v2:dbus_slot_required|
diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py
index aa71ee8..925fb0e 100644
--- a/reviewtools/sr_common.py
+++ b/reviewtools/sr_common.py
@@ -102,6 +102,7 @@ class SnapReview(Review):
     apps_required = ["command"]
     apps_optional_cli = ["autostart"]
     apps_optional_daemon = [
+        "activates-on",
         "after",
         "before",
         "bus-name",  # May deprecate: https://forum.snapcraft.io/t/support-for-daemon-dbus/8855/11
@@ -123,7 +124,6 @@ class SnapReview(Review):
         "watchdog-timeout",
     ]
     apps_optional_shared = [
-        "activates-on",  # eventually apps_optional_daemon (see https://forum.snapcraft.io/t/support-for-daemon-dbus/8855/11)
         "aliases",
         "command",
         "command-chain",
@@ -247,7 +247,6 @@ class SnapReview(Review):
         "dbus": {
             "name/slots": "",
             "bus/slots": "",
-            "activatable/slots": False,
             "name/plugs": "",
             "bus/plugs": "",
         },
diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py
index 1f935db..5d77376 100644
--- a/reviewtools/sr_lint.py
+++ b/reviewtools/sr_lint.py
@@ -874,8 +874,58 @@ class SnapReviewLint(SnapReview):
                 s = "'%s' must not be used with 'daemon'" % ",".join(needs_cli)
             self._add_result(t, n, s)
 
+        # 'activates-on' should specify an appropriate dbus slot
+        for app in self.snap_yaml["apps"]:
+            if "activates-on" not in self.snap_yaml["apps"][app]:
+                continue
+
+            t = "info"
+            n = self._get_check_name("dbus_activates-on", app=app)
+            s = "OK"
+
+            if not isinstance(self.snap_yaml["apps"][app]["activates-on"], list):
+                t = "error"
+                s = "'activates-on' (not a list)"
+                self._add_result(t, n, s)
+                continue
+
+            for name in self.snap_yaml["apps"][app]["activates-on"]:
+                slots = []
+                if "slots" in self.snap_yaml:
+                    slots = self.snap_yaml["slots"]
+                found = False
+                t = "info"
+                n = self._get_check_name("dbus_activates-on", app=app, extra=name)
+                s = "OK"
+                for ref in slots:
+                    if ref == name:
+                        spec = self.snap_yaml["slots"][ref]
+                        if (
+                                isinstance(spec, dict)
+                                and ("interface" in spec and spec["interface"] == "dbus")
+                                # daemon-scope must match dbus bus
+                                and ("daemon-scope" in self.snap_yaml["apps"][app] and (
+                                    (
+                                        self.snap_yaml["apps"][app]["daemon-scope"] == "user" and
+                                        spec["bus"] == "session"
+                                    ) or
+                                    (
+                                        self.snap_yaml["apps"][app]["daemon-scope"] == "system" and
+                                        spec["bus"] == "system"
+                                    )
+                                ))
+                        ):
+                            found = True
+                            break
+                if not found:
+                    t = "error"
+                    s = "'activates-on: %s' slot is not defined or is not a matching dbus slot" % name
+                self._add_result(t, n, s)
+
         # 'daemon: dbus' requires:
-        # a) that 'bus-name' is specified and
+        # a) that 'bus-name' is specified OR that the 'activates-on' property is
+        # specified in which case the last name used there is taken as
+        # 'bus-name'
         # b) that 'bus-name' matches one of the dbus slots
         for app in self.snap_yaml["apps"]:
             if (
@@ -887,17 +937,20 @@ class SnapReviewLint(SnapReview):
             t = "info"
             n = self._get_check_name("dbus_bus-name_required", app=app)
             s = "OK"
-            if "bus-name" not in self.snap_yaml["apps"][app]:
+            if "bus-name" not in self.snap_yaml["apps"][app] and "activates-on" not in self.snap_yaml["apps"][app]:
                 t = "error"
-                s = "use of 'daemon: dbus' requires 'bus-name'"
+                s = "use of 'daemon: dbus' requires 'bus-name' or 'activates-on'"
                 self._add_result(t, n, s)
                 continue
-            elif not isinstance(self.snap_yaml["apps"][app]["bus-name"], str):
+            elif "bus-name" in self.snap_yaml["apps"][app] and not isinstance(self.snap_yaml["apps"][app]["bus-name"], str):
                 t = "error"
                 s = "'bus-name' (not a str)"
                 self._add_result(t, n, s)
                 continue
 
+            if "bus-name" not in self.snap_yaml["apps"][app]:
+                # we checked activates-on above so no need to check it here
+                continue
             needle = self.snap_yaml["apps"][app]["bus-name"]
 
             needs_slot = False
@@ -981,37 +1034,25 @@ class SnapReviewLint(SnapReview):
             if show_report:
                 self._add_result(t, n, s)
 
-        # 'activatable: True' should be used with 'daemon: dbus'
-        has_activatable = False
-        for app in self.snap_yaml["apps"]:
-            if "daemon" not in self.snap_yaml["apps"][app]:
-                continue
-            if "slots" not in self.snap_yaml or len(self.snap_yaml["slots"]) == 0:
-                continue
+        # 'activatable' is now an invalid property
+        if "slots" not in self.snap_yaml or len(self.snap_yaml["slots"]) == 0:
+            return
 
-            for ref in self.snap_yaml["slots"]:
-                spec = self.snap_yaml["slots"][ref]
-                if (
+        for ref in self.snap_yaml["slots"]:
+            t = "info"
+            n = self._get_check_name("dbus_activatable")
+            s = "OK"
+            spec = self.snap_yaml["slots"][ref]
+            if (
                     isinstance(spec, dict)
                     and (
                         ("interface" not in spec and ref == "dbus")
                         or ("interface" in spec and spec["interface"] == "dbus")
                     )
                     and "activatable" in spec
-                    and spec["activatable"]
-                ):
-                    if (
-                        "slots" in self.snap_yaml["apps"][app]
-                        and ref in self.snap_yaml["apps"][app]["slots"]
-                    ):
-                        has_activatable = True
-
-            t = "info"
-            n = self._get_check_name("dbus_activatable", app=app)
-            s = "OK"
-            if has_activatable and self.snap_yaml["apps"][app]["daemon"] != "dbus":
+            ):
                 t = "warn"
-                s = "'activatable: true' should be used with 'daemon: dbus'"
+                s = "'activatable' is now an invalid dbus slot property"
             self._add_result(t, n, s)
 
     def check_apps_restart_condition(self):
diff --git a/reviewtools/tests/test_sr_lint.py b/reviewtools/tests/test_sr_lint.py
index 2f08d21..59e88d5 100644
--- a/reviewtools/tests/test_sr_lint.py
+++ b/reviewtools/tests/test_sr_lint.py
@@ -2307,7 +2307,6 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
                     "interface": "dbus",
                     "bus": "system",
                     "name": "foo",
-                    "activatable": True,
                 }
             },
         )
@@ -2317,15 +2316,17 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
                 "foo": {
                     "command": "bin/bar",
                     "daemon": "dbus",
+                    "daemon-scope": "system",
                     "bus-name": "foo",
                     "slots": ["system"],
+                    "activates-on": ["system"]
                 }
             },
         )
         c = SnapReviewLint(self.test_name)
         c.check_apps_invalid_combinations()
         r = c.review_report
-        expected_counts = {"info": 5, "warn": 0, "error": 0}
+        expected_counts = {"info": 6, "warn": 0, "error": 0}
         self.check_results(r, expected_counts)
 
     def test_check_apps_invalid_combinations_dbus_bus_name_mismatch(self):
@@ -2337,7 +2338,6 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
                     "interface": "dbus",
                     "bus": "system",
                     "name": "mismatch",
-                    "activatable": True,
                 }
             },
         )
@@ -2422,7 +2422,7 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
         c = SnapReviewLint(self.test_name)
         c.check_apps_invalid_combinations()
         r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
+        expected_counts = {"info": None, "warn": 1, "error": 1}
         self.check_results(r, expected_counts)
 
     def test_check_apps_invalid_combinations_dbus_activatable_simple(self):
@@ -2477,6 +2477,114 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
         expected_counts = {"info": None, "warn": 0, "error": 0}
         self.check_results(r, expected_counts)
 
+    def test_check_apps_dbus_activates_on(self):
+        """Test check_apps_invalid_combinations() - dbus activatable/simple"""
+        self.set_test_snap_yaml(
+            "slots", {"system": {"interface": "dbus", "bus": "system", "name": "foo"}}
+        )
+        self.set_test_snap_yaml(
+            "apps",
+            {
+                "foo": {
+                    "command": "bin/bar",
+                    "daemon": "simple",
+                    "bus-name": "foo",
+                    "slots": ["system"],
+                }
+            },
+        )
+        c = SnapReviewLint(self.test_name)
+        c.check_apps_invalid_combinations()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_apps_dbus_activates_on_without_daemon_scope(self):
+        """Test check_apps_invalid_combinations() - dbus activatable without bus-name"""
+        self.set_test_snap_yaml(
+            "slots", {"system": {"interface": "dbus", "bus": "system", "name": "foo"}}
+        )
+        self.set_test_snap_yaml(
+            "apps",
+            {
+                "foo": {
+                    "command": "bin/bar",
+                    "daemon": "dbus",
+                    "activates-on": ["foo"],
+                }
+            },
+        )
+        c = SnapReviewLint(self.test_name)
+        c.check_apps_invalid_combinations()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_apps_dbus_activates_on_without_bus_name(self):
+        """Test check_apps_invalid_combinations() - dbus activatable without bus-name"""
+        self.set_test_snap_yaml(
+            "slots", {"system": {"interface": "dbus", "bus": "system", "name": "foo"}}
+        )
+        self.set_test_snap_yaml(
+            "apps",
+            {
+                "foo": {
+                    "command": "bin/bar",
+                    "daemon": "dbus",
+                    "daemon-scope": "system",
+                    "activates-on": ["system"],
+                }
+            },
+        )
+        c = SnapReviewLint(self.test_name)
+        c.check_apps_invalid_combinations()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_apps_dbus_activates_on_user_session_scope(self):
+        """Test check_apps_invalid_combinations() - dbus activatable on user session scope"""
+        self.set_test_snap_yaml(
+            "slots", {"foo": {"interface": "dbus", "bus": "session", "name": "foo"}}
+        )
+        self.set_test_snap_yaml(
+            "apps",
+            {
+                "foo": {
+                    "command": "bin/bar",
+                    "daemon": "dbus",
+                    "daemon-scope": "user",
+                    "activates-on": ["foo"],
+                }
+            },
+        )
+        c = SnapReviewLint(self.test_name)
+        c.check_apps_invalid_combinations()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_apps_dbus_activates_on_without_bus_name_invalid(self):
+        """Test check_apps_invalid_combinations() - dbus activatable/simple without bus-name but invalid activates-on name"""
+        self.set_test_snap_yaml(
+            "slots", {"system": {"interface": "dbus", "bus": "system", "name": "foo"}}
+        )
+        self.set_test_snap_yaml(
+            "apps",
+            {
+                "foo": {
+                    "command": "bin/bar",
+                    "daemon": "dbus",
+                    "activates-on": ["bar"],
+                }
+            },
+        )
+        c = SnapReviewLint(self.test_name)
+        c.check_apps_invalid_combinations()
+        r = c.review_report
+        expected_counts = {"info": None, "warn": 0, "error": 1}
+        self.check_results(r, expected_counts)
+
     def test_check_apps_invalid_combinations_daemon_scope(self):
         """Test check_apps_invalid_combinations() - daemon-scope"""
         self.set_test_snap_yaml(
diff --git a/tests/test.sh.expected b/tests/test.sh.expected
index 631c6f3..ceb88b5 100644
--- a/tests/test.sh.expected
+++ b/tests/test.sh.expected
@@ -5471,7 +5471,7 @@ network-manager_1.2.2-1+test1_amd64.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
-    "lint-snap-v2:dbus_activatable:networkmanager": {
+    "lint-snap-v2:dbus_activatable": {
       "manual_review": false,
       "text": "OK"
     },
@@ -5713,7 +5713,7 @@ network-manager_1.2.2-1+test1_amd64.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
-      "lint-snap-v2:dbus_activatable:networkmanager": {
+      "lint-snap-v2:dbus_activatable": {
         "manual_review": false,
         "text": "OK"
       },
@@ -11269,6 +11269,10 @@ snappy-test-iface-attribs_0.1_all.snap: pass
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:description": {
       "manual_review": false,
       "text": "description is too short: 'This is a test snap'"
@@ -11485,6 +11489,10 @@ snappy-test-iface-attribs_0.1_all.snap: pass
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:description": {
         "manual_review": false,
         "text": "description is too short: 'This is a test snap'"
@@ -14095,7 +14103,7 @@ test-all-app_1_all.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
-    "lint-snap-v2:dbus_activatable:dockerd": {
+    "lint-snap-v2:dbus_activatable": {
       "manual_review": false,
       "text": "OK"
     },
@@ -17072,7 +17080,7 @@ test-all-app_1_all.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
-      "lint-snap-v2:dbus_activatable:dockerd": {
+      "lint-snap-v2:dbus_activatable": {
         "manual_review": false,
         "text": "OK"
       },
@@ -21557,6 +21565,10 @@ test-base-slots-plugs_1.0_all.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:dbus_mixed_slots:cmd": {
       "manual_review": false,
       "text": "OK"
@@ -21767,6 +21779,10 @@ test-base-slots-plugs_1.0_all.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:dbus_mixed_slots:cmd": {
         "manual_review": false,
         "text": "OK"
@@ -25438,6 +25454,10 @@ test-content_0.1_all.snap: pass
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:description": {
       "manual_review": false,
       "text": "OK"
@@ -25778,6 +25798,10 @@ test-content_0.1_all.snap: pass
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:description": {
         "manual_review": false,
         "text": "OK"
@@ -29445,7 +29469,7 @@ test-hello-dbus_2_amd64.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
-    "lint-snap-v2:dbus_activatable:dbusd-system": {
+    "lint-snap-v2:dbus_activatable": {
       "manual_review": false,
       "text": "OK"
     },
@@ -29886,7 +29910,7 @@ test-hello-dbus_2_amd64.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
-      "lint-snap-v2:dbus_activatable:dbusd-system": {
+      "lint-snap-v2:dbus_activatable": {
         "manual_review": false,
         "text": "OK"
       },
@@ -33113,6 +33137,10 @@ test-missing-required-attributes_0_all.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:dbus_mixed_slots:env": {
       "manual_review": false,
       "text": "OK"
@@ -33343,6 +33371,10 @@ test-missing-required-attributes_0_all.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:dbus_mixed_slots:env": {
         "manual_review": false,
         "text": "OK"
@@ -33576,6 +33608,10 @@ test-mpris-name-matches_0_amd64.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:description": {
       "manual_review": false,
       "text": "description is too short: 'My description\n'"
@@ -33768,6 +33804,10 @@ test-mpris-name-matches_0_amd64.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:description": {
         "manual_review": false,
         "text": "description is too short: 'My description\n'"
@@ -33969,6 +34009,10 @@ test-mpris-name-mismatch_0_amd64.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:description": {
       "manual_review": false,
       "text": "description is too short: 'My description\n'"
@@ -34161,6 +34205,10 @@ test-mpris-name-mismatch_0_amd64.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:description": {
         "manual_review": false,
         "text": "description is too short: 'My description\n'"
@@ -39151,6 +39199,10 @@ test-slot-reference-hook_1_all.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:description": {
       "manual_review": false,
       "text": "description is too short: '...'"
@@ -39351,6 +39403,10 @@ test-slot-reference-hook_1_all.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:description": {
         "manual_review": false,
         "text": "description is too short: '...'"
@@ -39568,6 +39624,10 @@ test-slot-reference_1_all.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:description": {
       "manual_review": false,
       "text": "description is too short: '...'"
@@ -39756,6 +39816,10 @@ test-slot-reference_1_all.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:description": {
         "manual_review": false,
         "text": "description is too short: '...'"
@@ -39953,6 +40017,10 @@ test-slot-toplevel_1_all.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
+    "lint-snap-v2:dbus_activatable": {
+      "manual_review": false,
+      "text": "OK"
+    },
     "lint-snap-v2:description": {
       "manual_review": false,
       "text": "description is too short: '...'"
@@ -40137,6 +40205,10 @@ test-slot-toplevel_1_all.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
+      "lint-snap-v2:dbus_activatable": {
+        "manual_review": false,
+        "text": "OK"
+      },
       "lint-snap-v2:description": {
         "manual_review": false,
         "text": "description is too short: '...'"
@@ -45055,11 +45127,7 @@ test-top-level-dbus-slot_1_amd64.snap: FAIL
       "manual_review": false,
       "text": "OK"
     },
-    "lint-snap-v2:dbus_activatable:dbusd-session": {
-      "manual_review": false,
-      "text": "OK"
-    },
-    "lint-snap-v2:dbus_activatable:dbusd-system": {
+    "lint-snap-v2:dbus_activatable": {
       "manual_review": false,
       "text": "OK"
     },
@@ -45319,11 +45387,7 @@ test-top-level-dbus-slot_1_amd64.snap: FAIL
         "manual_review": false,
         "text": "OK"
       },
-      "lint-snap-v2:dbus_activatable:dbusd-session": {
-        "manual_review": false,
-        "text": "OK"
-      },
-      "lint-snap-v2:dbus_activatable:dbusd-system": {
+      "lint-snap-v2:dbus_activatable": {
         "manual_review": false,
         "text": "OK"
       },