Distinguish semantic and formatting changes in modules

.github/workflows/tests.yml

@@ -65,7 +65,9 @@ jobs:
           export VIRTUAL_ENV="$PWD"
           export TESTS_RESOURCES_DIR="$PWD"/tests/resources
           export YANGCATALOG_CONFIG_PATH="$TESTS_RESOURCES_DIR"/test.conf
+          export PYANG_EXEC=$(which pyang)
           sed -i "s|<TESTS_RESOURCES_DIR>|${TESTS_RESOURCES_DIR}|g" "$YANGCATALOG_CONFIG_PATH"
+          sed -i "s|<PYANG_EXEC>|${PYANG_EXEC}|g" "$YANGCATALOG_CONFIG_PATH"
           coverage run -am pytest
           coverage xml
 

Dockerfile

@@ -22,32 +22,8 @@ ENV GIT_PYTHON_GIT_EXECUTABLE=/usr/bin/git
 ENV VIRTUAL_ENV=/module-compilation
 ENV PYTHONPATH=$VIRTUAL_ENV
 ENV CONF=$VIRTUAL_ENV/conf
-ENV YANGVAR="get_config.py --section Directory-Section --key var"
-ENV BACKUPDIR="get_config.py --section Directory-Section --key backup"
-ENV CONFD_DIR="get_config.py --section Tool-Section --key confd-dir"
-ENV PYANG="get_config.py --section Tool-Section --key pyang-exec"
 ENV PYANG_PLUGINPATH="/module-compilation/utility/pyang_plugin"
-ENV IS_PROD="get_config.py --section General-Section --key is-prod"
-
-#
-# Repositories
-#
-ENV NONIETFDIR="get_config.py --section Directory-Section --key non-ietf-directory"
-ENV IETFDIR="get_config.py --section Directory-Section --key ietf-directory"
-ENV MODULES="get_config.py --section Directory-Section --key modules-directory"
-
-#
-# Working directories
-#
-ENV LOGS="get_config.py --section Directory-Section --key logs"
-ENV TMP="get_config.py --section Directory-Section --key temp"
-
-#
-# Where the HTML pages lie
-#
-ENV WEB_PRIVATE="get_config.py --section Web-Section --key private-directory"
-ENV WEB_DOWNLOADABLES="get_config.py --section Web-Section --key downloadables-directory"
-ENV WEB="get_config.py --section Web-Section --key public-directory"
+
 
 RUN groupadd -g ${YANG_GID} -r yang && useradd --no-log-init -r -g yang -u ${YANG_ID} -d $VIRTUAL_ENV yang
 

modules_compilation/compile_modules.py

@@ -73,9 +73,7 @@ class Options:
     @dataclass
     class ModuleInfoForCompilation:
         yang_file_path: str
-        module_hash: str
-        module_hash_changed: bool
-        changed_validator_versions: t.Optional[list[str]]
+        module_hash_info: FileHasher.ModuleHashCheckForParsing
         yang_file_compilation_data: t.Optional[dict]
         previous_compilation_results: t.Optional[dict]
 
@@ -175,15 +173,20 @@ def _compile_modules(self) -> dict:
             module_info_for_compilation = self._get_module_info_for_compilation(yang_file_path, file_name_and_revision)
             yang_file_path = module_info_for_compilation.yang_file_path
             yang_file_compilation_data = module_info_for_compilation.yang_file_compilation_data
