Bphilip/refactor version entangle check add release number (#12910)

binujp · Binu Jose Philip · web-flow · commit 16a75f2d794c · 2025-03-18T18:41:01.000-07:00
MOFED and associated kernel modules did not include kernel minor version in the entangle check. check_entangle script did not provide enough details when it failed.

Added a unit test to make sure the script was indeed doing everything as expected. There was some refactoring done to the script to make the workflow more compact.

Co-authored-by: Binu Jose Philip &lt;bphilip@microsoft.com&gt;
diff --git a/.github/workflows/check-entangled-specs.yml b/.github/workflows/check-entangled-specs.yml
@@ -31,5 +31,9 @@ jobs:
       - name: Get Python dependencies
         run: python3 -m pip install -r toolkit/scripts/requirements.txt
 
-      - name: Run entanglement checking script
+      # Run unit test for check_entangled_specs.py before invoking it
+      - name: Unit test for spec entanglement check
+        run: PYTHONPATH=toolkit/scripts python3 toolkit/scripts/tests/test_check_entangled_specs.py
+
+      - name: Run spec entanglement checking script
         run: python3 toolkit/scripts/check_entangled_specs.py .
diff --git a/SPECS/fwctl/fwctl.spec b/SPECS/fwctl/fwctl.spec
@@ -30,7 +30,7 @@
 # SOFTWARE.
 #
 
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %{!?_name: %define _name fwctl}
 %{!?_version: %define _version 24.10}
diff --git a/SPECS/iser/iser.spec b/SPECS/iser/iser.spec
@@ -26,7 +26,7 @@
 #
 #
 
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/SPECS/isert/isert.spec b/SPECS/isert/isert.spec
@@ -26,7 +26,7 @@
 #
 #
 
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/SPECS/knem/knem.spec b/SPECS/knem/knem.spec
@@ -26,7 +26,7 @@
 # KMP is disabled by default
 %{!?KMP: %global KMP 0}
 
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/SPECS/mft_kernel/mft_kernel.spec b/SPECS/mft_kernel/mft_kernel.spec
@@ -1,4 +1,4 @@
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/SPECS/mlnx-nfsrdma/mlnx-nfsrdma.spec b/SPECS/mlnx-nfsrdma/mlnx-nfsrdma.spec
@@ -26,7 +26,7 @@
 #
 #
 
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/SPECS/mlnx-ofa_kernel/mlnx-ofa_kernel.spec b/SPECS/mlnx-ofa_kernel/mlnx-ofa_kernel.spec
@@ -25,7 +25,7 @@
 # and/or other materials provided with the distribution.
 #
 #
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/SPECS/srp/srp.spec b/SPECS/srp/srp.spec
@@ -26,7 +26,7 @@
 #
 #
 
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/SPECS/xpmem/xpmem.spec b/SPECS/xpmem/xpmem.spec
@@ -1,6 +1,6 @@
 %{!?KMP: %global KMP 0}
 
-%global last-known-kernel 6.6.82.1
+%global last-known-kernel 6.6.82.1-1
 
 %if 0%{azl}
 %global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)
diff --git a/toolkit/scripts/check_entangled_specs.py b/toolkit/scripts/check_entangled_specs.py
@@ -11,6 +11,10 @@
 
 from pyrpm.spec import replace_macros, Spec
 
+# Control output verbosity, keeping this module global since we do not
+# have a containing top-level scope
+verbose=False
+
 version_release_matching_groups = [
     frozenset([
         "SPECS-SIGNED/kernel-signed/kernel-signed.spec",
@@ -147,112 +151,75 @@
     ])
 ]
 
-def check_spec_tags(base_path: str, tags: List[str], groups: List[FrozenSet]) -> Set[FrozenSet]:
-    """Returns spec sets which violate matching rules for given tags. """
-    err_groups = set()
-    for group in groups:
-        variants = defaultdict(set)
-
-        for spec_filename in group:
-            parsed_spec = Spec.from_file(path.join(base_path, spec_filename))
-            for tag in tags:
-                tag_value = get_tag_value(parsed_spec, tag)
-                variants[tag].add(tag_value)
-
-        for tag in tags:
-            if len(variants[tag]) > 1: err_groups.add(group)
-    return err_groups
-
-
-def check_oot_kmodules(base_path: str, tag: str, groups: List[FrozenSet]) -> Set[FrozenSet]:
-    """Returns OOT kernel modules which violate matching with kernel-headers version. """
-    err_groups = set()
-
-    kernel_headers_spec = Spec.from_file(path.join(base_path, "SPECS/kernel-headers/kernel-headers.spec"))
-    kernel_headers_version = get_tag_value(kernel_headers_spec, 'version')
+def print_verbose(message: str):
+    "Print 'message' to stdout if global variable 'verbose' is true."
+    if verbose:
+        print(message)
 
