From 17aa60886cd2dafd67d852311c9d4640046aeb00 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Wed, 27 Sep 2023 12:50:25 +0200 Subject: [PATCH 1/4] fix(hints): properly identify source component If there is a component(child) within a component(parent), like for test_apps, the parent component may be wrongly identified as source component for the failed include. This may lead to a false bug report if the parent component has component, which provides the missing header, in requirements. Fix this by looking for the longest matching source component directory. Suggested-by: Ivan Grokhotkov Signed-off-by: Frantisek Hrbata --- .../hint_modules/component_requirements.py | 5 +- tools/test_idf_py/test_hints.py | 79 +++++++++++++++---- 2 files changed, 65 insertions(+), 19 deletions(-) diff --git a/tools/idf_py_actions/hint_modules/component_requirements.py b/tools/idf_py_actions/hint_modules/component_requirements.py index 3ce6dd9dba..ab555b9545 100644 --- a/tools/idf_py_actions/hint_modules/component_requirements.py +++ b/tools/idf_py_actions/hint_modules/component_requirements.py @@ -64,9 +64,10 @@ def generate_hint(output: str) -> Optional[str]: # 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 diff --git a/tools/test_idf_py/test_hints.py b/tools/test_idf_py/test_hints.py index d1164ff0a1..18c6092430 100755 --- a/tools/test_idf_py/test_hints.py +++ b/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(self, args: List[str]) -> str: - # Simple helper to run idf command and return it's stdout. - cmd = [ - sys.executable, - os.path.join(os.environ['IDF_PATH'], 'tools', 'idf.py') - ] - proc = run(cmd + args, capture_output=True, cwd=str(self.projectdir), text=True) - return proc.stdout + proc.stderr +def run_idf(args: List[str], cwd: Path) -> str: + # Simple helper to run idf command and return it's stdout. + cmd = [ + sys.executable, + os.path.join(os.environ['IDF_PATH'], 'tools', 'idf.py') + ] + proc = run(cmd + args, capture_output=True, cwd=cwd, text=True) + return str(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,88 @@ 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}")\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('BUG: 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() + + if __name__ == '__main__': unittest.main() From 87afd5e8292813b8e2eb328eccddd1ed1a6865aa Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Mon, 2 Oct 2023 15:31:06 +0200 Subject: [PATCH 2/4] feat(tools): export information about all components in __COMPONENT_TARGETS Add new "all_component_info" dictionary into the project_description.json file. It contains information about all registered components presented in the __COMPONENT_TARGETS list. Since components in this list are not fully evaluated, because only the first stage of cmakefiles processing is done, it does not contain the same information as the "build_component_info" dictionary. The "type", "file" and "sources" variables are missing. Most of the properties are already attached to the component target, so this only adds INCLUDE_DIRS property to the target during the first cmakefiles processing stage. The "all_component_info" dict is generated in a separate function, even though the original function for "build_component_info" could be adjusted. This introduces a little bit of boilerplate, but keeps it logically separated and probably easier if we want to extend it in the future. Signed-off-by: Frantisek Hrbata --- tools/cmake/project.cmake | 61 ++++++++++++++++++- tools/cmake/project_description.json.in | 3 +- .../scripts/component_get_requirements.cmake | 6 +- 3 files changed, 66 insertions(+), 4 deletions(-) diff --git a/tools/cmake/project.cmake b/tools/cmake/project.cmake index 97b15d2896..3a61d14c7a 100644 --- a/tools/cmake/project.cmake +++ b/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 diff --git a/tools/cmake/project_description.json.in b/tools/cmake/project_description.json.in index feee0dd845..1805db5339 100644 --- a/tools/cmake/project_description.json.in +++ b/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}" } diff --git a/tools/cmake/scripts/component_get_requirements.cmake b/tools/cmake/scripts/component_get_requirements.cmake index 21d2da8614..89531881a4 100644 --- a/tools/cmake/scripts/component_get_requirements.cmake +++ b/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) From 0fc2e770174bdf2ee1aef7aa6f57ef3b51f38a81 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Mon, 2 Oct 2023 16:24:41 +0200 Subject: [PATCH 3/4] feat(hints): use all_component_info from project_description.json Currently the component_requirements hint module does not work as expected if the component list for a project is trimmed down. With the new "all_component_info" dictionary info in project_description.json, the module can produce hints even if cmake's COMPONENTS variable is set. Signed-off-by: Frantisek Hrbata --- .../hint_modules/component_requirements.py | 8 ++--- tools/test_idf_py/test_hints.py | 29 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/tools/idf_py_actions/hint_modules/component_requirements.py b/tools/idf_py_actions/hint_modules/component_requirements.py index ab555b9545..04d1675a99 100644 --- a/tools/idf_py_actions/hint_modules/component_requirements.py +++ b/tools/idf_py_actions/hint_modules/component_requirements.py @@ -58,7 +58,7 @@ 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'] @@ -86,7 +86,7 @@ def generate_hint(output: str) -> Optional[str]: 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 @@ -100,7 +100,7 @@ def generate_hint(output: str) -> Optional[str]: # 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 @@ -117,7 +117,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)) diff --git a/tools/test_idf_py/test_hints.py b/tools/test_idf_py/test_hints.py index 18c6092430..faa0b82a3e 100755 --- a/tools/test_idf_py/test_hints.py +++ b/tools/test_idf_py/test_hints.py @@ -171,5 +171,34 @@ class TestNestedModuleComponentRequirements(unittest.TestCase): 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() From 6133810392a9b21f388bd937c62822cfce05da9f Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Tue, 23 Jan 2024 18:22:08 +0100 Subject: [PATCH 4/4] fix: harden input parsing in component_requirements hint module Currently we silently ignore when the original component is not found in a hope we can provide at least some meaningful hint. As it turned out it's not true. Instead of providing misleading hint, just return error. This adds several checks for situations, which should not happen, but when they do it should be easier to identify the root cause of the problem. For example when hint module received malformed output with extra new lines, e.g. caused by a bug in RunTool, it wrongly reported the original component as source component. This should also fix the tests on Windows. Signed-off-by: Frantisek Hrbata --- .../hint_modules/component_requirements.py | 28 ++++++++++++++----- tools/test_idf_py/test_hints.py | 4 +-- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/tools/idf_py_actions/hint_modules/component_requirements.py b/tools/idf_py_actions/hint_modules/component_requirements.py index 04d1675a99..92e3a66825 100644 --- a/tools/idf_py_actions/hint_modules/component_requirements.py +++ b/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. @@ -74,13 +78,14 @@ def generate_hint(output: str) -> Optional[str]: # 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]) @@ -92,11 +97,19 @@ def generate_hint(output: str) -> Optional[str]: 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 - # report something meaningful, so just keep going. - pass + # the preprocessor output was changed or we received malformed + # output. + 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 = [] @@ -154,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 diff --git a/tools/test_idf_py/test_hints.py b/tools/test_idf_py/test_hints.py index faa0b82a3e..393c48d67e 100755 --- a/tools/test_idf_py/test_hints.py +++ b/tools/test_idf_py/test_hints.py @@ -149,7 +149,7 @@ class TestNestedModuleComponentRequirements(unittest.TestCase): self.projectdir.mkdir(parents=True) (self.projectdir / 'CMakeLists.txt').write_text(( 'cmake_minimum_required(VERSION 3.16)\n' - f'set(EXTRA_COMPONENT_DIRS "{components}")\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)')) @@ -164,7 +164,7 @@ class TestNestedModuleComponentRequirements(unittest.TestCase): # 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('BUG: esp_timer.h found in component esp_timer which is already in the requirements list of component1', output) + 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: