From e89f9801934e92b8e4d7387e830f4af6c917fddf Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Mon, 27 Feb 2023 17:50:24 +0100 Subject: [PATCH 1/6] tools: add _parse_cmdl_cmakecache() helper This parses cmakes cache vars defined on command line with -D options into dictionary. It allows to simplify the check for new cache entries and also can be re-used for other checks. Signed-off-by: Frantisek Hrbata --- tools/idf_py_actions/tools.py | 36 ++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/tools/idf_py_actions/tools.py b/tools/idf_py_actions/tools.py index f756fcce09..d82380dc2f 100644 --- a/tools/idf_py_actions/tools.py +++ b/tools/idf_py_actions/tools.py @@ -384,19 +384,27 @@ def _parse_cmakecache(path: str) -> Dict: return result -def _new_cmakecache_entries(cache_path: str, new_cache_entries: List) -> bool: - if not os.path.exists(cache_path): - return True +def _parse_cmdl_cmakecache(entries: List) -> Dict[str, str]: + """ + Parse list of CMake cache entries passed in via the -D option. - if new_cache_entries: - current_cache = _parse_cmakecache(cache_path) + Returns a dict of name:value. + """ + result: Dict = {} + for entry in entries: + key, value = entry.split('=', 1) + value = _strip_quotes(value) + result[key] = value - for entry in new_cache_entries: - key, value = entry.split('=', 1) - current_value = current_cache.get(key, None) - if current_value is None or _strip_quotes(value) != current_value: - return True + return result + +def _new_cmakecache_entries(cache: Dict, cache_cmdl: Dict) -> bool: + for entry in cache_cmdl: + if entry not in cache: + return True + if cache_cmdl[entry] != cache[entry]: + return True return False @@ -444,12 +452,14 @@ def ensure_build_directory(args: 'PropertyDict', prog_name: str, always_run_cmak 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('CCACHE_ENABLE=%d' % args.ccache) + + cache_cmdl = _parse_cmdl_cmakecache(args.define_cache_entry) + # Validate or set IDF_TARGET _guess_or_check_idf_target(args, prog_name, cache) - args.define_cache_entry.append('CCACHE_ENABLE=%d' % args.ccache) - - if always_run_cmake or _new_cmakecache_entries(cache_path, args.define_cache_entry): + if always_run_cmake or _new_cmakecache_entries(cache, cache_cmdl): if args.generator is None: args.generator = _detect_cmake_generator(prog_name) try: From a8a4d7c66d2a25f5e271f1eac789b814d07afd67 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Wed, 1 Mar 2023 15:30:29 +0100 Subject: [PATCH 2/6] tools: add get_sdkconfig_filename() helper Get project's current sdkconfig file name. It looks in SDKCONFIG cmake var defined by the -D option and project_description.json. If not found return default sdkconfig. Signed-off-by: Frantisek Hrbata --- tools/idf_py_actions/tools.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tools/idf_py_actions/tools.py b/tools/idf_py_actions/tools.py index d82380dc2f..e50a1e3a8f 100644 --- a/tools/idf_py_actions/tools.py +++ b/tools/idf_py_actions/tools.py @@ -1,6 +1,7 @@ # SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: Apache-2.0 import asyncio +import json import os import re import subprocess @@ -532,6 +533,27 @@ def merge_action_lists(*action_lists: Dict) -> Dict: return merged_actions +def get_sdkconfig_filename(args: 'PropertyDict', cache_cmdl: Dict=None) -> str: + """ + Get project's sdkconfig file name. + """ + if not cache_cmdl: + cache_cmdl = _parse_cmdl_cmakecache(args.define_cache_entry) + config = cache_cmdl.get('SDKCONFIG') + if config: + return os.path.abspath(config) + + proj_desc_path = os.path.join(args.build_dir, 'project_description.json') + try: + with open(proj_desc_path, 'r') as f: + proj_desc = json.load(f) + return str(proj_desc['config_file']) + except (OSError, KeyError): + pass + + return os.path.join(args.project_dir, 'sdkconfig') + + def get_sdkconfig_value(sdkconfig_file: str, key: str) -> Optional[str]: """ Return the value of given key from sdkconfig_file. From 0d859f2786b3a5b85a4d79f4ea758d75385bb887 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Thu, 23 Feb 2023 10:56:06 +0100 Subject: [PATCH 3/6] tools: move target guessing into cmake The _guess_or_check_idf_target() function has sdkconfig and sdkconfig.defaults file names hardcoded. Since config file names may be specified with SDKCONFIG or SDKCONFIG_DEFAULTS cmake vars, directly in CMakeLists.txt or passed in with the -D cmake option, they are not respected. Problem is when SDKCONFIG or SDKCONFIG_DEFAULTS is set in CMakeLists.txt. While idf can detect cmake vars passed through it to cmake via the -D option, detecting SDKCONFIG and SDKCONFIG_DEFAULTS vars settings in CMakeLists.txt would require to parse it. This seems like error prone approach. Also if the vars defined by the -D option are passed directly to cmake, not via idf, they will not be visible to idf. It seems reasonable to move the logic into cmake, where we know the correct SDKCONFIG and SDKCONFIG_DEFAULTS values. So the IDF_TARGET detection/guessing is moved into targets.cmake, where the IDF_TARGET is actually set. The target is guessed based on the following precendence. 1) $ENV{IDF_TARGET} 2) IDF_TARGET 3) SDKCONFIG 4) sdkconfig 5) SDKCONFIG_DEFAULTS if non-empty or $ENV{SDKCONFIG_DEFAULTS} if non-empty or sdkconfig.defaults 6) esp32 All config files referred in $ENV{SDKCONFIG_DEFAULTS} and SDKCONFIG_DEFAULTS are searched, compared to the current behaviour. First target found in the above chain is used. The original _guess_or_check_idf_target() is renamed to _check_idf_target() and used for the target consistency checks only. The get_sdkconfig_filename() helper is now used to get the sdkconfig file for consistency checks. It looks in SDKCONFIG specified with the -D option and project_description.json. With this change config full paths are reported in messages, so it's clear e.g. from which config the target was guessed from or which config has consistency problem. test_non_default_target.py was adjusted to this change and also new test for testing the IDF_TARGET guessing was added. Signed-off-by: Frantisek Hrbata --- tools/cmake/targets.cmake | 80 ++++++++++++++++++- tools/idf_py_actions/tools.py | 36 +++------ .../test_non_default_target.py | 52 +++++++++++- 3 files changed, 137 insertions(+), 31 deletions(-) diff --git a/tools/cmake/targets.cmake b/tools/cmake/targets.cmake index 23d3ff4848..ade74db9cc 100644 --- a/tools/cmake/targets.cmake +++ b/tools/cmake/targets.cmake @@ -1,3 +1,72 @@ +# +# Get target from single sdkconfig file +# +function(__target_from_config config target_out file_out) + set(${target_out} NOTFOUND PARENT_SCOPE) + set(${file_out} NOTFOUND PARENT_SCOPE) + + if(NOT EXISTS "${config}") + return() + endif() + + file(STRINGS "${config}" lines) + foreach(line ${lines}) + if(NOT "${line}" MATCHES "^CONFIG_IDF_TARGET=\"[^\"]+\"$") + continue() + endif() + + string(REGEX REPLACE "CONFIG_IDF_TARGET=\"([^\"]+)\"" "\\1" target "${line}") + set(${target_out} ${target} PARENT_SCOPE) + set(${file_out} ${config} PARENT_SCOPE) + return() + endforeach() +endfunction() + +# +# Get target from list of sdkconfig files +# +function(__target_from_configs configs target_out file_out) + set(target NOTFOUND) + set(file NOTFOUND) + + foreach(config ${configs}) + message(DEBUG "Searching for target in '${config}'") + get_filename_component(config "${config}" ABSOLUTE) + __target_from_config("${config}" target file) + if(target) + break() + endif() + endforeach() + + set(${target_out} ${target} PARENT_SCOPE) + set(${file_out} ${file} PARENT_SCOPE) +endfunction() + +# +# Search for target in config files in the following order. +# SDKCONFIG cmake var, default sdkconfig, SDKCONFIG_DEFAULTS cmake var +# if non-empty or SDKCONFIG_DEFAULTS env var if non-empty or +# sdkconfig.defaults. +# +function(__target_guess target_out file_out) + # Select sdkconfig_defaults to look for target + if(SDKCONFIG_DEFAULTS) + set(defaults "${SDKCONFIG_DEFAULTS}") + elseif(DEFINED ENV{SDKCONFIG_DEFAULTS}) + set(defaults "$ENV{SDKCONFIG_DEFAULTS}") + endif() + + if(NOT defaults) + set(defaults "${CMAKE_SOURCE_DIR}/sdkconfig.defaults") + endif() + + set(configs "${SDKCONFIG}" "${CMAKE_SOURCE_DIR}/sdkconfig" "${defaults}") + message(DEBUG "Searching for target in '${configs}'") + __target_from_configs("${configs}" target file) + set(${target_out} ${target} PARENT_SCOPE) + set(${file_out} ${file} PARENT_SCOPE) +endfunction() + # # Set the target used for the standard project build. # @@ -10,8 +79,15 @@ macro(__target_init) if(IDF_TARGET) set(env_idf_target ${IDF_TARGET}) else() - set(env_idf_target esp32) - message(STATUS "IDF_TARGET not set, using default target: ${env_idf_target}") + # Try to guess IDF_TARGET from sdkconfig files while honoring + # SDKCONFIG and SDKCONFIG_DEFAULTS values + __target_guess(env_idf_target where) + if(env_idf_target) + message(STATUS "IDF_TARGET is not set, guessed '${env_idf_target}' from sdkconfig '${where}'") + else() + set(env_idf_target esp32) + message(STATUS "IDF_TARGET not set, using default target: ${env_idf_target}") + endif() endif() else() # IDF_TARGET set both in environment and in cache, must be the same diff --git a/tools/idf_py_actions/tools.py b/tools/idf_py_actions/tools.py index e50a1e3a8f..1fb6889f14 100644 --- a/tools/idf_py_actions/tools.py +++ b/tools/idf_py_actions/tools.py @@ -457,8 +457,8 @@ def ensure_build_directory(args: 'PropertyDict', prog_name: str, always_run_cmak cache_cmdl = _parse_cmdl_cmakecache(args.define_cache_entry) - # Validate or set IDF_TARGET - _guess_or_check_idf_target(args, prog_name, cache) + # Validate IDF_TARGET + _check_idf_target(args, prog_name, cache, cache_cmdl) if always_run_cmake or _new_cmakecache_entries(cache, cache_cmdl): if args.generator is None: @@ -581,36 +581,22 @@ def is_target_supported(project_path: str, supported_targets: List) -> bool: return get_target(project_path) in supported_targets -def _guess_or_check_idf_target(args: 'PropertyDict', prog_name: str, cache: Dict) -> None: +def _check_idf_target(args: 'PropertyDict', prog_name: str, cache: Dict, cache_cmdl: Dict) -> None: """ - If CMakeCache.txt doesn't exist, and IDF_TARGET is not set in the environment, guess the value from - sdkconfig or sdkconfig.defaults, and pass it to CMake in IDF_TARGET variable. - - Otherwise, cross-check the three settings (sdkconfig, CMakeCache, environment) and if there is + Cross-check the three settings (sdkconfig, CMakeCache, environment) and if there is mismatch, fail with instructions on how to fix this. """ - # Default locations of sdkconfig files. - # FIXME: they may be overridden in the project or by a CMake variable (IDF-1369). - # These are used to guess the target from sdkconfig, or set the default target by sdkconfig.defaults. - idf_target_from_sdkconfig = get_target(args.project_dir) - idf_target_from_sdkconfig_defaults = get_target(args.project_dir, 'sdkconfig.defaults') + sdkconfig = get_sdkconfig_filename(args, cache_cmdl) + idf_target_from_sdkconfig = get_sdkconfig_value(sdkconfig, 'CONFIG_IDF_TARGET') idf_target_from_env = os.environ.get('IDF_TARGET') idf_target_from_cache = cache.get('IDF_TARGET') - if not cache and not idf_target_from_env: - # CMakeCache.txt does not exist yet, and IDF_TARGET is not set in the environment. - guessed_target = idf_target_from_sdkconfig or idf_target_from_sdkconfig_defaults - if guessed_target: - if args.verbose: - print("IDF_TARGET is not set, guessed '%s' from sdkconfig" % (guessed_target)) - args.define_cache_entry.append('IDF_TARGET=' + guessed_target) - - elif idf_target_from_env: + if idf_target_from_env: # Let's check that IDF_TARGET values are consistent if idf_target_from_sdkconfig and idf_target_from_sdkconfig != idf_target_from_env: - raise FatalError("Project sdkconfig was generated for target '{t_conf}', but environment variable IDF_TARGET " + raise FatalError("Project sdkconfig '{cfg}' was generated for target '{t_conf}', but environment variable IDF_TARGET " "is set to '{t_env}'. Run '{prog} set-target {t_env}' to generate new sdkconfig file for target {t_env}." - .format(t_conf=idf_target_from_sdkconfig, t_env=idf_target_from_env, prog=prog_name)) + .format(cfg=sdkconfig, t_conf=idf_target_from_sdkconfig, t_env=idf_target_from_env, prog=prog_name)) if idf_target_from_cache and idf_target_from_cache != idf_target_from_env: raise FatalError("Target settings are not consistent: '{t_env}' in the environment, '{t_cache}' in CMakeCache.txt. " @@ -619,10 +605,10 @@ def _guess_or_check_idf_target(args: 'PropertyDict', prog_name: str, cache: Dict elif idf_target_from_cache and idf_target_from_sdkconfig and idf_target_from_cache != idf_target_from_sdkconfig: # This shouldn't happen, unless the user manually edits CMakeCache.txt or sdkconfig, but let's check anyway. - raise FatalError("Project sdkconfig was generated for target '{t_conf}', but CMakeCache.txt contains '{t_cache}'. " + raise FatalError("Project sdkconfig '{cfg}' was generated for target '{t_conf}', but CMakeCache.txt contains '{t_cache}'. " "To keep the setting in sdkconfig ({t_conf}) and re-generate CMakeCache.txt, run '{prog} fullclean'. " "To re-generate sdkconfig for '{t_cache}' target, run '{prog} set-target {t_cache}'." - .format(t_conf=idf_target_from_sdkconfig, t_cache=idf_target_from_cache, prog=prog_name)) + .format(cfg=sdkconfig, t_conf=idf_target_from_sdkconfig, t_cache=idf_target_from_cache, prog=prog_name)) class TargetChoice(click.Choice): diff --git a/tools/test_build_system/test_non_default_target.py b/tools/test_build_system/test_non_default_target.py index 0d0cbe2af7..da661e8da1 100644 --- a/tools/test_build_system/test_non_default_target.py +++ b/tools/test_build_system/test_non_default_target.py @@ -8,6 +8,9 @@ from pathlib import Path import pytest from test_build_system_helpers import EnvDict, IdfPyFunc, check_file_contains, run_cmake +ESP32C3_TARGET = 'esp32c3' +ESP32C2_TARGET = 'esp32c2' +ESP32S3_TARGET = 'esp32s3' ESP32S2_TARGET = 'esp32s2' ESP32_TARGET = 'esp32' @@ -36,9 +39,12 @@ def test_target_from_environment_idf_py(idf_py: IdfPyFunc, default_idf_env: EnvD idf_py('set-target', ESP32S2_TARGET) default_idf_env.update({'IDF_TARGET': ESP32_TARGET}) + cfg_path = test_app_copy.joinpath('sdkconfig') + logging.info("idf.py fails if IDF_TARGET settings don't match the environment") - reconfigure_and_check_return_values("Project sdkconfig was generated for target '{}', but environment " - "variable IDF_TARGET is set to '{}'.".format(ESP32S2_TARGET, ESP32_TARGET)) + reconfigure_and_check_return_values("Project sdkconfig '{}' was generated for target '{}', but environment " + "variable IDF_TARGET is set to '{}'.".format(cfg_path, ESP32S2_TARGET, + ESP32_TARGET)) logging.info("idf.py fails if IDF_TARGET settings in CMakeCache.txt don't match the environment") (test_app_copy / 'sdkconfig').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32_TARGET)) @@ -47,8 +53,8 @@ def test_target_from_environment_idf_py(idf_py: IdfPyFunc, default_idf_env: EnvD logging.info("idf.py fails if IDF_TARGET settings in CMakeCache.txt don't match the sdkconfig") default_idf_env.pop('IDF_TARGET') - reconfigure_and_check_return_values("Project sdkconfig was generated for target '{}', but CMakeCache.txt " - "contains '{}'.".format(ESP32_TARGET, ESP32S2_TARGET)) + reconfigure_and_check_return_values("Project sdkconfig '{}' was generated for target '{}', but CMakeCache.txt " + "contains '{}'.".format(cfg_path, ESP32_TARGET, ESP32S2_TARGET)) def test_target_precedence(idf_py: IdfPyFunc, default_idf_env: EnvDict, test_app_copy: Path) -> None: @@ -102,3 +108,41 @@ def test_target_using_sdkconfig(idf_py: IdfPyFunc, test_app_copy: Path) -> None: idf_py('reconfigure') check_file_contains('sdkconfig', 'CONFIG_IDF_TARGET="{}"'.format(ESP32S2_TARGET)) check_file_contains('sdkconfig', 'CONFIG_IDF_TARGET_{}=y'.format(ESP32S2_TARGET.upper())) + + +def test_target_guessing(idf_py: IdfPyFunc, test_app_copy: Path, default_idf_env: EnvDict) -> None: + """ + Tests are performed from the lowest to the highest priority, while + configs, except from the sdkconfig, and parameters of previous tests are + preserved. + """ + + logging.info('Can guess target from sdkconfig.defaults') + (test_app_copy / 'sdkconfig.defaults').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32_TARGET)) + idf_py('reconfigure') + check_file_contains('sdkconfig', 'CONFIG_IDF_TARGET="{}"'.format(ESP32_TARGET)) + check_file_contains('build/CMakeCache.txt', 'IDF_TARGET:STRING={}'.format(ESP32_TARGET)) + + logging.info('Can guess target from SDKCONFIG_DEFAULTS environment variable') + (test_app_copy / 'sdkconfig1').write_text('NOTHING HERE') + (test_app_copy / 'sdkconfig2').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32S2_TARGET)) + clean_app(test_app_copy) + default_idf_env.update({'SDKCONFIG_DEFAULTS': 'sdkconfig1;sdkconfig2'}) + idf_py('reconfigure') + check_file_contains('sdkconfig', 'CONFIG_IDF_TARGET="{}"'.format(ESP32S2_TARGET)) + check_file_contains('build/CMakeCache.txt', 'IDF_TARGET:STRING={}'.format(ESP32S2_TARGET)) + + logging.info('Can guess target from SDKCONFIG_DEFAULTS using -D') + (test_app_copy / 'sdkconfig3').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32S2_TARGET)) + (test_app_copy / 'sdkconfig4').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32S3_TARGET)) + clean_app(test_app_copy) + idf_py('-D', 'SDKCONFIG_DEFAULTS=sdkconfig4;sdkconfig3', 'reconfigure') + check_file_contains('sdkconfig', 'CONFIG_IDF_TARGET="{}"'.format(ESP32S3_TARGET)) + check_file_contains('build/CMakeCache.txt', 'IDF_TARGET:STRING={}'.format(ESP32S3_TARGET)) + + logging.info('Can guess target from custom sdkconfig') + (test_app_copy / 'sdkconfig5').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32C3_TARGET)) + clean_app(test_app_copy) + idf_py('-D', 'SDKCONFIG=sdkconfig5', '-D', 'SDKCONFIG_DEFAULTS=sdkconfig4;sdkconfig3', 'reconfigure') + check_file_contains('sdkconfig5', 'CONFIG_IDF_TARGET="{}"'.format(ESP32C3_TARGET)) + check_file_contains('build/CMakeCache.txt', 'IDF_TARGET:STRING={}'.format(ESP32C3_TARGET)) From 8e912faad15cbc1501fe6332a54c02b9fdeb7c35 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Sat, 4 Mar 2023 10:39:32 +0100 Subject: [PATCH 4/6] tools: add target consistency checks for target specified with -D option Extend existing target consistency checks for the two following cases. 1. Target does not match currently used toolchain $ IDF_TARGET=esp32s2 idf.py reconfigure $ idf.py -DIDF_TARGET=esp32c3 build 2. Target is ambiguous, because it's specified also as env. var. IDF_TARGET=esp32s3 idf.py set-target esp32c2 Signed-off-by: Frantisek Hrbata --- tools/idf_py_actions/tools.py | 24 +++++++++++++++---- .../test_non_default_target.py | 21 +++++++++++++--- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/tools/idf_py_actions/tools.py b/tools/idf_py_actions/tools.py index 1fb6889f14..10fdd89b64 100644 --- a/tools/idf_py_actions/tools.py +++ b/tools/idf_py_actions/tools.py @@ -590,6 +590,7 @@ def _check_idf_target(args: 'PropertyDict', prog_name: str, cache: Dict, cache_c idf_target_from_sdkconfig = get_sdkconfig_value(sdkconfig, 'CONFIG_IDF_TARGET') idf_target_from_env = os.environ.get('IDF_TARGET') idf_target_from_cache = cache.get('IDF_TARGET') + idf_target_from_cache_cmdl = cache_cmdl.get('IDF_TARGET') if idf_target_from_env: # Let's check that IDF_TARGET values are consistent @@ -603,12 +604,25 @@ def _check_idf_target(args: 'PropertyDict', prog_name: str, cache: Dict, cache_c "Run '{prog} fullclean' to start again." .format(t_env=idf_target_from_env, t_cache=idf_target_from_cache, prog=prog_name)) - elif idf_target_from_cache and idf_target_from_sdkconfig and idf_target_from_cache != idf_target_from_sdkconfig: + if idf_target_from_cache_cmdl and idf_target_from_cache_cmdl != idf_target_from_env: + raise FatalError("Target '{t_cmdl}' specified on command line is not consistent with " + "target '{t_env}' in the environment." + .format(t_cmdl=idf_target_from_cache_cmdl, t_env=idf_target_from_env)) + elif idf_target_from_cache_cmdl: + # Check if -DIDF_TARGET is consistent with target in CMakeCache.txt + if idf_target_from_cache and idf_target_from_cache != idf_target_from_cache_cmdl: + raise FatalError("Target '{t_cmdl}' specified on command line is not consistent with " + "target '{t_cache}' in CMakeCache.txt. Run '{prog} set-target {t_cmdl}' to re-generate " + 'CMakeCache.txt.' + .format(t_cache=idf_target_from_cache, t_cmdl=idf_target_from_cache_cmdl, prog=prog_name)) + + elif idf_target_from_cache: # This shouldn't happen, unless the user manually edits CMakeCache.txt or sdkconfig, but let's check anyway. - raise FatalError("Project sdkconfig '{cfg}' was generated for target '{t_conf}', but CMakeCache.txt contains '{t_cache}'. " - "To keep the setting in sdkconfig ({t_conf}) and re-generate CMakeCache.txt, run '{prog} fullclean'. " - "To re-generate sdkconfig for '{t_cache}' target, run '{prog} set-target {t_cache}'." - .format(cfg=sdkconfig, t_conf=idf_target_from_sdkconfig, t_cache=idf_target_from_cache, prog=prog_name)) + if idf_target_from_sdkconfig and idf_target_from_cache != idf_target_from_sdkconfig: + raise FatalError("Project sdkconfig '{cfg}' was generated for target '{t_conf}', but CMakeCache.txt contains '{t_cache}'. " + "To keep the setting in sdkconfig ({t_conf}) and re-generate CMakeCache.txt, run '{prog} fullclean'. " + "To re-generate sdkconfig for '{t_cache}' target, run '{prog} set-target {t_cache}'." + .format(cfg=sdkconfig, t_conf=idf_target_from_sdkconfig, t_cache=idf_target_from_cache, prog=prog_name)) class TargetChoice(click.Choice): diff --git a/tools/test_build_system/test_non_default_target.py b/tools/test_build_system/test_non_default_target.py index da661e8da1..e55812acb2 100644 --- a/tools/test_build_system/test_non_default_target.py +++ b/tools/test_build_system/test_non_default_target.py @@ -4,6 +4,7 @@ import logging import shutil from pathlib import Path +from typing import List, Optional import pytest from test_build_system_helpers import EnvDict, IdfPyFunc, check_file_contains, run_cmake @@ -31,15 +32,16 @@ def test_target_from_environment_cmake(default_idf_env: EnvDict) -> None: def test_target_from_environment_idf_py(idf_py: IdfPyFunc, default_idf_env: EnvDict, test_app_copy: Path) -> None: - def reconfigure_and_check_return_values(errmsg: str) -> None: - ret = idf_py('reconfigure', check=False) + def reconfigure_and_check_return_values(errmsg: str, opts: Optional[List[str]] = None) -> None: + opts = opts or [] + ret = idf_py(*opts, 'reconfigure', check=False) assert ret.returncode == 2 assert errmsg in ret.stderr idf_py('set-target', ESP32S2_TARGET) default_idf_env.update({'IDF_TARGET': ESP32_TARGET}) - cfg_path = test_app_copy.joinpath('sdkconfig') + cfg_path = (test_app_copy / 'sdkconfig') logging.info("idf.py fails if IDF_TARGET settings don't match the environment") reconfigure_and_check_return_values("Project sdkconfig '{}' was generated for target '{}', but environment " @@ -56,6 +58,19 @@ def test_target_from_environment_idf_py(idf_py: IdfPyFunc, default_idf_env: EnvD reconfigure_and_check_return_values("Project sdkconfig '{}' was generated for target '{}', but CMakeCache.txt " "contains '{}'.".format(cfg_path, ESP32_TARGET, ESP32S2_TARGET)) + logging.info('idf.py fails if IDF_TARGET is set differently in environment and with -D option') + (test_app_copy / 'sdkconfig').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32S2_TARGET)) + default_idf_env.update({'IDF_TARGET': ESP32S2_TARGET}) + reconfigure_and_check_return_values("Target '{}' specified on command line is not consistent with target '{}' " + 'in the environment.'.format(ESP32_TARGET, ESP32S2_TARGET), + ['-D', 'IDF_TARGET={}'.format(ESP32_TARGET)]) + + logging.info('idf.py fails if IDF_TARGET is set differently in CMakeCache.txt and with -D option') + default_idf_env.pop('IDF_TARGET') + reconfigure_and_check_return_values("Target '{}' specified on command line is not consistent with " + "target '{}' in CMakeCache.txt.".format(ESP32_TARGET, ESP32S2_TARGET), + ['-D', 'IDF_TARGET={}'.format(ESP32_TARGET)]) + def test_target_precedence(idf_py: IdfPyFunc, default_idf_env: EnvDict, test_app_copy: Path) -> None: logging.info('IDF_TARGET takes precedence over the value of CONFIG_IDF_TARGET in sdkconfig.defaults') From 1ca9e63e79e38ef2d3e6be9ca31725625973c844 Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Tue, 28 Feb 2023 18:15:12 +0100 Subject: [PATCH 5/6] tools: add target consistency checks to cmake Extend target checks in cmake, in case it's run directly and not via idf.py or if idf.py misses something. This may happen for example if cmake variables are set in project's CMakeLists.txt. Some clean-ups are included along with the new checks and tests. 1. __target_check() function is removed. IIUC it should never fail, because the selected target is explicitly passed as environmental variable to kconfgen. Meaning the IDF_TARGET from environment variable may not be actually used in kconfgen if IDF_TARGET is already set it cmake cache. Note that the IDF_TARGET environment variable used for kconfgen is not based on the actual IDF_TARGET environment variable set for idf.py, but rather on the value set in __target_init() with set(IDF_TARGET ${env_idf_target} CACHE STRING "IDF Build Target") My understanding is that the original check was introduced to handle situation, where IDF_TARGET was already set in cmake's cache and the IDF_TARGET from environment variable was different. Since the kconfgen would use the original environment variable(not explicitly passed as it is now) the IDF_TARGET in cmake and in sdkconfig could differ. IOW I think the original check was introduced to cope with the following cmake behaviour set(VARIABLE "value1" CACHE STRING "test variable") set(VARIABLE "value2" CACHE STRING "test variable") message("Variable value: ${VARIABLE}") output: Variable value: value1 2. I scratched by head how it is possible that the following code in __target_check() if(NOT ${IDF_TARGET} STREQUAL ${env_idf_target}) could fail if IDF_TARGET is not set. For example in clean project IDF_TARGET=esp32 idf.py reconfigure Here env_idf_target==esp32 and IDF_TARGET is not set, so I would expect that cmake will fail with error message that the cache and env target do not match. The thing is that the variable evaluation is done before the if command, so it actually sees this if(NOT STREQUAL esp32) which is false and the error is not printed. It can be seen with 'cmake --trace-expand' command. I don't know if this was used on purpose or it worked just as a coincidence, but I find it very confusing, so I added explicit check if the IDF_TARGET is defined before the actual check. Same for CMAKE_TOOLCHAIN_FILE. 3. Error messages are not formated(line-wrapped) by cmake's markup so it's easier to check the output in tests. Signed-off-by: Frantisek Hrbata --- tools/cmake/build.cmake | 3 - tools/cmake/targets.cmake | 69 ++++++++++++------- .../test_build_system_helpers/idf_utils.py | 8 ++- .../test_non_default_target.py | 36 ++++++++++ 4 files changed, 85 insertions(+), 31 deletions(-) diff --git a/tools/cmake/build.cmake b/tools/cmake/build.cmake index 3968dae1de..2f74364ca6 100644 --- a/tools/cmake/build.cmake +++ b/tools/cmake/build.cmake @@ -383,9 +383,6 @@ macro(__build_process_project_includes) set(${build_property} "${val}") endforeach() - # Check that the CMake target value matches the Kconfig target value. - __target_check() - idf_build_get_property(build_component_targets __BUILD_COMPONENT_TARGETS) # Include each component's project_include.cmake diff --git a/tools/cmake/targets.cmake b/tools/cmake/targets.cmake index ade74db9cc..75b466f34c 100644 --- a/tools/cmake/targets.cmake +++ b/tools/cmake/targets.cmake @@ -89,35 +89,42 @@ macro(__target_init) message(STATUS "IDF_TARGET not set, using default target: ${env_idf_target}") endif() endif() - else() - # IDF_TARGET set both in environment and in cache, must be the same - if(NOT ${IDF_TARGET} STREQUAL ${env_idf_target}) - message(FATAL_ERROR "IDF_TARGET in CMake cache does not match " - "IDF_TARGET environment variable. To change the target, clear " - "the build directory and sdkconfig file, and build the project again") + endif() + + # Check if selected target is consistent with CMake cache + if(DEFINED CACHE{IDF_TARGET}) + if(NOT $CACHE{IDF_TARGET} STREQUAL ${env_idf_target}) + message(FATAL_ERROR " IDF_TARGET '$CACHE{IDF_TARGET}' in CMake" + " cache does not match currently selected IDF_TARGET '${env_idf_target}'." + " To change the target, clear the build directory and sdkconfig file," + " and build the project again.") endif() endif() - # IDF_TARGET will be used by Kconfig, make sure it is set + if(SDKCONFIG) + get_filename_component(sdkconfig "${SDKCONFIG}" ABSOLUTE) + else() + set(sdkconfig "${CMAKE_SOURCE_DIR}/sdkconfig") + endif() + + # Check if selected target is consistent with sdkconfig + __target_from_config("${sdkconfig}" sdkconfig_target where) + if(sdkconfig_target) + if(NOT ${sdkconfig_target} STREQUAL ${env_idf_target}) + message(FATAL_ERROR " Target '${sdkconfig_target}' in sdkconfig '${where}'" + " does not match currently selected IDF_TARGET '${IDF_TARGET}'." + " To change the target, clear the build directory and sdkconfig file," + " and build the project again.") + endif() + endif() + + # IDF_TARGET will be used by component manager, make sure it is set set(ENV{IDF_TARGET} ${env_idf_target}) # Finally, set IDF_TARGET in cache set(IDF_TARGET ${env_idf_target} CACHE STRING "IDF Build Target") endmacro() -# -# Check that the set build target and the config target matches. -# -function(__target_check) - # Should be called after sdkconfig CMake file has been included. - idf_build_get_property(idf_target IDF_TARGET) - if(NOT ${idf_target} STREQUAL ${CONFIG_IDF_TARGET}) - message(FATAL_ERROR "CONFIG_IDF_TARGET in sdkconfig does not match " - "IDF_TARGET environment variable. To change the target, delete " - "sdkconfig file and build the project again.") - endif() -endfunction() - # # Used by the project CMake file to set the toolchain before project() call. # @@ -133,12 +140,12 @@ macro(__target_set_toolchain) else() set(env_idf_toolchain gcc) endif() - else() + elseif(DEFINED CACHE{IDF_TOOLCHAIN}) # IDF_TOOLCHAIN set both in environment and in cache, must be the same - if(NOT ${IDF_TOOLCHAIN} STREQUAL ${env_idf_toolchain}) - message(FATAL_ERROR "IDF_TOOLCHAIN in CMake cache does not match " - "IDF_TOOLCHAIN environment variable. To change the toolchain, clear " - "the build directory and sdkconfig file, and build the project again") + if(NOT $CACHE{IDF_TOOLCHAIN} STREQUAL ${env_idf_toolchain}) + message(FATAL_ERROR " IDF_TOOLCHAIN '$CACHE{IDF_TOOLCHAIN}' in CMake cache does not match" + " currently selected IDF_TOOLCHAIN '${env_idf_toolchain}'. To change the toolchain, clear" + " the build directory and sdkconfig file, and build the project again.") endif() endif() @@ -149,6 +156,18 @@ macro(__target_set_toolchain) set(toolchain_type "clang-") endif() + # Check if selected target is consistent with toolchain file in CMake cache + if(DEFINED CMAKE_TOOLCHAIN_FILE) + string(FIND "${CMAKE_TOOLCHAIN_FILE}" "-${toolchain_type}${IDF_TARGET}.cmake" found) + if(${found} EQUAL -1) + get_filename_component(toolchain "${CMAKE_TOOLCHAIN_FILE}" NAME_WE) + message(FATAL_ERROR " CMAKE_TOOLCHAIN_FILE '${toolchain}'" + " does not match currently selected IDF_TARGET '${IDF_TARGET}'." + " To change the target, clear the build directory and sdkconfig file," + " and build the project again.") + endif() + endif() + # First try to load the toolchain file from the tools/cmake/directory of IDF set(toolchain_file_global ${idf_path}/tools/cmake/toolchain-${toolchain_type}${IDF_TARGET}.cmake) if(EXISTS ${toolchain_file_global}) diff --git a/tools/test_build_system/test_build_system_helpers/idf_utils.py b/tools/test_build_system/test_build_system_helpers/idf_utils.py index 61d347536a..7578c17ee0 100644 --- a/tools/test_build_system/test_build_system_helpers/idf_utils.py +++ b/tools/test_build_system/test_build_system_helpers/idf_utils.py @@ -94,7 +94,8 @@ def run_idf_py(*args: str, text=True, encoding='utf-8', errors='backslashreplace') -def run_cmake(*cmake_args: str, env: typing.Optional[EnvDict] = None) -> None: +def run_cmake(*cmake_args: str, env: typing.Optional[EnvDict] = None, + check: bool = True) -> subprocess.CompletedProcess: """ Run cmake command with given arguments, raise an exception on failure :param cmake_args: arguments to pass cmake @@ -108,9 +109,10 @@ def run_cmake(*cmake_args: str, env: typing.Optional[EnvDict] = None) -> None: cmd = ['cmake'] + list(cmake_args) logging.debug('running {} in {}'.format(' '.join(cmd), workdir)) - subprocess.check_call( + return subprocess.run( cmd, env=env, cwd=workdir, - stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + check=check, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + text=True, encoding='utf-8', errors='backslashreplace') def check_file_contains(filename: Union[str, Path], what: Union[str, Pattern]) -> None: diff --git a/tools/test_build_system/test_non_default_target.py b/tools/test_build_system/test_non_default_target.py index e55812acb2..d7a3988b88 100644 --- a/tools/test_build_system/test_non_default_target.py +++ b/tools/test_build_system/test_non_default_target.py @@ -72,6 +72,42 @@ def test_target_from_environment_idf_py(idf_py: IdfPyFunc, default_idf_env: EnvD ['-D', 'IDF_TARGET={}'.format(ESP32_TARGET)]) +def test_target_consistency_cmake(default_idf_env: EnvDict, test_app_copy: Path) -> None: + def reconfigure_and_check_return_values(errmsg: str, opts: Optional[List[str]] = None) -> None: + opts = opts or [] + ret = run_cmake(*opts, '-G', 'Ninja', '..', env=default_idf_env, check=False) + assert ret.returncode == 1 + assert errmsg in ret.stderr + + run_cmake('-G', 'Ninja', '..') + + cfg_path = (test_app_copy / 'sdkconfig') + + logging.info("cmake fails if IDF_TARGET settings don't match the environment") + default_idf_env.update({'IDF_TARGET': ESP32S2_TARGET}) + reconfigure_and_check_return_values(f"IDF_TARGET '{ESP32_TARGET}' in CMake cache does not " + f"match currently selected IDF_TARGET '{ESP32S2_TARGET}'") + + logging.info("cmake fails if IDF_TARGET settings don't match the sdkconfig") + default_idf_env.pop('IDF_TARGET') + (test_app_copy / 'sdkconfig').write_text(f'CONFIG_IDF_TARGET="{ESP32S2_TARGET}"') + reconfigure_and_check_return_values(f"Target '{ESP32S2_TARGET}' in sdkconfig '{cfg_path}' does not " + f"match currently selected IDF_TARGET '{ESP32_TARGET}'.") + + logging.info("cmake fails if IDF_TOOLCHAIN settings don't match the environment") + (test_app_copy / 'sdkconfig').write_text(f'CONFIG_IDF_TARGET="{ESP32_TARGET}"') + default_idf_env.update({'IDF_TOOLCHAIN': 'clang'}) + reconfigure_and_check_return_values("IDF_TOOLCHAIN 'gcc' in CMake cache does not match " + "currently selected IDF_TOOLCHAIN 'clang'") + + logging.info("cmake fails if IDF_TARGET settings don't match CMAKE_TOOLCHAIN_FILE") + default_idf_env.pop('IDF_TOOLCHAIN') + reconfigure_and_check_return_values("CMAKE_TOOLCHAIN_FILE 'toolchain-esp32' does not " + f"match currently selected IDF_TARGET '{ESP32S2_TARGET}'", + ['-D', f'IDF_TARGET={ESP32S2_TARGET}', + '-D', 'SDKCONFIG=custom_sdkconfig']) + + def test_target_precedence(idf_py: IdfPyFunc, default_idf_env: EnvDict, test_app_copy: Path) -> None: logging.info('IDF_TARGET takes precedence over the value of CONFIG_IDF_TARGET in sdkconfig.defaults') (test_app_copy / 'sdkconfig.defaults').write_text('CONFIG_IDF_TARGET="{}"'.format(ESP32S2_TARGET)) From a558ad506e8ae8999e1c37621648638419c5139b Mon Sep 17 00:00:00 2001 From: Frantisek Hrbata Date: Mon, 6 Mar 2023 13:52:29 +0100 Subject: [PATCH 6/6] tools: fix custom sdkconfig renaming in set-target Currently the set-target has sdkconfig file name hardcoded to the default one and doesn't honor custom config paths or names. IMHO the only place where we can really now the config file name is in cmake. But also the config should be really renamed only if the set-target action is running. This moves the config file renaming into cmake and it's performed only when _IDF_PY_SET_TARGET_ACTION env. var. is set to 'ON'. This should hopefully guarantee that it's really renamed only while set-target is running. Signed-off-by: Frantisek Hrbata --- tools/cmake/project.cmake | 29 ++++++++++++++++++++--------- tools/cmake/targets.cmake | 10 ++-------- tools/idf_py_actions/core_ext.py | 11 +++-------- tools/idf_py_actions/tools.py | 5 +++-- 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/tools/cmake/project.cmake b/tools/cmake/project.cmake index 00d94c8a6a..469a573488 100644 --- a/tools/cmake/project.cmake +++ b/tools/cmake/project.cmake @@ -1,12 +1,29 @@ # Designed to be included from an IDF app's CMakeLists.txt file cmake_minimum_required(VERSION 3.16) +# Get the currently selected sdkconfig file early, so this doesn't +# have to be done multiple times on different places. +if(SDKCONFIG) + get_filename_component(sdkconfig "${SDKCONFIG}" ABSOLUTE) +else() + set(sdkconfig "${CMAKE_SOURCE_DIR}/sdkconfig") +endif() + +# Check if the cmake was started as part of the set-target action. +# If so, check for existing sdkconfig file and rename it. +# This is done before __target_init, so the existing IDF_TARGET from sdkconfig +# is not considered for consistence checking. +if("$ENV{_IDF_PY_SET_TARGET_ACTION}" EQUAL "1" AND EXISTS "${sdkconfig}") + file(RENAME "${sdkconfig}" "${sdkconfig}.old") + message(STATUS "Existing sdkconfig '${sdkconfig}' renamed to '${sdkconfig}.old'.") +endif() + include(${CMAKE_CURRENT_LIST_DIR}/targets.cmake) # Initialize build target for this build using the environment variable or # value passed externally. -__target_init() +__target_init("${sdkconfig}") -# The mere inclusion of this CMake file sets up some interal build properties. +# The mere inclusion of this CMake file sets up some internal build properties. # These properties can be modified in between this inclusion the the idf_build_process # call. include(${CMAKE_CURRENT_LIST_DIR}/idf.cmake) @@ -316,7 +333,7 @@ function(__project_init components_var test_components_var) set(${test_components_var} "${test_components}" PARENT_SCOPE) endfunction() -# Trick to temporarily redefine project(). When functions are overriden in CMake, the originals can still be accessed +# Trick to temporarily redefine project(). When functions are overridden in CMake, the originals can still be accessed # using an underscore prefixed function of the same name. The following lines make sure that __project calls # the original project(). See https://cmake.org/pipermail/cmake/2015-October/061751.html. function(project) @@ -431,12 +448,6 @@ macro(project project_name) list(APPEND sdkconfig_defaults ${sdkconfig_default}) endforeach() - if(SDKCONFIG) - get_filename_component(sdkconfig "${SDKCONFIG}" ABSOLUTE) - else() - set(sdkconfig "${CMAKE_CURRENT_LIST_DIR}/sdkconfig") - endif() - if(BUILD_DIR) get_filename_component(build_dir "${BUILD_DIR}" ABSOLUTE) if(NOT EXISTS "${build_dir}") diff --git a/tools/cmake/targets.cmake b/tools/cmake/targets.cmake index 75b466f34c..e4c3f195b1 100644 --- a/tools/cmake/targets.cmake +++ b/tools/cmake/targets.cmake @@ -70,7 +70,7 @@ endfunction() # # Set the target used for the standard project build. # -macro(__target_init) +macro(__target_init config_file) # Input is IDF_TARGET environement variable set(env_idf_target $ENV{IDF_TARGET}) @@ -101,14 +101,8 @@ macro(__target_init) endif() endif() - if(SDKCONFIG) - get_filename_component(sdkconfig "${SDKCONFIG}" ABSOLUTE) - else() - set(sdkconfig "${CMAKE_SOURCE_DIR}/sdkconfig") - endif() - # Check if selected target is consistent with sdkconfig - __target_from_config("${sdkconfig}" sdkconfig_target where) + __target_from_config("${config_file}" sdkconfig_target where) if(sdkconfig_target) if(NOT ${sdkconfig_target} STREQUAL ${env_idf_target}) message(FATAL_ERROR " Target '${sdkconfig_target}' in sdkconfig '${where}'" diff --git a/tools/idf_py_actions/core_ext.py b/tools/idf_py_actions/core_ext.py index 7fb66556ba..3068130d64 100644 --- a/tools/idf_py_actions/core_ext.py +++ b/tools/idf_py_actions/core_ext.py @@ -155,14 +155,9 @@ def action_extensions(base_actions: Dict, project_path: str) -> Any: "%s is still in preview. You have to append '--preview' option after idf.py to use any preview feature." % idf_target) args.define_cache_entry.append('IDF_TARGET=' + idf_target) - sdkconfig_path = os.path.join(args.project_dir, 'sdkconfig') - sdkconfig_old = sdkconfig_path + '.old' - if os.path.exists(sdkconfig_old): - os.remove(sdkconfig_old) - if os.path.exists(sdkconfig_path): - os.rename(sdkconfig_path, sdkconfig_old) - print('Set Target to: %s, new sdkconfig created. Existing sdkconfig renamed to sdkconfig.old.' % idf_target) - ensure_build_directory(args, ctx.info_name, True) + print(f'Set Target to: {idf_target}, new sdkconfig will be created.') + env = {'_IDF_PY_SET_TARGET_ACTION': '1'} + ensure_build_directory(args, ctx.info_name, True, env) def reconfigure(action: str, ctx: Context, args: PropertyDict) -> None: ensure_build_directory(args, ctx.info_name, True) diff --git a/tools/idf_py_actions/tools.py b/tools/idf_py_actions/tools.py index 10fdd89b64..f2c546f8fa 100644 --- a/tools/idf_py_actions/tools.py +++ b/tools/idf_py_actions/tools.py @@ -419,7 +419,8 @@ def _detect_cmake_generator(prog_name: str) -> Any: raise FatalError("To use %s, either the 'ninja' or 'GNU make' build tool must be available in the PATH" % prog_name) -def ensure_build_directory(args: 'PropertyDict', prog_name: str, always_run_cmake: bool=False) -> None: +def ensure_build_directory(args: 'PropertyDict', prog_name: str, always_run_cmake: bool=False, + env: Dict=None) -> None: """Check the build directory exists and that cmake has been run there. If this isn't the case, create the build directory (if necessary) and @@ -480,7 +481,7 @@ def ensure_build_directory(args: 'PropertyDict', prog_name: str, always_run_cmak cmake_args += [project_dir] hints = not args.no_hints - RunTool('cmake', cmake_args, cwd=args.build_dir, hints=hints)() + RunTool('cmake', cmake_args, cwd=args.build_dir, env=env, hints=hints)() except Exception: # don't allow partially valid CMakeCache.txt files, # to keep the "should I run cmake?" logic simple