+def check_spec_tags(base_path: str, tags: dict, groups: List[FrozenSet]) -> bool:
+    """Check if spec set violates matching rules for any of given tags. Return True/False accordingly."""
+    has_error = False
     for group in groups:
+        print_verbose(f"Processing group: {group}")
+        spec_tag_map = {tag: {} for tag in tags}
         for spec_filename in group:
             parsed_spec = Spec.from_file(path.join(base_path, spec_filename))
-            tag_value = get_tag_value(parsed_spec, tag)
-            if tag_value != kernel_headers_version:
-                err_groups.add(spec_filename)
-    return err_groups
-
-def check_mstflintver_match_groups(base_path: str) -> Set[FrozenSet]:
-    return check_spec_tags(base_path, ['mstflintver'], mstflintver_matching_groups)
-
-def check_sdkver_match_groups(base_path: str) -> Set[FrozenSet]:
-    return check_spec_tags(base_path, ['sdkver'], sdkver_matching_groups)
-
-def check_version_release_match_groups(base_path: str) -> Set[FrozenSet]:
-    return check_spec_tags(base_path, ['epoch', 'version', 'release'], version_release_matching_groups)
-
-def check_version_match_groups(base_path: str) -> Set[FrozenSet]:
-    return check_spec_tags(base_path, ['epoch', 'version'], version_matching_groups)
+            print_verbose(f"\t{spec_filename}")
 
-def check_oot_kmodule_matching_groups(base_path: str) -> Set[FrozenSet]:
-    return check_oot_kmodules(base_path, 'last-known-kernel', oot_kmodule_matching_groups)
+            for tag, tag_current in tags.items():
+                tag_value = get_tag_value(parsed_spec, tag)
+                spec_tag_map[tag][spec_filename] = tag_value
+                tag_want = f" (want: {tag_current})" if tag_current else ""
+                print_verbose(f"\t\ttag({tag}) value: {tag_value}{tag_want}")
+        
+        for tag, specs_values in spec_tag_map.items():
+            # Skip to next tag if tag value is unique and it matches "tag_expected_value" if set
+            value_list = list(specs_values.values())
+            tag_expected = tags[tag]
+            if len(set(value_list)) > 1 or (tag_expected and value_list[0] != tag_expected):
+                has_error = True
+                print(f'Mismatch in expected value of "{tag}":{tag_expected or ""}')
+                for spec_name, value in specs_values.items():
+                    print(f"\t{value:30} => {spec_name}")
+
+    return has_error
 
 def check_matches(base_path: str):
