mirror of
https://github.com/espressif/esp-idf.git
synced 2024-10-05 20:47:46 -04:00
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 <frantisek.hrbata@espressif.com>
This commit is contained in:
parent
273324c635
commit
a773072734
@ -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
|
||||
|
@ -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:
|
||||
|
Loading…
Reference in New Issue
Block a user