From c3729929a84a23ffa79098a348111f4041f98077 Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Fri, 22 Aug 2025 12:20:28 +0200 Subject: [PATCH 1/7] change(tools): Fix pre-commit checks for test_components.py --- tools/test_build_system/test_components.py | 158 ++++++++++++--------- 1 file changed, 94 insertions(+), 64 deletions(-) diff --git a/tools/test_build_system/test_components.py b/tools/test_build_system/test_components.py index 1cbff9706f..5f13f2a80a 100644 --- a/tools/test_build_system/test_components.py +++ b/tools/test_build_system/test_components.py @@ -1,16 +1,16 @@ -# SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD +# SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: Apache-2.0 import json import logging import os import shutil +from collections.abc import Generator from pathlib import Path -from typing import Generator import pytest -from test_build_system_helpers import append_to_file from test_build_system_helpers import EnvDict from test_build_system_helpers import IdfPyFunc +from test_build_system_helpers import append_to_file from test_build_system_helpers import replace_in_file @@ -42,8 +42,11 @@ def create_idf_components(request: pytest.FixtureRequest) -> Generator: def test_component_extra_dirs(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('Setting EXTRA_COMPONENT_DIRS works') shutil.move(test_app_copy / 'main', test_app_copy / 'different_main' / 'main') - replace_in_file((test_app_copy / 'CMakeLists.txt'), '# placeholder_before_include_project_cmake', - 'set(EXTRA_COMPONENT_DIRS {})'.format(Path('different_main', 'main').as_posix())) + replace_in_file( + (test_app_copy / 'CMakeLists.txt'), + '# placeholder_before_include_project_cmake', + 'set(EXTRA_COMPONENT_DIRS {})'.format(Path('different_main', 'main').as_posix()), + ) ret = idf_py('reconfigure') assert str((test_app_copy / 'different_main' / 'main').as_posix()) in ret.stdout assert str((test_app_copy / 'main').as_posix()) not in ret.stdout @@ -64,10 +67,10 @@ def test_component_names_contain_spaces(idf_py: IdfPyFunc, test_app_copy: Path) def test_component_can_not_be_empty_dir(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('Empty directory not treated as a component') - empty_component_dir = (test_app_copy / 'components' / 'esp32') + empty_component_dir = test_app_copy / 'components' / 'esp32' empty_component_dir.mkdir(parents=True) idf_py('reconfigure') - data = json.load(open(test_app_copy / 'build' / 'project_description.json', 'r')) + data = json.load(open(test_app_copy / 'build' / 'project_description.json')) assert str(empty_component_dir) not in data.get('build_component_paths') @@ -76,14 +79,14 @@ def test_component_subdirs_not_added_to_component_dirs(idf_py: IdfPyFunc, test_a (test_app_copy / 'main' / 'test').mkdir(parents=True) (test_app_copy / 'main' / 'test' / 'CMakeLists.txt').write_text('idf_component_register()') idf_py('reconfigure') - data = json.load(open(test_app_copy / 'build' / 'project_description.json', 'r')) + data = json.load(open(test_app_copy / 'build' / 'project_description.json')) assert str((test_app_copy / 'main' / 'test').as_posix()) not in data.get('build_component_paths') assert str((test_app_copy / 'main').as_posix()) in data.get('build_component_paths') def test_component_sibling_dirs_not_added_to_component_dirs(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('If a component directory is added to COMPONENT_DIRS, its sibling directories are not added') - mycomponents_subdir = (test_app_copy / 'mycomponents') + mycomponents_subdir = test_app_copy / 'mycomponents' (mycomponents_subdir / 'mycomponent').mkdir(parents=True) (mycomponents_subdir / 'mycomponent' / 'CMakeLists.txt').write_text('idf_component_register()') @@ -91,7 +94,7 @@ def test_component_sibling_dirs_not_added_to_component_dirs(idf_py: IdfPyFunc, t (mycomponents_subdir / 'esp32').mkdir(parents=True) (mycomponents_subdir / 'esp32' / 'CMakeLists.txt').write_text('idf_component_register()') idf_py('-DEXTRA_COMPONENT_DIRS={}'.format(str(mycomponents_subdir / 'mycomponent')), 'reconfigure') - data = json.load(open(test_app_copy / 'build' / 'project_description.json', 'r')) + data = json.load(open(test_app_copy / 'build' / 'project_description.json')) assert str((mycomponents_subdir / 'esp32').as_posix()) not in data.get('build_component_paths') assert str((mycomponents_subdir / 'mycomponent').as_posix()) in data.get('build_component_paths') shutil.rmtree(mycomponents_subdir / 'esp32') @@ -99,74 +102,95 @@ def test_component_sibling_dirs_not_added_to_component_dirs(idf_py: IdfPyFunc, t # now the same thing, but add a components directory (test_app_copy / 'esp32').mkdir() (test_app_copy / 'esp32' / 'CMakeLists.txt').write_text('idf_component_register()') - idf_py('-DEXTRA_COMPONENT_DIRS={}'.format(str(mycomponents_subdir)), 'reconfigure') - data = json.load(open(test_app_copy / 'build' / 'project_description.json', 'r')) + idf_py(f'-DEXTRA_COMPONENT_DIRS={str(mycomponents_subdir)}', 'reconfigure') + data = json.load(open(test_app_copy / 'build' / 'project_description.json')) assert str((test_app_copy / 'esp32').as_posix()) not in data.get('build_component_paths') assert str((mycomponents_subdir / 'mycomponent').as_posix()) in data.get('build_component_paths') def test_component_properties_are_set(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('Component properties are set') - append_to_file(test_app_copy / 'CMakeLists.txt', '\n'.join(['', - 'idf_component_get_property(srcs main SRCS)', - 'message(STATUS SRCS:${srcs})'])) + append_to_file( + test_app_copy / 'CMakeLists.txt', + '\n'.join(['', 'idf_component_get_property(srcs main SRCS)', 'message(STATUS SRCS:${srcs})']), + ) ret = idf_py('reconfigure') - assert 'SRCS:{}'.format((test_app_copy / 'main' / 'build_test_app.c').as_posix()) in ret.stdout, 'Component properties should be set' + assert 'SRCS:{}'.format((test_app_copy / 'main' / 'build_test_app.c').as_posix()) in ret.stdout, ( + 'Component properties should be set' + ) def test_get_property_for_unknown_component(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('Getting property of unknown component fails gracefully') - append_to_file(test_app_copy / 'CMakeLists.txt', '\n'.join(['', - 'idf_component_get_property(VAR UNKNOWN PROP)'])) + append_to_file(test_app_copy / 'CMakeLists.txt', '\n'.join(['', 'idf_component_get_property(VAR UNKNOWN PROP)'])) ret = idf_py('reconfigure', check=False) - assert "Failed to resolve component 'UNKNOWN'" in ret.stderr, ('idf_component_get_property ' - 'for unknown component should fail gracefully') + assert "Failed to resolve component 'UNKNOWN'" in ret.stderr, ( + 'idf_component_get_property for unknown component should fail gracefully' + ) def test_set_property_for_unknown_component(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('Setting property of unknown component fails gracefully') - append_to_file(test_app_copy / 'CMakeLists.txt', '\n'.join(['', - 'idf_component_set_property(UNKNOWN PROP VAL)'])) + append_to_file(test_app_copy / 'CMakeLists.txt', '\n'.join(['', 'idf_component_set_property(UNKNOWN PROP VAL)'])) ret = idf_py('reconfigure', check=False) - assert "Failed to resolve component 'UNKNOWN'" in ret.stderr, ('idf_component_set_property ' - 'for unknown component should fail gracefully') + assert "Failed to resolve component 'UNKNOWN'" in ret.stderr, ( + 'idf_component_set_property for unknown component should fail gracefully' + ) def test_component_overridden_dir(idf_py: IdfPyFunc, test_app_copy: Path, default_idf_env: EnvDict) -> None: logging.info('Getting component overridden dir') (test_app_copy / 'components' / 'hal').mkdir(parents=True) - (test_app_copy / 'components' / 'hal' / 'CMakeLists.txt').write_text('\n'.join([ - 'idf_component_get_property(overridden_dir ${COMPONENT_NAME} COMPONENT_OVERRIDEN_DIR)', - 'message(STATUS overridden_dir:${overridden_dir})', 'idf_component_register()'])) + (test_app_copy / 'components' / 'hal' / 'CMakeLists.txt').write_text( + '\n'.join( + [ + 'idf_component_get_property(overridden_dir ${COMPONENT_NAME} COMPONENT_OVERRIDEN_DIR)', + 'message(STATUS overridden_dir:${overridden_dir})', + 'idf_component_register()', + ] + ) + ) ret = idf_py('reconfigure') idf_path = Path(default_idf_env.get('IDF_PATH')) # no registration, overrides registration as well - assert 'overridden_dir:{}'.format((idf_path / 'components' / 'hal').as_posix()) in ret.stdout, 'Failed to get overridden dir' - append_to_file((test_app_copy / 'components' / 'hal' / 'CMakeLists.txt'), '\n'.join([ - '', - 'idf_component_register(KCONFIG ${overridden_dir}/Kconfig)', - 'idf_component_get_property(kconfig ${COMPONENT_NAME} KCONFIG)', - 'message(STATUS kconfig:${overridden_dir}/Kconfig)'])) + assert 'overridden_dir:{}'.format((idf_path / 'components' / 'hal').as_posix()) in ret.stdout, ( + 'Failed to get overridden dir' + ) + append_to_file( + (test_app_copy / 'components' / 'hal' / 'CMakeLists.txt'), + '\n'.join( + [ + '', + 'idf_component_register(KCONFIG ${overridden_dir}/Kconfig)', + 'idf_component_get_property(kconfig ${COMPONENT_NAME} KCONFIG)', + 'message(STATUS kconfig:${overridden_dir}/Kconfig)', + ] + ), + ) ret = idf_py('reconfigure', check=False) - assert 'kconfig:{}'.format((idf_path / 'components' / 'hal').as_posix()) in ret.stdout, 'Failed to verify original `main` directory' + assert 'kconfig:{}'.format((idf_path / 'components' / 'hal').as_posix()) in ret.stdout, ( + 'Failed to verify original `main` directory' + ) def test_project_components_overrides_extra_components(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('Project components override components defined in EXTRA_COMPONENT_DIRS') (test_app_copy / 'extra_dir' / 'my_component').mkdir(parents=True) (test_app_copy / 'extra_dir' / 'my_component' / 'CMakeLists.txt').write_text('idf_component_register()') - replace_in_file(test_app_copy / 'CMakeLists.txt', - '# placeholder_before_include_project_cmake', - 'set(EXTRA_COMPONENT_DIRS extra_dir)') + replace_in_file( + test_app_copy / 'CMakeLists.txt', + '# placeholder_before_include_project_cmake', + 'set(EXTRA_COMPONENT_DIRS extra_dir)', + ) idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) assert str((test_app_copy / 'extra_dir' / 'my_component').as_posix()) in data.get('build_component_paths') (test_app_copy / 'components' / 'my_component').mkdir(parents=True) (test_app_copy / 'components' / 'my_component' / 'CMakeLists.txt').write_text('idf_component_register()') idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) assert str((test_app_copy / 'components' / 'my_component').as_posix()) in data.get('build_component_paths') assert str((test_app_copy / 'extra_dir' / 'my_component').as_posix()) not in data.get('build_component_paths') @@ -179,22 +203,24 @@ def test_extra_components_overrides_managed_components(idf_py: IdfPyFunc, test_a example/cmp: "*" """) idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) - assert str((test_app_copy / 'managed_components' / 'example__cmp').as_posix()) in data.get( - 'build_component_paths') + assert str((test_app_copy / 'managed_components' / 'example__cmp').as_posix()) in data.get('build_component_paths') (test_app_copy / 'extra_dir' / 'cmp').mkdir(parents=True) (test_app_copy / 'extra_dir' / 'cmp' / 'CMakeLists.txt').write_text('idf_component_register()') - replace_in_file(test_app_copy / 'CMakeLists.txt', - '# placeholder_before_include_project_cmake', - 'set(EXTRA_COMPONENT_DIRS extra_dir)') + replace_in_file( + test_app_copy / 'CMakeLists.txt', + '# placeholder_before_include_project_cmake', + 'set(EXTRA_COMPONENT_DIRS extra_dir)', + ) idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) assert str((test_app_copy / 'extra_dir' / 'cmp').as_posix()) in data.get('build_component_paths') assert str((test_app_copy / 'managed_components' / 'example__cmp').as_posix()) not in data.get( - 'build_component_paths') + 'build_component_paths' + ) @pytest.mark.with_idf_components(['cmp']) @@ -203,32 +229,31 @@ def test_managed_components_overrides_idf_components(idf_py: IdfPyFunc, test_app # created idf component 'cmp' in marker idf_path = Path(os.environ['IDF_PATH']) idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) - assert str((idf_path / 'components' / 'cmp').as_posix()) in data.get( - 'build_component_paths') + assert str((idf_path / 'components' / 'cmp').as_posix()) in data.get('build_component_paths') (test_app_copy / 'main' / 'idf_component.yml').write_text(""" dependencies: example/cmp: "*" """) idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) - assert str((test_app_copy / 'managed_components' / 'example__cmp').as_posix()) in data.get( - 'build_component_paths') - assert str((idf_path / 'components' / 'cmp').as_posix()) not in data.get( - 'build_component_paths') + assert str((test_app_copy / 'managed_components' / 'example__cmp').as_posix()) in data.get('build_component_paths') + assert str((idf_path / 'components' / 'cmp').as_posix()) not in data.get('build_component_paths') def test_manifest_local_source_overrides_extra_components(idf_py: IdfPyFunc, test_app_copy: Path) -> None: (test_app_copy / '..' / 'extra_dir' / 'cmp').mkdir(parents=True) (test_app_copy / '..' / 'extra_dir' / 'cmp' / 'CMakeLists.txt').write_text('idf_component_register()') - replace_in_file(test_app_copy / 'CMakeLists.txt', - '# placeholder_before_include_project_cmake', - 'set(EXTRA_COMPONENT_DIRS ../extra_dir)') + replace_in_file( + test_app_copy / 'CMakeLists.txt', + '# placeholder_before_include_project_cmake', + 'set(EXTRA_COMPONENT_DIRS ../extra_dir)', + ) idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) assert str((test_app_copy / '..' / 'extra_dir' / 'cmp').resolve().as_posix()) in data.get('build_component_paths') @@ -241,10 +266,12 @@ dependencies: path: '../../cmp' """) idf_py('reconfigure') - with open(test_app_copy / 'build' / 'project_description.json', 'r') as f: + with open(test_app_copy / 'build' / 'project_description.json') as f: data = json.load(f) assert str((test_app_copy / '..' / 'cmp').resolve().as_posix()) in data.get('build_component_paths') - assert str((test_app_copy / '..' / 'extra_dir' / 'cmp').resolve().as_posix()) not in data.get('build_component_paths') + assert str((test_app_copy / '..' / 'extra_dir' / 'cmp').resolve().as_posix()) not in data.get( + 'build_component_paths' + ) def test_exclude_components_not_passed(idf_py: IdfPyFunc, test_app_copy: Path) -> None: @@ -258,8 +285,11 @@ def test_exclude_components_not_passed(idf_py: IdfPyFunc, test_app_copy: Path) - def test_version_in_component_cmakelist(idf_py: IdfPyFunc, test_app_copy: Path) -> None: logging.info('Use IDF version variables in component CMakeLists.txt file') - replace_in_file((test_app_copy / 'main' / 'CMakeLists.txt'), '# placeholder_before_idf_component_register', - '\n'.join(['if (NOT IDF_VERSION_MAJOR)', ' message(FATAL_ERROR "IDF version not set")', 'endif()'])) + replace_in_file( + (test_app_copy / 'main' / 'CMakeLists.txt'), + '# placeholder_before_idf_component_register', + '\n'.join(['if (NOT IDF_VERSION_MAJOR)', ' message(FATAL_ERROR "IDF version not set")', 'endif()']), + ) idf_py('reconfigure') @@ -271,4 +301,4 @@ def test_unknown_component_error(idf_py: IdfPyFunc, test_app_copy: Path) -> None replace='REQUIRES unknown', ) ret = idf_py('reconfigure', check=False) - assert 'Failed to resolve component \'unknown\' required by component \'main\'' in ret.stderr + assert "Failed to resolve component 'unknown' required by component 'main'" in ret.stderr From adb2d5deeebeddf02aa702146d714ef1e5e50cfa Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Mon, 2 Jun 2025 16:32:49 +0200 Subject: [PATCH 2/7] feat(cmake): Produce warnings when component dependencies are not defined There are idf.py hints for helping the user to set component dependencies properly instead of building sources out-of-component or including headers from outside the component directory. These are produced with tools/idf_py_actions/hint_modules/component_requirements.py. However, idf.py hints are printed only when the build fails. If the user starts with a buildable solution then the suggestions to add component dependencies are not printed. This commit introduces cmake-level warnings for building source files from outside the component and including header files without setting up proper component dependencies. --- CMakeLists.txt | 9 + tools/cmake/component_validation.cmake | 132 +++++++++++ tools/test_build_system/test_components.py | 246 +++++++++++++++++++++ 3 files changed, 387 insertions(+) create mode 100644 tools/cmake/component_validation.cmake diff --git a/CMakeLists.txt b/CMakeLists.txt index d21d2a38ec..9b049561b2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -320,3 +320,12 @@ foreach(component_target ${build_component_targets}) endif() set(__idf_component_context 0) endforeach() + +# Run component validation checks after all components have been processed +# Only run validation for the main project, not subprojects like bootloader +idf_build_get_property(bootloader_build BOOTLOADER_BUILD) +idf_build_get_property(esp_tee_build ESP_TEE_BUILD) +if(NOT bootloader_build AND NOT esp_tee_build) + include("${CMAKE_CURRENT_LIST_DIR}/tools/cmake/component_validation.cmake") + __component_validation_run_checks() +endif() diff --git a/tools/cmake/component_validation.cmake b/tools/cmake/component_validation.cmake new file mode 100644 index 0000000000..f7d66103df --- /dev/null +++ b/tools/cmake/component_validation.cmake @@ -0,0 +1,132 @@ +# +# Component validation checks +# +# This module contains checks that validate component source files and include directories +# to ensure they belong to the correct component. These checks run after all components +# have been discovered and processed. +# + +# +# Check if a path belongs to a specific component +# +function(__component_validation_get_component_for_path var path) + # Determine the starting directory to check: use the path itself if it's a directory, + # otherwise use its containing directory + set(current_dir "${path}") + if(NOT IS_DIRECTORY "${current_dir}") + get_filename_component(current_dir "${path}" DIRECTORY) + endif() + + # Get all component targets + idf_build_get_property(component_targets __COMPONENT_TARGETS) + + # Walk up the directory tree from the deepest path towards root and return + # the first component whose COMPONENT_DIR matches exactly. This guarantees + # selecting the deepest matching component without extra heuristics. + while(NOT "${current_dir}" STREQUAL "" AND + NOT "${current_dir}" STREQUAL "/" AND + NOT "${current_dir}" MATCHES "^[A-Za-z]:/$") + foreach(component_target ${component_targets}) + __component_get_property(component_dir ${component_target} COMPONENT_DIR) + if(current_dir STREQUAL component_dir) + set(${var} ${component_target} PARENT_SCOPE) + return() + endif() + endforeach() + get_filename_component(current_dir "${current_dir}" DIRECTORY) + endwhile() + + # If no component found, return empty + set(${var} "" PARENT_SCOPE) +endfunction() + +# +# Validate that source files belong to the correct component +# +function(__component_validation_check_sources component_target) + __component_get_property(sources ${component_target} SRCS) + __component_get_property(component_name ${component_target} COMPONENT_NAME) + __component_get_property(component_dir ${component_target} COMPONENT_DIR) + + foreach(src ${sources}) + # Check if this source file belongs to another component + __component_validation_get_component_for_path(owner_component ${src}) + + if(owner_component AND NOT owner_component STREQUAL component_target) + __component_get_property(owner_name ${owner_component} COMPONENT_NAME) + message(WARNING + "Source file '${src}' belongs to component ${owner_name} but is being built by " + "component ${component_name}. It is recommended to build source files by " + "defining component dependencies for ${component_name} " + "via using idf_component_register(REQUIRES ${owner_name}) " + "or idf_component_register(PRIV_REQUIRES ${owner_name}) in the CMakeLists.txt of " + "${component_name}.") + endif() + endforeach() +endfunction() + +# +# Validate that include directories belong to the correct component +# +function(__component_validation_check_include_dirs component_target) + __component_get_property(include_dirs ${component_target} INCLUDE_DIRS) + __component_get_property(priv_include_dirs ${component_target} PRIV_INCLUDE_DIRS) + __component_get_property(component_name ${component_target} COMPONENT_NAME) + __component_get_property(component_dir ${component_target} COMPONENT_DIR) + + # Check public include directories + foreach(dir ${include_dirs}) + # Check if this include directory belongs to another component + # Normalize to absolute path relative to this component directory + get_filename_component(abs_dir ${dir} ABSOLUTE BASE_DIR ${component_dir}) + __component_validation_get_component_for_path(owner_component ${abs_dir}) + + if(owner_component AND NOT owner_component STREQUAL component_target) + __component_get_property(owner_name ${owner_component} COMPONENT_NAME) + message(WARNING + "Include directory '${abs_dir}' belongs to component ${owner_name} but is being " + "used by component ${component_name}. It is recommended to define the " + "component dependency for '${component_name}' on the component ${owner_name}, " + "i.e. 'idf_component_register(... REQUIRES ${owner_name})' in the " + "CMakeLists.txt of ${component_name}, and specify the included directory " + "as idf_component_register(... INCLUDE_DIRS ) " + "in the CMakeLists.txt of component ${owner_name}.") + endif() + endforeach() + + # Check private include directories + foreach(dir ${priv_include_dirs}) + # Check if this include directory belongs to another component + # Normalize to absolute path relative to this component directory + get_filename_component(abs_dir ${dir} ABSOLUTE BASE_DIR ${component_dir}) + __component_validation_get_component_for_path(owner_component ${abs_dir}) + + if(owner_component AND NOT owner_component STREQUAL component_target) + __component_get_property(owner_name ${owner_component} COMPONENT_NAME) + message(WARNING + "Private include directory '${abs_dir}' belongs to component ${owner_name} but " + "is being used by component ${component_name}. " + "It is recommended to define the component dependency for ${component_name} " + "on the component ${owner_name}, " + "i.e. 'idf_component_register(... PRIV_REQUIRES ${owner_name})' in the " + "CMakeLists.txt of ${component_name}, " + "and specify the included directory as " + "idf_component_register(... PRIV_INCLUDE_DIRS ) " + "in the CMakeLists.txt of component ${owner_name}.") + endif() + endforeach() +endfunction() + +# +# Run validation checks for all components +# +function(__component_validation_run_checks) + # Get all component targets + idf_build_get_property(component_targets __COMPONENT_TARGETS) + + # Run validation checks for each component + foreach(component_target ${component_targets}) + __component_validation_check_sources(${component_target}) + __component_validation_check_include_dirs(${component_target}) + endforeach() +endfunction() diff --git a/tools/test_build_system/test_components.py b/tools/test_build_system/test_components.py index 5f13f2a80a..f035e0ec64 100644 --- a/tools/test_build_system/test_components.py +++ b/tools/test_build_system/test_components.py @@ -3,6 +3,7 @@ import json import logging import os +import re import shutil from collections.abc import Generator from pathlib import Path @@ -302,3 +303,248 @@ def test_unknown_component_error(idf_py: IdfPyFunc, test_app_copy: Path) -> None ) ret = idf_py('reconfigure', check=False) assert "Failed to resolve component 'unknown' required by component 'main'" in ret.stderr + + +def test_component_with_improper_dependency(idf_py: IdfPyFunc, test_app_copy: Path) -> None: + # test for __component_validation_check_include_dirs and __component_validation_check_sources + # Checks that the following warnings are produced: + # - Include directory X belongs to component Y but is being used by component Z + # - Source file A belongs to component Y but is being built by component Z + logging.info( + 'Check for warnings when component includes source files or include directories that belong to other components' + ) + idf_py('create-component', '-C', 'components', 'my_comp') + + # Create a source file and include directory in my_comp + (test_app_copy / 'components' / 'my_comp' / 'my_comp.c').write_text('void my_func() {}') + (test_app_copy / 'components' / 'my_comp' / 'include').mkdir(exist_ok=True) + (test_app_copy / 'components' / 'my_comp' / 'include' / 'my_comp.h').write_text('#pragma once') + + # Make main component try to use files from my_comp + replace_in_file( + (test_app_copy / 'main' / 'CMakeLists.txt'), + '# placeholder_inside_idf_component_register', + '"../components/my_comp/my_comp.c"\n INCLUDE_DIRS "../components/my_comp/include"', + ) + ret = idf_py('reconfigure') + + inc_dir = (test_app_copy / 'components' / 'my_comp' / 'include').as_posix() + src_file = (test_app_copy / 'components' / 'my_comp' / 'my_comp.c').as_posix() + + # Check for new validation warnings + re_include = re.compile( + rf"Include directory\s+'{re.escape(inc_dir)}'\s+belongs to component\s+my_comp\s+but is being used by " + rf'component\s+main' + ) + re_source = re.compile( + rf"Source file\s+'{re.escape(src_file)}'\s+belongs to component\s+my_comp\s+but is being built by " + rf'component\s+main' + ) + + assert re_include.search(ret.stderr) is not None, f'Expected include directory warning not found in: {ret.stderr}' + assert re_source.search(ret.stderr) is not None, f'Expected source file warning not found in: {ret.stderr}' + + +def test_component_validation_not_run_in_subprojects(idf_py: IdfPyFunc, test_app_copy: Path) -> None: + # test that component validation doesn't run in subprojects like bootloader + logging.info('Check that component validation warnings are not shown in subprojects') + + # Create a component that would trigger validation warnings + idf_py('create-component', '-C', 'components', 'test_comp') + (test_app_copy / 'components' / 'test_comp' / 'test_comp.c').write_text('void test_func() {}') + (test_app_copy / 'components' / 'test_comp' / 'include').mkdir(exist_ok=True) + (test_app_copy / 'components' / 'test_comp' / 'include' / 'test_comp.h').write_text('#pragma once') + + # Make main component try to use files from test_comp + replace_in_file( + (test_app_copy / 'main' / 'CMakeLists.txt'), + '# placeholder_inside_idf_component_register', + '"../components/test_comp/test_comp.c"\n INCLUDE_DIRS "../components/test_comp/include"', + ) + + # Build the project - this will trigger bootloader build as well + ret = idf_py('build') + + # Check that validation warnings appear in the main build output + inc_dir = (test_app_copy / 'components' / 'test_comp' / 'include').as_posix() + src_file = (test_app_copy / 'components' / 'test_comp' / 'test_comp.c').as_posix() + + re_include = re.compile( + rf"Include directory\s+'{re.escape(inc_dir)}'\s+belongs to component\s+test_comp\s+but is being used by " + rf'component\s+main' + ) + re_source = re.compile( + rf"Source file\s+'{re.escape(src_file)}'\s+belongs to component\s+test_comp\s+but is being built by " + rf'component\s+main' + ) + + # The warnings should appear in the main build, not in bootloader build + assert re_include.search(ret.stderr) is not None, f'Expected include directory warning not found in: {ret.stderr}' + assert re_source.search(ret.stderr) is not None, f'Expected source file warning not found in: {ret.stderr}' + + # Verify that the build completed successfully despite the warnings + assert ret.returncode == 0, 'Build should complete successfully with validation warnings' + + +def test_component_validation_private_include_dirs(idf_py: IdfPyFunc, test_app_copy: Path) -> None: + # test that component validation works for private include directories + logging.info('Check that component validation warnings are shown for private include directories') + + # Create a component with private include directory + idf_py('create-component', '-C', 'components', 'private_comp') + (test_app_copy / 'components' / 'private_comp' / 'private').mkdir(exist_ok=True) + (test_app_copy / 'components' / 'private_comp' / 'private' / 'private.h').write_text('#pragma once') + + # Make main component try to use private include directory from private_comp + replace_in_file( + (test_app_copy / 'main' / 'CMakeLists.txt'), + '# placeholder_inside_idf_component_register', + 'PRIV_INCLUDE_DIRS "../components/private_comp/private"', + ) + + ret = idf_py('reconfigure') + + # Check for private include directory warning + priv_inc_dir = (test_app_copy / 'components' / 'private_comp' / 'private').as_posix() + re_priv_include = re.compile( + rf"Private include directory\s+'{re.escape(priv_inc_dir)}'\s+belongs to " + rf'component\s+private_comp\s+but is being used by component\s+main' + ) + + assert re_priv_include.search(ret.stderr) is not None, ( + f'Expected private include directory warning not found in: {ret.stderr}' + ) + + +def test_component_validation_finds_right_component(idf_py: IdfPyFunc, test_app_copy: Path) -> None: + # test that __component_validation_get_component_for_path finds the correct component for a given path + # + # components/my_comp/test_apps/components/my_subcomp/src/test.c + # The component for test.c should be my_subcomp, not my_comp + + idf_py('create-component', '-C', 'components', 'my_comp') + + nested_components_dir = test_app_copy / 'components' / 'my_comp' / 'test_apps' / 'components' + my_subcomp_dir = nested_components_dir / 'my_subcomp' + (my_subcomp_dir / 'src').mkdir(parents=True) + (my_subcomp_dir / 'include').mkdir(parents=True) + + # Files of the nested component + (my_subcomp_dir / 'src' / 'test.c').write_text('void test_func() {}') + (my_subcomp_dir / 'include' / 'test.h').write_text('#pragma once') + (my_subcomp_dir / 'CMakeLists.txt').write_text('idf_component_register(SRCS "src/test.c" INCLUDE_DIRS "include")') + + # Make sure build system discovers the nested component by adding its parent directory to EXTRA_COMPONENT_DIRS + replace_in_file( + test_app_copy / 'CMakeLists.txt', + '# placeholder_before_include_project_cmake', + f'set(EXTRA_COMPONENT_DIRS {nested_components_dir.as_posix()})', + ) + + # Make main component try to use files from my_subcomp via absolute-like relative paths + replace_in_file( + test_app_copy / 'main' / 'CMakeLists.txt', + '# placeholder_inside_idf_component_register', + '"../components/my_comp/test_apps/components/my_subcomp/src/test.c"\n' + ' INCLUDE_DIRS "../components/my_comp/test_apps/components/my_subcomp/include"', + ) + + ret = idf_py('reconfigure') + + inc_dir = (my_subcomp_dir / 'include').as_posix() + src_file = (my_subcomp_dir / 'src' / 'test.c').as_posix() + + # The warnings must attribute ownership to my_subcomp (deepest component), not my_comp + re_include = re.compile( + rf"Include directory\s+'{re.escape(inc_dir)}'\s+belongs to component\s+my_subcomp\s+but is being used by " + rf'component\s+main' + ) + re_source = re.compile( + rf"Source file\s+'{re.escape(src_file)}'\s+belongs to component\s+my_subcomp\s+but is being built by " + rf'component\s+main' + ) + + assert re_include.search(ret.stderr) is not None, f'Expected include directory warning not found in: {ret.stderr}' + assert re_source.search(ret.stderr) is not None, f'Expected source file warning not found in: {ret.stderr}' + + # components/my_comp/test_apps/components/my_subcomp/include/test.h + # The component for test.h should be my_subcomp, not my_comp + # Modify main to also list the header as a source to exercise file-level ownership + replace_in_file( + test_app_copy / 'main' / 'CMakeLists.txt', + '"../components/my_comp/test_apps/components/my_subcomp/src/test.c"\n' + ' INCLUDE_DIRS "../components/my_comp/test_apps/components/my_subcomp/include"', + '"../components/my_comp/test_apps/components/my_subcomp/src/test.c" ' + '"../components/my_comp/test_apps/components/my_subcomp/include/test.h"\n' + ' INCLUDE_DIRS "../components/my_comp/test_apps/components/my_subcomp/include"', + ) + + ret = idf_py('reconfigure') + + header_path = (my_subcomp_dir / 'include' / 'test.h').as_posix() + re_header = re.compile( + rf"Source file\s+'{re.escape(header_path)}'\s+belongs to component\s+my_subcomp\s+but is being built by " + rf'component\s+main' + ) + assert re_header.search(ret.stderr) is not None, ( + f'Expected header file ownership warning not found in: {ret.stderr}' + ) + + +def test_component_validation_with_common_platform_example(idf_py: IdfPyFunc, test_app_copy: Path) -> None: + # Test the following structure which should not produce a warning:: + # + # my_project/ + # ├── common/ # Common product code for all platforms (not a component) + # │ ├── include/ + # │ │ ├── product_config.h + # │ │ └── business_logic.h + # │ └── src/ + # │ └── business_logic.c + # └── env/ + # ├── esp-idf/ + # │ ├── main/ # main component + # │ │ ├── idf_main.c # includes product_config.h and business_logic.h + # │ │ └── CMakeLists.txt # adds ../../../common/include to include dirs + # │ └── CMakeLists.txt + # └── other_rtos/ + # + # + # Implementation: create a sibling 'common' directory outside the IDF project and + # make the main component include headers and a source file from it. This should + # NOT produce component ownership warnings because the paths don't belong to any component. + + # Create common directory with headers and source outside the project root + common_dir = (test_app_copy / '..' / 'common').resolve() + (common_dir / 'include').mkdir(parents=True, exist_ok=True) + (common_dir / 'src').mkdir(parents=True, exist_ok=True) + (common_dir / 'include' / 'product_config.h').write_text('#pragma once\n') + (common_dir / 'include' / 'business_logic.h').write_text('#pragma once\n') + (common_dir / 'src' / 'business_logic.c').write_text('void bl(void) {}\n') + + # From main component dir to common dir is ../../common + replace_in_file( + test_app_copy / 'main' / 'CMakeLists.txt', + '# placeholder_inside_idf_component_register', + '"../../common/src/business_logic.c"\n INCLUDE_DIRS "../../common/include"', + ) + + # Optionally create a main source that includes the headers (not required for validation) + (test_app_copy / 'main' / 'idf_main.c').write_text( + '#include "product_config.h"\n#include "business_logic.h"\nvoid app_main(void) {}\n' + ) + + ret = idf_py('reconfigure') + + inc_dir_abs = (common_dir / 'include').as_posix() + src_file_abs = (common_dir / 'src' / 'business_logic.c').as_posix() + + re_include = re.compile(rf"Include directory\s+'{re.escape(inc_dir_abs)}'\s+belongs to component") + re_source = re.compile(rf"Source file\s+'{re.escape(src_file_abs)}'\s+belongs to component") + + assert re_include.search(ret.stderr) is None, ( + f'Unexpected include directory ownership warning for common path: {ret.stderr}' + ) + assert re_source.search(ret.stderr) is None, ( + f'Unexpected source file ownership warning for common path: {ret.stderr}' + ) From 26ae9e8589c6b7d9af002cd9409e507bfcd4b881 Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Mon, 15 Sep 2025 16:18:04 +0200 Subject: [PATCH 3/7] ci(gitlab): Split up Windows tests into (more) parallel jobs --- .gitlab/ci/test-win.yml | 6 +++--- tools/test_idf_py/pytest.ini | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.gitlab/ci/test-win.yml b/.gitlab/ci/test-win.yml index 91cf2df5ee..158413f8ee 100644 --- a/.gitlab/ci/test-win.yml +++ b/.gitlab/ci/test-win.yml @@ -47,6 +47,7 @@ test_tools_win: extends: - .host_test_win_template - .rules:labels:windows_pytest_build_system + parallel: 4 artifacts: paths: - ${IDF_PATH}/*.out @@ -64,8 +65,7 @@ test_tools_win: - .\export.ps1 - python "${SUBMODULE_FETCH_TOOL}" -s "all" - cd ${IDF_PATH}/tools/test_idf_py - - pytest --noconftest test_idf_py.py --junitxml=${IDF_PATH}/XUNIT_IDF_PY.xml - - pytest --noconftest test_hints.py --junitxml=${IDF_PATH}/XUNIT_HINTS.xml + - pytest --parallel-count ${CI_NODE_TOTAL} --parallel-index ${CI_NODE_INDEX} --junitxml=${IDF_PATH}/XUNIT_RESULT.xml # Build tests .test_build_system_template_win: @@ -88,7 +88,7 @@ pytest_build_system_win: extends: - .test_build_system_template_win - .rules:labels:windows_pytest_build_system - parallel: 2 + parallel: 6 needs: [] tags: [windows-build, brew] artifacts: diff --git a/tools/test_idf_py/pytest.ini b/tools/test_idf_py/pytest.ini index 8cb59810c3..1ef576adc8 100644 --- a/tools/test_idf_py/pytest.ini +++ b/tools/test_idf_py/pytest.ini @@ -1,5 +1,5 @@ [pytest] -addopts = -s -p no:pytest_embedded -p no:idf-ci +addopts = -s -p no:idf-ci # log related log_cli = True From 9e8962c6ccdabb80fdbde103e7f6ae56e9970d50 Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Thu, 18 Sep 2025 07:26:58 +0200 Subject: [PATCH 4/7] change(tools): Fix pre-commit checks for test_idf_qemu.py --- tools/test_idf_py/test_idf_qemu.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tools/test_idf_py/test_idf_qemu.py b/tools/test_idf_py/test_idf_qemu.py index de277753c2..27da2f511b 100755 --- a/tools/test_idf_py/test_idf_qemu.py +++ b/tools/test_idf_py/test_idf_qemu.py @@ -1,6 +1,6 @@ #!/usr/bin/env python # -# SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD +# SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: Apache-2.0 import logging import os @@ -17,11 +17,9 @@ class IdfPyQemuTest(unittest.TestCase): idf_path = os.environ['IDF_PATH'] hello_world_dir = os.path.join(idf_path, 'examples', 'get-started', 'hello_world') idf_py = os.path.join(idf_path, 'tools', 'idf.py') - args = [idf_py, '-C', hello_world_dir, '-B', build_dir, - 'qemu', '--qemu-extra-args', '-no-reboot', 'monitor'] + args = [idf_py, '-C', hello_world_dir, '-B', build_dir, 'qemu', '--qemu-extra-args', '-no-reboot', 'monitor'] logfile_name = os.path.join(os.environ['IDF_PATH'], 'qemu_log.out') - with open(logfile_name, 'w+b') as logfile, \ - pexpect.spawn(sys.executable, args=args, logfile=logfile) as child: + with open(logfile_name, 'w+b') as logfile, pexpect.spawn(sys.executable, args=args, logfile=logfile) as child: child.expect_exact('Executing action: all') logging.info('Waiting for the build to finish...') child.expect_exact('Executing action: qemu', timeout=120) @@ -33,8 +31,7 @@ class IdfPyQemuTest(unittest.TestCase): child.expect_exact('Restarting now.') args = [idf_py, '-C', hello_world_dir, '-B', build_dir, 'qemu', 'efuse-summary', '--format=summary'] - with open(logfile_name, 'w+b') as logfile, \ - pexpect.spawn(sys.executable, args=args, logfile=logfile) as child: + with open(logfile_name, 'w+b') as logfile, pexpect.spawn(sys.executable, args=args, logfile=logfile) as child: child.expect_exact('Executing action: efuse-summary') child.expect_exact('WR_DIS (BLOCK0)') From ef878f4d25123db722d761a9cca9db54815b93c9 Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Tue, 16 Sep 2025 19:01:16 +0200 Subject: [PATCH 5/7] ci(test): Test requiring pexpect.spawn cannot be run on Windows --- tools/test_idf_py/test_idf_qemu.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/test_idf_py/test_idf_qemu.py b/tools/test_idf_py/test_idf_qemu.py index 27da2f511b..b7bbaf1e2f 100755 --- a/tools/test_idf_py/test_idf_qemu.py +++ b/tools/test_idf_py/test_idf_qemu.py @@ -8,7 +8,12 @@ import sys import tempfile import unittest -import pexpect +if sys.platform == 'win32': + import pytest + + pytest.skip('pexpect.spawn is not available on Windows', allow_module_level=True) +else: + import pexpect class IdfPyQemuTest(unittest.TestCase): From 36fb960e5f6e2a26e6b5a28d05394d5b428a62ad Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Thu, 18 Sep 2025 08:01:07 +0200 Subject: [PATCH 6/7] change(tools): Fix pre-commit checks for tools.py --- tools/idf_py_actions/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/idf_py_actions/tools.py b/tools/idf_py_actions/tools.py index d9694cb317..de6c236dd3 100644 --- a/tools/idf_py_actions/tools.py +++ b/tools/idf_py_actions/tools.py @@ -646,7 +646,7 @@ def ensure_build_directory( cache_path = os.path.join(build_dir, 'CMakeCache.txt') cache = _parse_cmakecache(cache_path) if os.path.exists(cache_path) else {} - args.define_cache_entry.append(f'CCACHE_ENABLE={args.ccache:d}') + args.define_cache_entry.append(f'CCACHE_ENABLE={args.ccache}') cache_cmdl = _parse_cmdl_cmakecache(args.define_cache_entry) From 5dad2a4884be921c8da063db45a18cb34632e727 Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Wed, 17 Sep 2025 13:23:29 +0200 Subject: [PATCH 7/7] ci(test): Improve test failures on Windows Tests are made cancellable and to print STDOUT and STDERR upon failures. --- tools/test_idf_py/test_hints.py | 40 ++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/tools/test_idf_py/test_hints.py b/tools/test_idf_py/test_hints.py index 74a5226238..110d5327a0 100755 --- a/tools/test_idf_py/test_hints.py +++ b/tools/test_idf_py/test_hints.py @@ -6,7 +6,9 @@ import os import sys import tempfile import unittest +import warnings from pathlib import Path +from subprocess import TimeoutExpired from subprocess import run import yaml @@ -27,6 +29,18 @@ except ImportError: from idf_py_actions.tools import generate_hints +def safe_cleanup_tmpdir(tmpdir: tempfile.TemporaryDirectory) -> None: + """Safely cleanup temporary directory, handling specific errors on Windows.""" + try: + tmpdir.cleanup() + except (PermissionError, NotADirectoryError): + warnings.warn( + f'Failed to cleanup temporary directory {tmpdir.name}. ' + 'This is common on Windows when files are still in use.', + UserWarning, + ) + + class TestHintsMassages(unittest.TestCase): def setUp(self) -> None: self.tmpdir = tempfile.TemporaryDirectory() @@ -43,14 +57,28 @@ class TestHintsMassages(unittest.TestCase): self.assertEqual(generated_hint, hint) def tearDown(self) -> None: - self.tmpdir.cleanup() + safe_cleanup_tmpdir(self.tmpdir) 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) + try: + proc = run(cmd + args, capture_output=True, cwd=cwd, text=True, timeout=10 * 60) + return str(proc.stdout + proc.stderr) + except TimeoutExpired as e: + # Print captured output on timeout to help with debugging + print(f'\n{"=" * 80}') + print(f'TEST TIMEOUT: idf.py {" ".join(args)} timed out') + print(f'{"=" * 80}') + if e.stdout: + print('CAPTURED STDOUT:') + print(e.stdout) + if e.stderr: + print('CAPTURED STDERR:') + print(e.stderr) + print(f'{"=" * 80}') + raise class TestHintModuleComponentRequirements(unittest.TestCase): @@ -114,7 +142,7 @@ class TestHintModuleComponentRequirements(unittest.TestCase): self.assertIn('To fix this, move esp_psram from PRIV_REQUIRES into REQUIRES', output) def tearDown(self) -> None: - self.tmpdir.cleanup() + safe_cleanup_tmpdir(self.tmpdir) class TestNestedModuleComponentRequirements(unittest.TestCase): @@ -161,7 +189,7 @@ class TestNestedModuleComponentRequirements(unittest.TestCase): 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() + safe_cleanup_tmpdir(self.tmpdir) class TestTrimmedModuleComponentRequirements(unittest.TestCase): @@ -193,7 +221,7 @@ class TestTrimmedModuleComponentRequirements(unittest.TestCase): ) def tearDown(self) -> None: - self.tmpdir.cleanup() + safe_cleanup_tmpdir(self.tmpdir) if __name__ == '__main__':