-    version_match_errors = check_version_match_groups(base_path)
-    version_release_match_errors = check_version_release_match_groups(base_path)
-    sdkver_match_errors = check_sdkver_match_groups(base_path)
-    mstflintver_match_errors = check_mstflintver_match_groups(base_path)
-    oot_kmodule_match_errors = check_oot_kmodule_matching_groups(base_path)
-
-    printer = pprint.PrettyPrinter()
-
-    if len(version_match_errors) or \
-       len(version_release_match_errors) or \
-       len(sdkver_match_errors) or \
-       len(mstflintver_match_errors) or \
-       len(oot_kmodule_match_errors):
-        print('The current repository state violates a spec entanglement rule!')
-
-        if len(version_match_errors):
-            print(
-                '\nPlease update the following sets of specs to have the same "Epoch" and "Version" tags:')
-            for e in version_match_errors:
-                printer.pprint(e)
-
-        if len(version_release_match_errors):
-            print(
-                '\nPlease update the following sets of specs to have the same "Epoch", "Version", and "Release" tags:')
-            for e in version_release_match_errors:
-                printer.pprint(e)
-
-        if len(sdkver_match_errors):
-            print(
-                '\nPlease update the following sets of specs to have the same "sdkver" global variables:')
-            for e in sdkver_match_errors:
-                printer.pprint(e)
-
-        if len(mstflintver_match_errors):
-            print(
-                '\nPlease update the following sets of specs to have the same "mstflintver" global variables:')
-            for e in mstflintver_match_errors:
-                printer.pprint(e)
-
-        if len(oot_kmodule_match_errors):
-            print(
-                '\nPlease update the following sets of specs to match the "last-known-kernel" global variable with kernel-headers "version":')
-            for e in oot_kmodule_match_errors:
-                printer.pprint(e)
-
+    kernel_headers_spec = Spec.from_file(path.join(base_path, "SPECS/kernel-headers/kernel-headers.spec"))
+    kernel_headers_version = get_tag_value(kernel_headers_spec, 'version')
+    kernel_headers_release = get_tag_value(kernel_headers_spec, 'release')
+    kernel_version_release = f"{kernel_headers_version}-{kernel_headers_release}"
+    
+    groups_to_check = [({'mstflintver':{}}, mstflintver_matching_groups),
+                       ({'sdkver':{}}, sdkver_matching_groups),
+                       ({'epoch':{}, 'version':{}, 'release':{}}, version_release_matching_groups),
+                       ({'epoch':{}, 'version':{}}, version_matching_groups),
+                       ({'last-known-kernel' : kernel_version_release}, oot_kmodule_matching_groups)]
+    
+    check_result = []
+    for check_args in groups_to_check:
+        print_verbose(f'Calling check_spec_tags with "{check_args}"')
+        check_result.append(check_spec_tags(base_path, *check_args))
+    if any(check_result):
+        print('The current repository state violates one or more spec entanglement rule!')
         sys.exit(1)
-
+    print('Repository state is consistent with spec entanglement rules.')
 
 def get_tag_value(spec: "Spec", tag: str) -> str:
     value = getattr(spec, tag)
     if value:
         value = replace_macros(value, spec)
     return value
 
+def main():
+    global verbose
 
-if __name__ == '__main__':
     parser = argparse.ArgumentParser()
-    parser.add_argument(
-        'repo_root', help='path to the root of the Azure Linux repository')
+    parser.add_argument('repo_root', help='path to the root of the Azure Linux repository')
+    parser.add_argument ("--verbose", action="store_true", help='Print details about each action')
     args = parser.parse_args()
+    verbose = args.verbose
     check_matches(args.repo_root)