+            module_hash_info = module_info_for_compilation.module_hash_info
+            changed_validator_versions = module_hash_info.get_changed_validator_versions(
+                self.validator_versions,
+            )
+            module_content_changed = module_hash_info.hash_changed and not module_hash_info.only_formatting_changed
             if (
                 not module_info_for_compilation.previous_compilation_results
-                or module_info_for_compilation.module_hash_changed
-                or module_info_for_compilation.changed_validator_versions
+                or module_content_changed
+                or changed_validator_versions
             ):
                 parsers_to_use, module_compilation_results = self._get_parsers_to_use_and_previous_compilation_results(
                     module_info_for_compilation.previous_compilation_results,
-                    module_info_for_compilation.module_hash_changed,
-                    module_info_for_compilation.changed_validator_versions,
+                    module_content_changed,
+                    changed_validator_versions,
                 )
                 compilation_status, module_compilation_results = self._parse_module(
                     parsers_to_use,
@@ -210,9 +213,19 @@ def _compile_modules(self) -> dict:
                 # Revert to previous hash if compilation status is 'UNKNOWN' -> try to parse model again next time
                 if compilation_status != 'UNKNOWN':
                     self.file_hasher.updated_hashes[yang_file_path] = {
-                        'hash': module_info_for_compilation.module_hash,
+                        'hash': module_hash_info.hash,
                         'validator_versions': self.validator_versions,
+                        'normalized_file_hash': module_hash_info.normalized_file_hash,
                     }
+            elif (module_hash_info.hash_changed and module_hash_info.only_formatting_changed) or (
+                not module_hash_info.hash_changed
+                and not self.file_hasher.files_hashes.get(yang_file_path, {}).get('normalized_file_hash')
+            ):
+                self.file_hasher.updated_hashes[yang_file_path] = {
+                    'hash': module_hash_info.hash,
+                    'validator_versions': self.validator_versions,
+                    'normalized_file_hash': module_hash_info.normalized_file_hash,
+                }
             aggregated_results['all'][file_name_and_revision] = yang_file_compilation_data
             if module_or_submodule(yang_file_path) == 'module':
                 aggregated_results['no_submodules'][file_name_and_revision] = yang_file_compilation_data
@@ -224,35 +237,35 @@ def _get_module_info_for_compilation(
         file_name_and_revision: str,
     ) -> ModuleInfoForCompilation:
         all_modules_dir_yang_file_path = os.path.join(self.all_modules_dir, file_name_and_revision)
-        all_modules_dir_yang_file_hash_info = (
-            self.file_hasher.should_parse(all_modules_dir_yang_file_path)
-            if os.path.exists(all_modules_dir_yang_file_path)
-            else None
-        )
         yang_file_compilation_data = self.cached_compilation_results.get(file_name_and_revision, {})
         module_hash_info = self.file_hasher.should_parse(yang_file_path)
-        if all_modules_dir_yang_file_hash_info:
+        if os.path.exists(all_modules_dir_yang_file_path):
             if yang_file_compilation_data.get('yang_file_path') == all_modules_dir_yang_file_path:
-                # the file has been already re-compiled with the path in all_modules_dir
+                # the file has been already re-compiled with the path in all_modules_dir,
                 # so this path is the right one for this file compilation
                 yang_file_path = all_modules_dir_yang_file_path
-                module_hash_info = all_modules_dir_yang_file_hash_info
-            elif module_hash_info.hash != all_modules_dir_yang_file_hash_info.hash:
-                # the file in yang_file_path isn't the right one
-                # and should be re-compiled with the path in all_modules_dir
+                module_hash_info = self.file_hasher.should_parse(all_modules_dir_yang_file_path)
+            elif (
+                all_modules_dir_yang_file_hash := self.file_hasher.hash_file(all_modules_dir_yang_file_path)
+            ) != module_hash_info.hash:
+                # the file in yang_file_path has different content with the file in all_modules_dir,
+                # and should be re-compiled with the path in all_modules_dir. Normalized hashes aren't being compared
+                # in this situation to avoid such a case when the hashes of the yang_file_path and
+                # all_modules_dir_yang_file_path are different, but they are equivalent in the normalized form,
+                # and the hash of the all_modules_dir_yang_file_path in the normalized form is calculated everytime,
+                # so it's better to just use the all_modules_dir_yang_file_path and re-compile the module
                 return self.ModuleInfoForCompilation(
                     yang_file_path=all_modules_dir_yang_file_path,
-                    module_hash=all_modules_dir_yang_file_hash_info.hash,
-                    module_hash_changed=True,
-                    changed_validator_versions=None,
+                    module_hash_info=self.file_hasher.should_parse(
+                        all_modules_dir_yang_file_path,
+                        already_calculated_hash=all_modules_dir_yang_file_hash,
+                    ),
                     yang_file_compilation_data=None,
                     previous_compilation_results=None,
                 )
         return self.ModuleInfoForCompilation(
             yang_file_path=yang_file_path,
-            module_hash=module_hash_info.hash,
-            module_hash_changed=module_hash_info.hash_changed,
-            changed_validator_versions=module_hash_info.get_changed_validator_versions(self.validator_versions),
+            module_hash_info=module_hash_info,
             yang_file_compilation_data=yang_file_compilation_data,
             previous_compilation_results=(
                 yang_file_compilation_data.get('compilation_results')
@@ -320,10 +333,10 @@ def _get_mod_rev(self, yang_file) -> str:
     def _get_parsers_to_use_and_previous_compilation_results(
         self,
         previous_compilation_results: t.Optional[dict],
-        module_hash_changed: bool,
+        module_content_changed: bool,
         changed_validator_versions: t.Optional[list[str]],
     ) -> tuple[dict, dict]:
-        if previous_compilation_results and not module_hash_changed and changed_validator_versions:
+        if previous_compilation_results and not module_content_changed and changed_validator_versions:
             parsers_to_use = {
                 parser_name: parser_object
                 for parser_name, parser_object in self.parsers.items()

modules_compilation/file_hasher.py

@@ -29,6 +29,7 @@
 import hashlib
 import json
 import os.path
+import typing as t
 from configparser import ConfigParser
 from dataclasses import dataclass
 
@@ -41,14 +42,15 @@
 
 class FileHasher:
     def __init__(self, dst_dir: str = '', force_compilation: bool = False, config: ConfigParser = create_config()):
+        self.config = config
         self.cache_dir = config.get('Directory-Section', 'cache')
         self.force_compilation = force_compilation
         self.files_hashes = self._load_hashed_files_list(dst_dir)
         self.updated_hashes = {}
 
     def hash_file(self, path: str) -> str:
         """
-        Create hash from content of the given file. Each time the content of the file change,
+        Create hash from content of the given file. Each time the content of the file changes,
         the resulting hash will be different.
 
         Arguments:
@@ -111,8 +113,10 @@ def dump_hashed_files_list(self, dst_dir: str = ''):
     @dataclass
     class ModuleHashCheckForParsing:
         hash_changed: bool
+        only_formatting_changed: bool
         hash: str
         validator_versions: dict
+        normalized_file_hash: str
 
         def get_changed_validator_versions(self, validators_to_check: dict) -> list[str]:
             changed_validators = []
@@ -122,20 +126,62 @@ def get_changed_validator_versions(self, validators_to_check: dict) -> list[str]
                 changed_validators.append(validator)
             return changed_validators
 
-    def should_parse(self, path: str) -> ModuleHashCheckForParsing:
+    def should_parse(self, path: str, already_calculated_hash: t.Optional[str] = None) -> ModuleHashCheckForParsing:
         """
         Decide whether module at the given path should be parsed or not.
         Check whether file content hash has changed and keep it for the future use.
 
         Argument:
-            :param path     (str) Full path to the file to be hashed
+            :param path (str) Full path to the file to check hash.
+            :param already_calculated_hash (Optional[str]) Already calculated hash of the path, can be used if the hash
+            has been calculated before calling this method in order to not re-calculate the hash to improve performance,
+            be careful passing this argument, to not pass an incorrect hash.
         """
-        file_hash = self.hash_file(path)
+        file_hash = already_calculated_hash if already_calculated_hash else self.hash_file(path)
         old_file_hash_info = self.files_hashes.get(path)
         if not old_file_hash_info or not isinstance(old_file_hash_info, dict):
-            return self.ModuleHashCheckForParsing(hash_changed=True, hash=file_hash, validator_versions={})
+            return self.ModuleHashCheckForParsing(
+                hash_changed=True,
+                only_formatting_changed=False,
+                hash=file_hash,
+                validator_versions={},
+                normalized_file_hash=self.get_normalized_file_hash(path),
+            )
+        hash_changed = old_file_hash_info['hash'] != file_hash
+        old_normalized_file_hash = old_file_hash_info.get('normalized_file_hash')
+        new_normalized_file_hash = None
+        only_formatting_changed = not hash_changed
+        if (
+            hash_changed
+            and old_normalized_file_hash
+            and (new_normalized_file_hash := self.get_normalized_file_hash(path)) == old_normalized_file_hash
+        ):
+            only_formatting_changed = True
         return self.ModuleHashCheckForParsing(
-            hash_changed=self.force_compilation or old_file_hash_info['hash'] != file_hash,
+            hash_changed=self.force_compilation or hash_changed,
+            only_formatting_changed=False if self.force_compilation else only_formatting_changed,
             hash=file_hash,
             validator_versions=old_file_hash_info['validator_versions'],
+            normalized_file_hash=(
+                old_normalized_file_hash
+                if (not hash_changed and old_normalized_file_hash) or (hash_changed and only_formatting_changed)
+                else new_normalized_file_hash
+                if new_normalized_file_hash
+                else self.get_normalized_file_hash(path)
+            ),
         )
+
+    def get_normalized_file_hash(self, path: str) -> str:
+        tmp_file_path = os.path.join(self.config.get('Directory-Section', 'temp'), os.path.basename(path))
+        pyang_exec = self.config.get('Tool-Section', 'pyang-exec')
+        with os.popen(
+            (
+                f'python3 {pyang_exec} -f yang -p {os.path.dirname(path)} --yang-canonical --yang-remove-comments '
+                f'{path}'  # TODO: --yang-join-substrings option should be added when available in pyang
+            ),
+        ) as normalized_file, open(tmp_file_path, 'w') as tmp_file:
+            tmp_file.write(normalized_file.read())
+        del normalized_file
+        normalized_file_hash = self.hash_file(tmp_file_path)
+        os.remove(tmp_file_path)
+        return normalized_file_hash

tests/README.md

@@ -4,7 +4,7 @@
 
 ## HOW TO RUN
 - Attach to the ```yc-module-compilation``` container
-- Set all the needed environment variables using: ```export PYTHONPATH="$PWD":$PYTHONPATH && export VIRTUAL_ENV="$PWD" && export TESTS_RESOURCES_DIR="$PWD"/tests/resources && export YANGCATALOG_CONFIG_PATH="$TESTS_RESOURCES_DIR"/test.conf && sed -i "s|<TESTS_RESOURCES_DIR>|${TESTS_RESOURCES_DIR}|g" "$YANGCATALOG_CONFIG_PATH"```
+- Set all the needed environment variables using: ```export PYTHONPATH="$PWD":$PYTHONPATH && export VIRTUAL_ENV="$PWD" && export TESTS_RESOURCES_DIR="$PWD"/tests/resources && export YANGCATALOG_CONFIG_PATH="$TESTS_RESOURCES_DIR"/test.conf && export PYANG_EXEC=$(which pyang) && sed -i "s|<TESTS_RESOURCES_DIR>|${TESTS_RESOURCES_DIR}|g" "$YANGCATALOG_CONFIG_PATH" && sed -i "s|<PYANG_EXEC>|${PYANG_EXEC}|g" "$YANGCATALOG_CONFIG_PATH"```
 - Now you're able to run all the tests locally:
   - To run all the tests: ```pytest```
   - To run the tests in a particular file: ```pytest tests/test_file_name.py```

tests/resources/cache/draft_dict.json

@@ -1,7 +1,5 @@
 {
   "[email protected]": "draft-ietf-idr-bgp-model-16.txt",
   "[email protected]": "draft-palmero-opsawg-dmlmo-09.txt",
-  "[email protected]": "draft-ietf-netmod-acl-extensions-01.txt",
-  "[email protected]": "draft-ietf-rtgwg-qos-model-10.txt",
-  "[email protected]": "draft-ietf-netconf-ssh-client-server-32.txt"
+  "[email protected]": "draft-ietf-netmod-acl-extensions-01.txt"
 }

tests/resources/cache/example_dict.json

@@ -1,7 +1,5 @@
 {
   "example-application.yang": "test_rfc_document_name.txt",
   "[email protected]": "test_rfc_document_name.txt",
-  "[email protected]": "test_rfc_document_name.txt",
-  "[email protected]": "test_rfc_document_name.txt",
-  "[email protected]": "test_rfc_document_name.txt"
+  "[email protected]": "test_rfc_document_name.txt"
 }

tests/resources/cache/rfc_dict.json

@@ -1,7 +1,5 @@
 {
   "[email protected]": "test_rfc_document_name.txt",
   "[email protected]": "test_rfc_document_name.txt",
-  "[email protected]": "test_rfc_document_name.txt",
-  "[email protected]": "test_rfc_document_name.txt",
-  "[email protected]": "test_rfc_document_name.txt"
+  "[email protected]": "test_rfc_document_name.txt"
 }