implement feedback

Author: Leon-hk

src/gardenlinux/features/parser.py

@@ -84,7 +84,7 @@ def graph(self) -> networkx.Graph:
 
         if self._graph is None:
             feature_yaml_files = glob("{0}/*/info.yaml".format(self._feature_base_dir))
-            features = [self.read_feature_yaml(i) for i in feature_yaml_files]
+            features = [self._read_feature_yaml(i) for i in feature_yaml_files]
 
             feature_graph = networkx.DiGraph()
 
@@ -345,7 +345,7 @@ def _get_node_features(self, node: Dict[str, Any]) -> Dict[str, Any]:
 
         return node.get("content", {}).get("features", {})  # type: ignore[no-any-return]
 
-    def read_feature_yaml(self, feature_yaml_file: str) -> Dict[str, Any]:
+    def _read_feature_yaml(self, feature_yaml_file: str) -> Dict[str, Any]:
         """
         Reads and returns the content of the given features file.
 

src/gardenlinux/features/reproducibility/__main__.py

@@ -13,6 +13,9 @@
 from .comparator import Comparator
 from .markdown_formatter import MarkdownFormatter
 
+# Use custom exit code to make a controlled failure visible
+DIFFERENCE_DETECTED_EXIT_CODE = 64
+
 
 def generate(args: argparse.Namespace) -> None:
     """
@@ -42,7 +45,7 @@ def generate(args: argparse.Namespace) -> None:
         print(result, end="")
 
     if files != []:
-        exit(64)
+        exit(DIFFERENCE_DETECTED_EXIT_CODE)
 
 
 def format(args: argparse.Namespace) -> None:

src/gardenlinux/features/reproducibility/comparator.py

@@ -5,6 +5,8 @@
 """
 
 import filecmp
+import importlib
+import importlib.resources
 import json
 import re
 import tarfile
@@ -29,17 +31,9 @@ class Comparator(object):
 
     _default_whitelist: list[str] = []
 
-    _nightly_whitelist = [
-        r"/etc/apt/sources\.list\.d/gardenlinux\.sources",
-        r"/etc/os-release",
-        r"/etc/shadow",
-        r"/etc/update-motd\.d/05-logo",
-        r"/var/lib/apt/lists/packages\.gardenlinux\.io_gardenlinux_dists_[0-9]*\.[0-9]*\.[0-9]*_.*",
-        r"/var/lib/apt/lists/packages\.gardenlinux\.io_gardenlinux_dists_[0-9]*\.[0-9]*\.[0-9]*_main_binary-(arm64|amd64)_Packages",
-        r"/efi/loader/entries/Default-[0-9]*\.[0-9]*\.[0-9]*-(cloud-)?(arm64|amd64)\.conf",
-        r"/efi/Default/[0-9]*\.[0-9]*\.[0-9]*-(cloud-)?(arm64|amd64)/initrd",
-        r"/boot/initrd\.img-[0-9]*\.[0-9]*\.[0-9]*-(cloud-)?(arm64|amd64)",
-    ]
+    _nightly_whitelist = json.loads(
+        importlib.resources.read_text(__name__, "nightly_whitelist.json")
+    )
 
     def __init__(
         self, nightly: bool = False, whitelist: list[str] = _default_whitelist

src/gardenlinux/features/reproducibility/diff_parser.py

@@ -56,25 +56,23 @@ def __init__(
         self._parser = Parser(gardenlinux_root, feature_dir_name, logger)
         self._feature_dir_name = Path(self._gardenlinux_root).joinpath(feature_dir_name)
 
-        self.all: set[str] = set()
-        self.successful: set[str] = set()
-        self.whitelist: set[str] = set()
+        self.all_flavors: set[str] = set()
+        self.reproducible_flavors: set[str] = set()
+        self.passed_by_whitelist: set[str] = set()
         self.expected_falvors: set[str] = set()
         self.missing_flavors: set[str] = set()
         self.unexpected_falvors: set[str] = set()
 
-    def read_feature_info(self, feature: str) -> Any:
+    def sort_features(self, graph: nx.DiGraph) -> list[str]:
         """
-        Read the content of the feature info.yaml
+        Forward sorting implemented in the parser
 
-        :param feature:                 The queried feature
+        :param graph:                   The feature graph
 