+
+if __name__ == '__main__':
+    main()
diff --git a/toolkit/scripts/tests/test_check_entangled_specs.py b/toolkit/scripts/tests/test_check_entangled_specs.py
@@ -0,0 +1,94 @@
+#!/usr/bin/env python3
+# Copyright (c) Microsoft Corporation.
+# Licensed under the MIT License.
+
+import unittest
+from unittest.mock import patch, MagicMock
+import check_entangled_specs
+
+# Enable verbose output to make debugging easier
+check_entangled_specs.verbose = True
+
+class TestCheckSpecTags(unittest.TestCase):
+
+    @patch('check_entangled_specs.Spec.from_file')
+    @patch('check_entangled_specs.get_tag_value')
+    def test_check_spec_tags_no_errors(self, mock_get_tag_value, mock_from_file):
+        mock_get_tag_value.side_effect = ["v1", "r1", "v1", "r1",
+                                          "v1.1", "r1.1", "v1.1", "r1.1"]
+        mock_from_file.return_value = MagicMock()
+
+        base_path = "/fake/path"
+        tags = {"version":{}, "release":{}}
+        groups = [frozenset(["spec1.spec", "spec2.spec"]),
+                  frozenset(["spec3.spec", "spec4.spec"])]
+
+        result = check_entangled_specs.check_spec_tags(base_path, tags, groups)
+        self.assertFalse(result)
+
+    @patch('check_entangled_specs.Spec.from_file')
+    @patch('check_entangled_specs.get_tag_value')
+    def test_check_spec_tags_with_errors(self, mock_get_tag_value, mock_from_file):
+        mock_get_tag_value.side_effect = ["v2", "r2", "v2.1", "r2.1"]
+        mock_from_file.return_value = MagicMock()
+
+        base_path = "/fake/path"
+        tags = {"version":"", "release":""}
+        groups = [frozenset(["spec5.spec", "spec6.spec"])]
+
+        result = check_entangled_specs.check_spec_tags(base_path, tags, groups)
+        self.assertTrue(result)
+
+    @patch('check_entangled_specs.Spec.from_file')
+    @patch('check_entangled_specs.get_tag_value')
+    def test_check_spec_tags_with_expected_value(self, mock_get_tag_value, mock_from_file):
+        mock_get_tag_value.side_effect = ["v3","v3"]
+        mock_from_file.return_value = MagicMock()
+
+        base_path = "/fake/path"
+        tags = {"version":"v3"}
+        groups = [frozenset(["spec7.spec", "spec8.spec"])]
+
+        result = check_entangled_specs.check_spec_tags(base_path, tags, groups)
+        self.assertFalse(result)
+
+    @patch('check_entangled_specs.Spec.from_file')
+    @patch('check_entangled_specs.get_tag_value')
+    def test_check_spec_tags_with_mismatched_expected_value(self, mock_get_tag_value, mock_from_file):
+        mock_get_tag_value.side_effect = ["v4","v4"]
+        mock_from_file.return_value = MagicMock()
+
+        base_path = "/fake/path"
+        tags = {"version":"v5"}
+        groups = [frozenset(["spec1.spec", "spec2.spec"])]
+
+        result = check_entangled_specs.check_spec_tags(base_path, tags, groups)
+        self.assertTrue(result)
+
+class TestCheckMatches(unittest.TestCase):
+    @patch('check_entangled_specs.Spec.from_file')
+    @patch('check_entangled_specs.get_tag_value')
+    @patch('check_entangled_specs.check_spec_tags')
+    @patch('sys.exit')
+    def test_check_matches_no_errors(self, mock_exit, mock_check_spec_tags, mock_get_tag_value, mock_from_file):
+        mock_get_tag_value.side_effect = ["1.0", "1"]
+        mock_from_file.return_value = MagicMock()
+        mock_check_spec_tags.side_effect = [ False, False, False, False, False ]
+        base_path = "/fake/path"
+        check_entangled_specs.check_matches(base_path)
+        mock_exit.assert_not_called()
+
+    @patch('check_entangled_specs.Spec.from_file')
+    @patch('check_entangled_specs.get_tag_value')
+    @patch('check_entangled_specs.check_spec_tags')
+    @patch('sys.exit')
+    def test_check_matches_with_errors(self, mock_exit, mock_check_spec_tags, mock_get_tag_value, mock_from_file):
+        mock_get_tag_value.side_effect = ["1.0", "1"]
+        mock_from_file.return_value = MagicMock()
+        mock_check_spec_tags.side_effect = [False, False, True, False, False]
+        base_path = "/fake/path"
+        check_entangled_specs.check_matches(base_path)
+        mock_exit.assert_called_once_with(1)
+
+if __name__ == '__main__':
+    unittest.main()

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@`
`30`	`30`	`# SOFTWARE.`
`31`	`31`	`#`
`32`	`32`
`33`		`-%global last-known-kernel 6.6.82.1`
	`33`	`+%global last-known-kernel 6.6.82.1-1`
`34`	`34`
`35`	`35`	`%{!?_name: %define _name fwctl}`
`36`	`36`	`%{!?_version: %define _version 24.10}`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@`
`26`	`26`	`#`
`27`	`27`	`#`
`28`	`28`
`29`		`-%global last-known-kernel 6.6.82.1`
	`29`	`+%global last-known-kernel 6.6.82.1-1`
`30`	`30`
`31`	`31`	`%if 0%{azl}`
`32`	`32`	`%global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-%global last-known-kernel 6.6.82.1`
	`1`	`+%global last-known-kernel 6.6.82.1-1`
`2`	`2`
`3`	`3`	`%if 0%{azl}`
`4`	`4`	`%global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@`
`25`	`25`	`# and/or other materials provided with the distribution.`
`26`	`26`	`#`
`27`	`27`	`#`
`28`		`-%global last-known-kernel 6.6.82.1`
	`28`	`+%global last-known-kernel 6.6.82.1-1`
`29`	`29`
`30`	`30`	`%if 0%{azl}`
`31`	`31`	`%global target_kernel_version_full %(/bin/rpm -q --queryformat '%{VERSION}-%{RELEASE}' kernel-headers)`