fix/caching (#5494)

fix some stuff not being cached, and add test to catch caching regressions.

Test takes quite a while to run, not sure it is a good idea?

Co-authored-by: Darragh Elliott <me@delliott.net>
Reviewed-on: #5494

build/fsfe_website_build/build.py

@@ -22,7 +22,7 @@ from .phase3.stage_to_target import stage_to_target
 logger = logging.getLogger(__name__)
 
 
-def parse_arguments() -> argparse.Namespace:
+def _parse_arguments() -> argparse.Namespace:
     """Parse the arguments of the website build process."""
     parser = argparse.ArgumentParser(
         description="Python script to handle building of the fsfe webpage",
@@ -95,9 +95,8 @@ def parse_arguments() -> argparse.Namespace:
     return args
 
 
-def main() -> None:
-    """Parse args and coordinate the website builder."""
-    args = parse_arguments()
+def build(args: argparse.Namespace) -> None:
+    """Coordinate the website builder."""
     logging.basicConfig(
         format="%(asctime)s - %(name)s - %(levelname)s - %(message)s",
         datefmt="%Y-%m-%d %H:%M:%S",
@@ -168,3 +167,9 @@ def main() -> None:
 
     if args.serve:
         serve_websites(working_target, args.sites, 2000, 100)
+
+
+def main() -> None:
+    """Parse args and run build."""
+    args = _parse_arguments()
+    build(args)

build/fsfe_website_build/lib/checks.py

@@ -21,7 +21,8 @@ def compare_files(
 ) -> list[str]:
     """Compare two xml files, passes as paths."""
     try:
-        t1, t2 = etree.parse(file1), etree.parse(file2)
+        parser = etree.XMLParser(remove_comments=True)
+        t1, t2 = etree.parse(file1, parser), etree.parse(file2, parser)
     except etree.XMLSyntaxError as e:
         logger.critical("XML parse error: %s", e)
         sys.exit(1)
@@ -54,14 +55,14 @@ def compare_elements(
     attributes_of_elem1 = dict(elem1.attrib.items())
     attributes_of_elem2 = dict(elem2.attrib.items())
 
-    only_in_elem1 = set(attributes_of_elem1) - set(attributes_of_elem2)
-    only_in_elem2 = set(attributes_of_elem2) - set(attributes_of_elem1)
-    common = set(attributes_of_elem1) & set(attributes_of_elem2)
+    only_in_elem1 = sorted(set(attributes_of_elem1) - set(attributes_of_elem2))
+    only_in_elem2 = sorted(set(attributes_of_elem2) - set(attributes_of_elem1))
+    common = sorted(set(attributes_of_elem1) & set(attributes_of_elem2))
 
     if only_in_elem1 or only_in_elem2:
         errors.append(
             f"Attribute delta at <{elem1.tag}>"
-            f"  only 1: {list(only_in_elem1)}  only 2: {list(only_in_elem2)}"
+            f"  only 1: {only_in_elem1}  only 2: {only_in_elem2}"
         )
     for key in common:
         if (
@@ -87,4 +88,5 @@ def compare_elements(
             compare_elements(child1, child2, attr_whitelist, _path=f"{tag_path}[{idx}]")
         )
 
+    # this should be stable from the sorts above, so no need to sort it here
     return errors

build/fsfe_website_build/lib/misc.py

@@ -109,7 +109,6 @@ def get_version(file: Path) -> int:
     xml = etree.parse(file)
     result_list = xml.xpath("/*/version")
     result = result_list[0].text if result_list else str(0)
-    logger.debug("Got version: %s", result)
     return int(result)
 
 

build/fsfe_website_build/phase1/prepare_subdirectories.py

@@ -23,7 +23,12 @@ def prepare_subdirectories(
 ) -> None:
     """Find any subdir scripts in subdirectories and run them."""
     logger.info("Preparing Subdirectories")
-    for subdir_path in (path.parent for path in source_dir.glob("**/subdir.py")):
+    for subdir_path in sorted(
+        (path.parent for path in source_dir.glob("**/subdir.py")),
+        key=lambda directory: directory.joinpath("subdir-prio.txt").read_text().strip()
+        if directory.joinpath("subdir-prio.txt").exists()
+        else "0",
+    ):
         logger.info("Preparing subdirectory %s", subdir_path)
         sys.path.append(str(subdir_path.resolve()))
         # Ignore this very sensible warning, as we do evil things

build/fsfe_website_build/phase1/update_xmllists.py

@@ -54,12 +54,14 @@ def _update_for_base(  # noqa: PLR0913
                     .replace("$lastyear", lastyear)
                     .strip()
                 )
-                if len(pattern) <= 0:
-                    logger.debug("Pattern too short, continue!")
+                if not pattern:
+                    logger.debug("Pattern match empty, continue!")
                     continue
-                search_result = re.search(r":\[(.*)\]", line)
+                tag_search_result = re.search(r":\[(.*)\]", line)
                 tag = (
-                    search_result.group(1).strip() if search_result is not None else ""
+                    tag_search_result.group(1).strip()
+                    if tag_search_result is not None
+                    else ""
                 )
 
                 for xml_file in filter(
@@ -81,9 +83,7 @@ def _update_for_base(  # noqa: PLR0913
                         )
                         if tag != ""
                         else True
-                    )
-                    # Not just matching an empty xml_file
-                    and len(str(xml_file)) > 0,
+                    ),
                     all_xml,
                 ):
                     matching_files.add(str(xml_file.relative_to(source)))
@@ -94,10 +94,9 @@ def _update_for_base(  # noqa: PLR0913
             matching_files.add(
                 f"{source}/global/data/modules/{module.get('id').strip()}"
             )
-    matching_files = set(sorted(matching_files))  # noqa: C414
     update_if_changed(
         Path(f"{base.parent}/.{base.name}.xmllist"),
-        ("\n".join(matching_files) + "\n") if matching_files else "",
+        ("\n".join(sorted(matching_files)) + "\n"),
     )
 
 
@@ -112,10 +111,14 @@ def _update_module_xmllists(
     # Get all the bases and stuff before multithreading the update bit
     all_xml = {
         get_basepath(path)
-        for path in filter(
-            lambda path: lang_from_filename(path) in languages,
-            list(source_dir.glob("**/*.*.xml"))
-            + list(source.joinpath("global/").glob("**/*.*.xml")),
+        for path in sorted(
+            filter(
+                lambda path: lang_from_filename(path) in languages,
+                (
+                    *source_dir.glob("**/*.*.xml"),
+                    *source.joinpath("global/").glob("**/*.*.xml"),
+                ),
+            )
         )
     }
     source_bases = {path.with_suffix("") for path in source_dir.glob("**/*.sources")}

build/fsfe_website_build_tests/caching_test.py

@@ -0,0 +1,51 @@
+# SPDX-FileCopyrightText: Free Software Foundation Europe e.V. <https://fsfe.org>
+#
+# SPDX-License-Identifier: GPL-3.0-or-later
+from argparse import Namespace
+from pathlib import Path
+
+from fsfe_website_build.build import build
+from pytest_mock import MockFixture
+
+
+def no_rebuild_twice_test(mocker: MockFixture) -> None:
+    # first, run a full build
+    args = Namespace(
+        full=True,
+        languages=[
+            "ar",
+            "de",
+            "en",
+            "es",
+            "fr",
+            "it",
+        ],
+        log_level="DEBUG",
+        processes=8,
+        source=Path(),
+        serve=False,
+        sites=[
+            Path("drm.info"),
+            Path("fsfe.org"),
+            Path("pdfreaders.org"),
+            Path("status.fsfe.org"),
+        ],
+        stage=False,
+        target="output/final",
+    )
+    build(args)
+
+    # replace update_if_changed with
+    # mocked one that exceptions if the file would be changed
+    def fail_if_update(path: Path, content: str) -> None:
+        if not path.exists() or path.read_text() != content:
+            raise AssertionError(
+                f"File {path} would have been updated on incremental build."
+            )
+
+    mocker.patch(
+        "fsfe_website_build.lib.misc.update_if_changed", side_effect=fail_if_update
+    )
+    # now, run a normal build
+    args.full = False
+    build(args)