-        :return: Dict[str, Any]         Parsed content of the features' info.yaml file
+        :return: list[str]              Sorted features considering the type
         :since: 1.0.0
         """
-        return self._parser.read_feature_yaml(
-            str(self._feature_dir_name.joinpath(f"{feature}/info.yaml"))
-        )["content"]
+        return self._parser.sort_graph_nodes(graph)
 
     def parse(
         self,
@@ -92,10 +90,10 @@ def parse(
         :since:  1.0.0
         """
 
-        self.all = set()
-        self.successful = set()
-        self.whitelist = set()
-        failed = {}  # {flavor: [files...]}
+        self.all_flavors = set()
+        self.reproducible_flavors = set()
+        self.passed_by_whitelist = set()
+        non_reproducible_flavors = {}  # {flavor: [files...]}
 
         diff_dir = Path(self._gardenlinux_root).joinpath(diff_dir)
 
@@ -110,32 +108,32 @@ def parse(
                     content = f.read()
 
                 flavor = flavor.rstrip(self._SUFFIX)
-                self.all.add(flavor)
+                self.all_flavors.add(flavor)
                 if content == "":
-                    self.successful.add(flavor)
+                    self.reproducible_flavors.add(flavor)
                 elif content == "whitelist\n":
-                    self.successful.add(flavor)
-                    self.whitelist.add(flavor)
+                    self.reproducible_flavors.add(flavor)
+                    self.passed_by_whitelist.add(flavor)
                 else:
-                    failed[flavor] = content.split("\n")[:-1]
+                    non_reproducible_flavors[flavor] = content.split("\n")[:-1]
 
-        self.missing_flavors = self.expected_falvors - self.all
-        self.unexpected_falvors = self.all - self.expected_falvors
+        self.missing_flavors = self.expected_falvors - self.all_flavors
+        self.unexpected_falvors = self.all_flavors - self.expected_falvors
 
         # Map files to flavors
-        affected: Dict[str, set[str]] = {}  # {file: {flavors...}}
-        for flavor in failed:
-            for file in failed[flavor]:
-                if file not in affected:
-                    affected[file] = set()
-                affected[file].add(flavor)
-
-        # Merge files affected by the same flavors by mapping flavor sets to files
+        affected_flavors: Dict[str, set[str]] = {}  # {file: {flavors...}}
+        for flavor in non_reproducible_flavors:
+            for file in non_reproducible_flavors[flavor]:
+                if file not in affected_flavors:
+                    affected_flavors[file] = set()
+                affected_flavors[file].add(flavor)
+
+        # Merge files affected_flavors by the same flavors by mapping flavor sets to files
         self._bundled: Dict[frozenset[str], set[str]] = {}  # {{flavors...}: {files...}}
-        for file in affected:
-            if frozenset(affected[file]) not in self._bundled:
-                self._bundled[frozenset(affected[file])] = set()
-            self._bundled[frozenset(affected[file])].add(file)
+        for file in affected_flavors:
+            if frozenset(affected_flavors[file]) not in self._bundled:
+                self._bundled[frozenset(affected_flavors[file])] = set()
+            self._bundled[frozenset(affected_flavors[file])].add(file)
 
     def intersectionTrees(
         self,
@@ -150,27 +148,32 @@ def intersectionTrees(
         # Compute the intersecting features of the affected flavors and store them in a graph to allow hierarchical formatting
         trees = {}
         for flavors in self._bundled:
-            first = True
             tree = None
             # Compute the intersecting features of all affected flavors
             for flavor in flavors:
                 # Ignore bare flavors, as they may not be affected due to removing the file and could therefore disrupt the analysis
                 if not flavor.startswith("bare-"):
-                    t = self._parser.filter(self._remove_arch.sub("", flavor))
-                    if first:
-                        first = False
-                        tree = t
-                    else:
-                        tree = nx.intersection(tree, t)
+                    # Compute intersecting features
+                    tree = self._parser.filter(
+                        self._remove_arch.sub("", flavor),
+                        additional_filter_func=tree.__contains__
+                        if tree is not None
+                        else lambda _: True,
+                    )
 
             # Remove any features which are contained in unaffected flavors, as they cannot cause the problem
             if tree is not None:
-                unaffected = self.all - flavors
+                # Unfreeze tree
+                tree = nx.DiGraph(tree)
+                unaffected = self.all_flavors - flavors
+                merged_features: set[str] = set()
                 for flavor in unaffected:
                     # Again, ignore bare flavors
                     if not flavor.startswith("bare-"):
-                        t = self._parser.filter(self._remove_arch.sub("", flavor))
-                        tree.remove_nodes_from(n for n in t)
+                        merged_features.update(
+                            self._parser.filter(self._remove_arch.sub("", flavor))
+                        )
+                tree.remove_nodes_from(n for n in merged_features)
             else:
                 tree = nx.DiGraph()
 

src/gardenlinux/features/reproducibility/markdown_formatter.py

@@ -7,14 +7,16 @@
 import logging
 from os import PathLike
 from pathlib import Path
+from string import Template
 from typing import Collection, Optional
 
 import networkx as nx
 from attr import dataclass
-from networkx.algorithms.traversal.depth_first_search import dfs_tree
 
 from gardenlinux.features.reproducibility.diff_parser import DiffParser
 
+SUCCESS_TRESHOLD = 50.0
+
 
 @dataclass
 class Nightly:
@@ -40,6 +42,10 @@ class MarkdownFormatter(object):
 
     _DROPDOWN_THRESHOLD = 10
 
+    _nighly_url_template = Template(
+        "https://github.com/gardenlinux/gardenlinux/actions/runs/$id"
+    )
+
     def __init__(
         self,
         flavors_matrix: dict[str, list[dict[str, str]]],
@@ -69,24 +75,6 @@ def __init__(
 
         self._nightly_stats = Path(nightly_stats)
 
-    def _node_key(self, node: str) -> str:
-        """
-        Key order function to sort platforms before elements, platforms before flags and elements before flags
-
-        :param node:                    The node name (can be any of platform, element or flag)
-
-        :return: ("1-" || "2-" || "2-") + node
-        :since:  1.0.0
-        """
-
-        info = self._diff_parser.read_feature_info(node)
-        if info["type"] == "platform":
-            return "1-" + node
-        elif info["type"] == "element":
-            return "2-" + node
-        else:
-            return "3-" + node
-
     def _treeStr(
         self, graph: nx.DiGraph, found: Optional[set[str]] = None
     ) -> tuple[str, set[str]]:
@@ -104,17 +92,25 @@ def _treeStr(
             found = set()
 
         s = ""
-        for node in sorted(graph, key=self._node_key):
+        for node in self._diff_parser.sort_features(graph):
             if node not in found and len(list(graph.predecessors(node))) == 0:
                 found.add(node)
                 if len(set(graph.successors(node)) - found) == 0:
                     s += str(node) + "\n"
                 else:
                     s += str(node) + ":\n"
-                    for successor in sorted(
-                        set(graph.successors(node)) - found, key=self._node_key
+                    # This filters all direct successors of node in a sorted order
+                    for successor in self._diff_parser.sort_features(
+                        nx.subgraph_view(
+                            graph, filter_node=list(graph.successors(node)).__contains__
+                        )
                     ):
-                        st, fnd = self._treeStr(dfs_tree(graph, successor), found)
+                        # Manually create dfs tree to keep attributes
+                        subtree = nx.DiGraph(graph)
+                        subtree.remove_edges_from(
+                            subtree.edges - nx.dfs_edges(graph, successor)
+                        )
+                        st, fnd = self._treeStr(subtree, found)
                         found.update(fnd)
                         s += "  " + st.replace("\n", "\n  ") + "\n"
         # Remove last linebreak as the last line can contain spaces
@@ -156,14 +152,14 @@ def _format_nighlty_stats(self) -> str:
                 Nightly(*n.split(",")) for n in f.read().rstrip().split("\n")
             )
             if nightly_a.run_number != "":
-                result += f"\n\nComparison of nightly **[#{nightly_a.run_number}](https://github.com/gardenlinux/gardenlinux/actions/runs/{nightly_a.id})** \
-and **[#{nightly_b.run_number}](https://github.com/gardenlinux/gardenlinux/actions/runs/{nightly_b.id})**"
+                result += f"\n\nComparison of nightly **[#{nightly_a.run_number}]({self._nighly_url_template.substitute(id=nightly_a.id)})** \
+and **[#{nightly_b.run_number}]({self._nighly_url_template.substitute(id=nightly_b.id)})**"
                 if nightly_a.commit != nightly_b.commit:
                     result += f"\n\n⚠️ The nightlies used different commits: `{nightly_a.commit[:7]}` (#{nightly_a.run_number}) != `{nightly_b.commit[:7]}` (#{nightly_b.run_number})"
                 if nightly_a.run_number == nightly_b.run_number:
-                    result += f"\n\n⚠️ Comparing the nightly **[#{nightly_a.run_number}](https://github.com/gardenlinux/gardenlinux/actions/runs/{nightly_a.id})** to itself can not reveal any issues"
+                    result += f"\n\n⚠️ Comparing the nightly **[#{nightly_a.run_number}]({self._nighly_url_template.substitute(id=nightly_a.id)})** to itself can not reveal any issues"
             else:
-                result += f"\n\nComparison of the latest nightly **[#{nightly_b.run_number}](https://github.com/gardenlinux/gardenlinux/actions/runs/{nightly_b.id})** \
+                result += f"\n\nComparison of the latest nightly **[#{nightly_b.run_number}]({self._nighly_url_template.substitute(id=nightly_b.id)})** \
 with a new build"
                 if nightly_a.commit != nightly_b.commit:
                     result += f"\n\n⚠️ The build used different commits: `{nightly_b.commit[:7]}` (#{nightly_b.run_number}) != `{nightly_a.commit[:7]}` (new build)"
@@ -193,7 +189,7 @@ def _header(
         successrate = round(
             100
             * (
-                len(self._diff_parser.successful)
+                len(self._diff_parser.reproducible_flavors)
                 / len(self._diff_parser.expected_falvors)
             ),
             1,
@@ -202,8 +198,8 @@ def _header(
         emoji = (
             "✅"
             if len(self._diff_parser.expected_falvors)
-            == len(self._diff_parser.successful)
-            else ("⚠️" if successrate >= 50.0 else "❌")
+            == len(self._diff_parser.reproducible_flavors)
+            else ("⚠️" if successrate >= SUCCESS_TRESHOLD else "❌")
         )
 
         total_count = len(self._diff_parser.expected_falvors)
@@ -223,10 +219,10 @@ def _header(
         if self._nightly_stats.is_file():
             explanation += self._format_nighlty_stats()
 
-        if len(self._diff_parser.whitelist) > 0:
+        if len(self._diff_parser.passed_by_whitelist) > 0:
             explanation += (
                 "\n\n<details><summary>📃 These flavors only passed due to the nightly whitelist</summary><pre>"
-                + "<br>".join(sorted(self._diff_parser.whitelist))
+                + "<br>".join(sorted(self._diff_parser.passed_by_whitelist))
                 + "</pre></details>"
             )
 
@@ -242,7 +238,7 @@ def _header(
         explanation += (
             ""
             if len(self._diff_parser.expected_falvors)
-            <= len(self._diff_parser.successful)
+            <= len(self._diff_parser.reproducible_flavors)
             else "\n\n*The mentioned features are included in every affected flavor and not included in every unaffected flavor.*"
         )
 
@@ -299,19 +295,21 @@ def sorting_function(files: frozenset[str]) -> tuple[int, str]:
             row += "|\n"
             rows += row
 
-        if len(self._diff_parser.successful) > 0:
+        if len(self._diff_parser.reproducible_flavors) > 0:
             # Success row
             row = "|"
             row += "✅ No problems found"
             row += "|"
-            row += f"**{round(100 * (len(self._diff_parser.successful) / len(self._diff_parser.expected_falvors)), 1)}%**<br>"
-            row += self._dropdown(self._diff_parser.successful)
+            row += f"**{round(100 * (len(self._diff_parser.reproducible_flavors) / len(self._diff_parser.expected_falvors)), 1)}%**<br>"
+            row += self._dropdown(self._diff_parser.reproducible_flavors)
             row += "|"
             row += "-"
             row += "|\n"
             rows += row
 
-        if len(self._diff_parser.successful) < len(self._diff_parser.expected_falvors):
+        if len(self._diff_parser.reproducible_flavors) < len(
+            self._diff_parser.expected_falvors
+        ):
             rows += "\n*To add affected files to the whitelist, edit the `whitelist` variable in python-gardenlinux-lib `src/gardenlinux/features/reproducibility/comparator.py`*\n"
 
         return table.format(rows=rows)