Merge branch 'fix/harden_hints_parsing_v5.1' into 'release/v5.1'

fix: harden input parsing in component_requirements hint module (v5.1)

See merge request espressif/esp-idf!28711

tools/cmake/project.cmake

@@ -119,7 +119,7 @@ function(paths_with_spaces_to_list variable_name)
     endif()
 endfunction()
 
-function(__component_info components output)
+function(__build_component_info components output)
     set(components_json "")
     foreach(name ${components})
         __component_get_target(target ${name})
@@ -187,6 +187,62 @@ function(__component_info components output)
     set(${output} "${components_json}" PARENT_SCOPE)
 endfunction()
 
+function(__all_component_info output)
+    set(components_json "")
+
+    idf_build_get_property(build_prefix __PREFIX)
+    idf_build_get_property(component_targets __COMPONENT_TARGETS)
+
+    foreach(target ${component_targets})
+        __component_get_property(name ${target} COMPONENT_NAME)
+        __component_get_property(alias ${target} COMPONENT_ALIAS)
+        __component_get_property(prefix ${target} __PREFIX)
+        __component_get_property(dir ${target} COMPONENT_DIR)
+        __component_get_property(type ${target} COMPONENT_TYPE)
+        __component_get_property(lib ${target} COMPONENT_LIB)
+        __component_get_property(reqs ${target} REQUIRES)
+        __component_get_property(include_dirs ${target} INCLUDE_DIRS)
+        __component_get_property(priv_reqs ${target} PRIV_REQUIRES)
+        __component_get_property(managed_reqs ${target} MANAGED_REQUIRES)
+        __component_get_property(managed_priv_reqs ${target} MANAGED_PRIV_REQUIRES)
+
+        if(prefix STREQUAL build_prefix)
+            set(name ${name})
+        else()
+            set(name ${alias})
+        endif()
+
+        make_json_list("${reqs}" reqs)
+        make_json_list("${priv_reqs}" priv_reqs)
+        make_json_list("${managed_reqs}" managed_reqs)
+        make_json_list("${managed_priv_reqs}" managed_priv_reqs)
+        make_json_list("${include_dirs}" include_dirs)
+
+        string(JOIN "\n" component_json
+            "        \"${name}\": {"
+            "            \"alias\": \"${alias}\","
+            "            \"target\": \"${target}\","
+            "            \"prefix\": \"${prefix}\","
+            "            \"dir\": \"${dir}\","
+            "            \"lib\": \"${lib}\","
+            "            \"reqs\": ${reqs},"
+            "            \"priv_reqs\": ${priv_reqs},"
+            "            \"managed_reqs\": ${managed_reqs},"
+            "            \"managed_priv_reqs\": ${managed_priv_reqs},"
+            "            \"include_dirs\": ${include_dirs}"
+            "        }"
+        )
+        string(CONFIGURE "${component_json}" component_json)
+        if(NOT "${components_json}" STREQUAL "")
+            string(APPEND components_json ",\n")
+        endif()
+        string(APPEND components_json "${component_json}")
+    endforeach()
+    string(PREPEND components_json "{\n")
+    string(APPEND components_json "\n    }")
+    set(${output} "${components_json}" PARENT_SCOPE)
+endfunction()
+
 #
 # Output the built components to the user. Generates files for invoking esp_idf_monitor
 # that doubles as an overview of some of the more important build properties.
@@ -251,7 +307,8 @@ function(__project_info test_components)
     make_json_list("${build_component_paths};${test_component_paths}" build_component_paths_json)
     make_json_list("${common_component_reqs}" common_component_reqs_json)
 
-    __component_info("${build_components};${test_components}" build_component_info_json)
+    __build_component_info("${build_components};${test_components}" build_component_info_json)
+    __all_component_info(all_component_info_json)
 
     # The configure_file function doesn't process generator expressions, which are needed
     # e.g. to get component target library(TARGET_LINKER_FILE), so the project_description

tools/cmake/project_description.json.in

@@ -1,5 +1,5 @@
 {
-    "version":            "1",
+    "version":            "1.1",
     "project_name":       "${PROJECT_NAME}",
     "project_version":    "${PROJECT_VER}",
     "project_path":       "${PROJECT_PATH}",
@@ -28,5 +28,6 @@
     "build_components" : ${build_components_json},
     "build_component_paths" : ${build_component_paths_json},
     "build_component_info" : ${build_component_info_json},
+    "all_component_info" : ${all_component_info_json},
     "debug_prefix_map_gdbinit": "${debug_prefix_map_gdbinit}"
 }

tools/cmake/scripts/component_get_requirements.cmake

@@ -76,6 +76,7 @@ macro(idf_component_register)
     set(__component_requires "${__REQUIRES}")
     set(__component_kconfig "${__KCONFIG}")
     set(__component_kconfig_projbuild "${__KCONFIG_PROJBUILD}")
+    set(__component_include_dirs "${__INCLUDE_DIRS}")
     set(__component_registered 1)
     return()
 endmacro()
@@ -107,11 +108,13 @@ function(__component_get_requirements)
 
     spaces2list(__component_requires)
     spaces2list(__component_priv_requires)
+    spaces2list(__component_include_dirs)
 
     set(__component_requires "${__component_requires}" PARENT_SCOPE)
     set(__component_priv_requires "${__component_priv_requires}" PARENT_SCOPE)
     set(__component_kconfig "${__component_kconfig}" PARENT_SCOPE)
     set(__component_kconfig_projbuild "${__component_kconfig_projbuild}" PARENT_SCOPE)
+    set(__component_include_dirs "${__component_include_dirs}" PARENT_SCOPE)
     set(__component_registered ${__component_registered} PARENT_SCOPE)
 endfunction()
 
@@ -141,7 +144,8 @@ foreach(__component_target ${__component_targets})
     set(__contents
 "__component_set_property(${__component_target} REQUIRES \"${__component_requires}\")
 __component_set_property(${__component_target} PRIV_REQUIRES \"${__component_priv_requires}\")
-__component_set_property(${__component_target} __COMPONENT_REGISTERED ${__component_registered})"
+__component_set_property(${__component_target} __COMPONENT_REGISTERED ${__component_registered})
+__component_set_property(${__component_target} INCLUDE_DIRS \"${__component_include_dirs}\")"
     )
 
     if(__component_kconfig)

tools/idf_py_actions/hint_modules/component_requirements.py

@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD
+# SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD
 # SPDX-License-Identifier: Apache-2.0
 import os
 import re
@@ -27,6 +27,10 @@ ENOENT_FULL_RE = re.compile(r'^(In file included.*No such file or directory)$',
 ORIGINAL_FILE_RE = re.compile(r'.*from (.*):[\d]+:')
 
 
+def _bug(msg: str) -> str:
+    return f'BUG: {os.path.basename(__file__)}: {msg}'
+
+
 def _get_absolute_path(filename: str, base: str) -> str:
     # If filename path is relative, return absolute path based
     # on base directory. The filename is normalized in any case.
@@ -58,48 +62,58 @@ def generate_hint(output: str) -> Optional[str]:
     # find the source_component that contains the source file
     found_source_component_name = None
     found_source_component_info = None
-    for component_name, component_info in proj_desc['build_component_info'].items():
+    for component_name, component_info in proj_desc['all_component_info'].items():
         # look if the source_filename is within a component directory, not only
         # at component_info['sources'], because the missing file may be included
         # from header file, which is not present in component_info['sources']
         component_dir = os.path.normpath(component_info['dir'])
         if source_filename.startswith(component_dir):
-            found_source_component_name = component_name
+            if found_source_component_info and len(found_source_component_info['dir']) >= len(component_dir):
+                continue
             found_source_component_info = component_info
-            break
+            found_source_component_name = component_name
     if not found_source_component_name:
         # The source file is not in any component.
         # It could be in a subproject added via ExternalProject_Add, in which case
         # we can't help much.
         return None
 
-    # find the original_component, which may be different from sourc_component
+    # find the original_component, which may be different from source_component
     found_original_component_name = found_source_component_name
     found_original_component_info = found_source_component_info
     original_filename = source_filename
     hint_match_full = ENOENT_FULL_RE.search(output)
     if hint_match_full:
-        lines = hint_match_full.group().splitlines()
+        # As a precaution remove empty lines, but there should be none.
+        lines = [line for line in hint_match_full.group().splitlines() if line]
         # second line from the end contains filename which is part of the
         # original_component
         original_file_match = ORIGINAL_FILE_RE.match(lines[-2])
         if original_file_match:
             original_filename = _get_absolute_path(original_file_match.group(1), proj_desc['build_dir'])
-            for component_name, component_info in proj_desc['build_component_info'].items():
+            for component_name, component_info in proj_desc['all_component_info'].items():
                 component_dir = os.path.normpath(component_info['dir'])
                 if original_filename.startswith(component_dir):
                     found_original_component_name = component_name
                     found_original_component_info = component_info
                     break
+            else:
+                # Original component not found. This should never happen, because the
+                # original_filename has to part of some component, which was added to
+                # the build.
+                return _bug((f'cannot find original component for source '
+                             f'component {found_source_component_name}'))
+
         else:
             # We should never reach this path. It would probably mean
-            # the preprocessor output was changed. Anyway we can still
+            # the preprocessor output was changed or we received malformed
-            # report something meaningful, so just keep going.
+            # output.
-            pass
+            return _bug((f'cannot match original component filename for '
+                         f'source component {found_source_component_name}'))
 
     # look for the header file in the public include directories of all components
     found_dep_component_names = []
-    for candidate_component_name, candidate_component_info in proj_desc['build_component_info'].items():
+    for candidate_component_name, candidate_component_info in proj_desc['all_component_info'].items():
         if candidate_component_name == found_source_component_name:
             # skip the component that contains the source file
             continue
@@ -116,7 +130,7 @@ def generate_hint(output: str) -> Optional[str]:
         # directories if we can find the missing header there and notify user about possible
         # missing entry in INCLUDE_DIRS.
         candidate_component_include_dirs = []
-        for component_name, component_info in proj_desc['build_component_info'].items():
+        for component_name, component_info in proj_desc['all_component_info'].items():
             component_dir = os.path.normpath(component_info['dir'])
             for root, _, _ in os.walk(component_dir):
                 full_path = os.path.normpath(os.path.join(root, missing_header))
@@ -153,7 +167,8 @@ def generate_hint(output: str) -> Optional[str]:
         if dep_component_name in all_reqs:
             # Oops. This component is already in the requirements list.
             # How did this happen?
-            return f'BUG: {missing_header} found in component {dep_component_name} which is already in the requirements list of {found_source_component_name}'
+            return _bug((f'{missing_header} found in component {dep_component_name} '
+                         f'which is already in the requirements list of {found_source_component_name}'))
 
     # try to figure out the correct require type: REQUIRES or PRIV_REQUIRES
     requires_type = None

tools/test_idf_py/test_hints.py

@@ -48,16 +48,17 @@ class TestHintsMassages(unittest.TestCase):
         self.tmpdir.cleanup()
 
 
-class TestHintModuleComponentRequirements(unittest.TestCase):
+def run_idf(args: List[str], cwd: Path) -> str:
-    def run_idf(self, args: List[str]) -> str:
+    # Simple helper to run idf command and return it's stdout.
-        # Simple helper to run idf command and return it's stdout.
+    cmd = [
-        cmd = [
+        sys.executable,
-            sys.executable,
+        os.path.join(os.environ['IDF_PATH'], 'tools', 'idf.py')
-            os.path.join(os.environ['IDF_PATH'], 'tools', 'idf.py')
+    ]
-        ]
+    proc = run(cmd + args, capture_output=True, cwd=cwd, text=True)
-        proc = run(cmd + args, capture_output=True, cwd=str(self.projectdir), text=True)
+    return str(proc.stdout + proc.stderr)
-        return proc.stdout + proc.stderr
 
+
+class TestHintModuleComponentRequirements(unittest.TestCase):
     def setUp(self) -> None:
         # Set up a dummy project in tmp directory with main and component1 component.
         # The main component includes component1.h from component1, but the header dir is
@@ -87,44 +88,117 @@ class TestHintModuleComponentRequirements(unittest.TestCase):
         # The main component uses component1.h, but this header is not in component1 public
         # interface. Hints should suggest that component1.h should be added into INCLUDE_DIRS
         # of component1.
-        output = self.run_idf(['app'])
+        output = run_idf(['app'], self.projectdir)
         self.assertIn('Missing "component1.h" file name found in the following component(s): component1(', output)
 
         # Based on previous hint the component1.h is added to INCLUDE_DIRS, but main still doesn't
         # have dependency on compoment1. Hints should suggest to add component1 into main component
         # PRIV_REQUIRES, because foo.h is not in main public interface.
-        self.run_idf(['fullclean'])
+        run_idf(['fullclean'], self.projectdir)
         component1cmake = self.projectdir / 'components' / 'component1' / 'CMakeLists.txt'
         component1cmake.write_text('idf_component_register(INCLUDE_DIRS ".")')
-        output = self.run_idf(['app'])
+        output = run_idf(['app'], self.projectdir)
         self.assertIn('To fix this, add component1 to PRIV_REQUIRES list of idf_component_register call', output)
 
         # Add foo.h into main public interface. Now the hint should suggest to use
         # REQUIRES instead of PRIV_REQUIRES.
-        self.run_idf(['fullclean'])
+        run_idf(['fullclean'], self.projectdir)
         maincmake = self.projectdir / 'main' / 'CMakeLists.txt'
         maincmake.write_text(('idf_component_register(SRCS "foo.c" '
                               'REQUIRES esp_timer '
                               'INCLUDE_DIRS ".")'))
-        output = self.run_idf(['app'])
+        output = run_idf(['app'], self.projectdir)
         self.assertIn('To fix this, add component1 to REQUIRES list of idf_component_register call', output)
 
         # Add component1 to REQUIRES as suggested by previous hint, but also
         # add esp_psram as private req for component1 and add esp_psram.h
-        # to component1.h. New the hint should report that esp_psram should
+        # to component1.h. Now the hint should report that esp_psram should
         # be moved from PRIV_REQUIRES to REQUIRES for component1.
-        self.run_idf(['fullclean'])
+        run_idf(['fullclean'], self.projectdir)
         maincmake.write_text(('idf_component_register(SRCS "foo.c" '
                               'REQUIRES esp_timer component1 '
                               'INCLUDE_DIRS ".")'))
         (self.projectdir / 'components' / 'component1' / 'component1.h').write_text('#include "esp_psram.h"')
         component1cmake.write_text('idf_component_register(INCLUDE_DIRS "." PRIV_REQUIRES esp_psram)')
-        output = self.run_idf(['app'])
+        output = run_idf(['app'], self.projectdir)
         self.assertIn('To fix this, move esp_psram from PRIV_REQUIRES into REQUIRES', output)
 
     def tearDown(self) -> None:
         self.tmpdir.cleanup()
 
 
+class TestNestedModuleComponentRequirements(unittest.TestCase):
+    def setUp(self) -> None:
+        # Set up a nested component structure. The components directory contains
+        # component1, which also contains foo project with main component.
+        # components/component1/project/main
+        #            ^^^^^^^^^^         ^^^^
+        #            component          nested component
+        # Both components include esp_timer.h, but only component1 has esp_timer
+        # in requirements.
+        self.tmpdir = tempfile.TemporaryDirectory()
+        self.tmpdirpath = Path(self.tmpdir.name)
+
+        components = self.tmpdirpath / 'components'
+        maindir = components / 'component1'
+        maindir.mkdir(parents=True)
+        (maindir / 'CMakeLists.txt').write_text('idf_component_register(SRCS "component1.c" PRIV_REQUIRES esp_timer)')
+        (maindir / 'component1.c').write_text('#include "esp_timer.h"')
+
+        self.projectdir = maindir / 'project'
+        self.projectdir.mkdir(parents=True)
+        (self.projectdir / 'CMakeLists.txt').write_text((
+            'cmake_minimum_required(VERSION 3.16)\n'
+            f'set(EXTRA_COMPONENT_DIRS "{components.as_posix()}")\n'
+            'set(COMPONENTS main)\n'
+            'include($ENV{IDF_PATH}/tools/cmake/project.cmake)\n'
+            'project(foo)'))
+
+        maindir = self.projectdir / 'main'
+        maindir.mkdir()
+        (maindir / 'CMakeLists.txt').write_text('idf_component_register(SRCS "foo.c" REQUIRES component1)')
+        (maindir / 'foo.c').write_text('#include "esp_timer.h"\nvoid app_main(){}')
+
+    def test_nested_component_requirements(self) -> None:
+        # Verify that source component for a failed include is properly identified
+        # when components are nested. The main component should be identified as the
+        # real source, not the component1 component.
+        output = run_idf(['app'], self.projectdir)
+        self.assertNotIn('esp_timer.h found in component esp_timer which is already in the requirements list of component1', output)
+        self.assertIn('To fix this, add esp_timer to PRIV_REQUIRES list of idf_component_register call', output)
+
+    def tearDown(self) -> None:
+        self.tmpdir.cleanup()
+
+
+class TestTrimmedModuleComponentRequirements(unittest.TestCase):
+    def setUp(self) -> None:
+        # Set up a dummy project with a trimmed down list of components and main component.
+        # The main component includes "esp_http_client.h", but the esp_http_client
+        # component is not added to main's requirements.
+        self.tmpdir = tempfile.TemporaryDirectory()
+        self.tmpdirpath = Path(self.tmpdir.name)
+
+        self.projectdir = self.tmpdirpath / 'project'
+        self.projectdir.mkdir(parents=True)
+        (self.projectdir / 'CMakeLists.txt').write_text((
+            'cmake_minimum_required(VERSION 3.16)\n'
+            'set(COMPONENTS main)\n'
+            'include($ENV{IDF_PATH}/tools/cmake/project.cmake)\n'
+            'project(foo)'))
+
+        maindir = self.projectdir / 'main'
+        maindir.mkdir()
+        (maindir / 'CMakeLists.txt').write_text('idf_component_register(SRCS "foo.c")')
+        (maindir / 'foo.c').write_text('#include "esp_http_client.h"\nvoid app_main(){}')
+
+    def test_trimmed_component_requirements(self) -> None:
+        output = run_idf(['app'], self.projectdir)
+        self.assertIn('To fix this, add esp_http_client to PRIV_REQUIRES list of idf_component_register call in', output)
+
+    def tearDown(self) -> None:
+        self.tmpdir.cleanup()
+
+
 if __name__ == '__main__':
     unittest.main()