From 636ce4750faeab2bbb2a3ee83e6b7c2277762450 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 6 Oct 2020 00:23:20 +0200 Subject: [PATCH 1/7] ldgen: remove unused variables --- tools/cmake/ldgen.cmake | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tools/cmake/ldgen.cmake b/tools/cmake/ldgen.cmake index 1e4341e8b3..d8c5d0eb7d 100644 --- a/tools/cmake/ldgen.cmake +++ b/tools/cmake/ldgen.cmake @@ -46,14 +46,9 @@ function(__ldgen_process_template template output) # Create command to invoke the linker script generator tool. idf_build_get_property(sdkconfig SDKCONFIG) idf_build_get_property(root_kconfig __ROOT_KCONFIG) - idf_build_get_property(kconfigs KCONFIGS) - idf_build_get_property(kconfig_projbuilds KCONFIG_PROJBUILDS) idf_build_get_property(python PYTHON) - string(REPLACE ";" " " kconfigs "${kconfigs}") - string(REPLACE ";" " " kconfig_projbuilds "${kconfig_projbuilds}") - idf_build_get_property(config_env_path CONFIG_ENV_PATH) if($ENV{LDGEN_CHECK_MAPPING}) From 8da98b864d93a0087095b3a22ce67fa9a979d8a9 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 6 Oct 2020 00:13:55 +0200 Subject: [PATCH 2/7] build system: pass semicolon-separated directory lists to kconfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New —-list-separator argument of confgen.py and prepare_kconfig_files.py is used to select which character is used as list separator. For compatibility with esp-docs, we still keep support for space separator. Otherwise esp-docs would have to choose the separator depending on the IDF version. --- tools/ci/check_copyright_ignore.txt | 1 - tools/cmake/kconfig.cmake | 6 +-- tools/kconfig_new/confgen.py | 10 ++++- tools/kconfig_new/confserver.py | 8 ++-- tools/kconfig_new/prepare_kconfig_files.py | 45 +++++++++++++--------- 5 files changed, 42 insertions(+), 28 deletions(-) diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 21c47bfee4..8ff0808175 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -3208,7 +3208,6 @@ tools/idf_py_actions/tools.py tools/idf_py_actions/uf2_ext.py tools/kconfig_new/confserver.py tools/kconfig_new/gen_kconfig_doc.py -tools/kconfig_new/prepare_kconfig_files.py tools/kconfig_new/test/confgen/test_confgen.py tools/kconfig_new/test/confserver/test_confserver.py tools/kconfig_new/test/gen_kconfig_doc/test_kconfig_out.py diff --git a/tools/cmake/kconfig.cmake b/tools/cmake/kconfig.cmake index 40e425e258..1a1b4e05c9 100644 --- a/tools/cmake/kconfig.cmake +++ b/tools/cmake/kconfig.cmake @@ -94,10 +94,6 @@ function(__kconfig_generate_config sdkconfig sdkconfig_defaults) idf_build_get_property(idf_path IDF_PATH) idf_build_get_property(idf_env_fpga __IDF_ENV_FPGA) - string(REPLACE ";" " " kconfigs "${kconfigs}") - string(REPLACE ";" " " kconfig_projbuilds "${kconfig_projbuilds}") - string(REPLACE ";" " " sdkconfig_renames "${sdkconfig_renames}") - # These are the paths for files which will contain the generated "source" lines for COMPONENT_KCONFIGS and # COMPONENT_KCONFIGS_PROJBUILD set(kconfigs_projbuild_path "${CMAKE_CURRENT_BINARY_DIR}/kconfigs_projbuild.in") @@ -130,10 +126,12 @@ function(__kconfig_generate_config sdkconfig sdkconfig_defaults) set(prepare_kconfig_files_command ${python} ${idf_path}/tools/kconfig_new/prepare_kconfig_files.py + --list-separator=semicolon --env-file ${config_env_path}) set(confgen_basecommand ${python} ${idf_path}/tools/kconfig_new/confgen.py + --list-separator=semicolon --kconfig ${root_kconfig} --sdkconfig-rename ${root_sdkconfig_rename} --config ${sdkconfig} diff --git a/tools/kconfig_new/confgen.py b/tools/kconfig_new/confgen.py index 4e9d1e9618..e6b7568bf1 100755 --- a/tools/kconfig_new/confgen.py +++ b/tools/kconfig_new/confgen.py @@ -223,6 +223,10 @@ def main(): help='Optional file to load environment variables from. Contents ' 'should be a JSON object where each key/value pair is a variable.') + parser.add_argument('--list-separator', choices=['space', 'semicolon'], + default='space', + help='Separator used in environment list variables (COMPONENT_SDKCONFIG_RENAMES)') + args = parser.parse_args() for fmt, filename in args.output: @@ -247,8 +251,12 @@ def main(): config.warn_assign_redun = False config.warn_assign_override = False + sdkconfig_renames_sep = ';' if args.list_separator == 'semicolon' else ' ' + sdkconfig_renames = [args.sdkconfig_rename] if args.sdkconfig_rename else [] - sdkconfig_renames += os.environ.get('COMPONENT_SDKCONFIG_RENAMES', '').split() + sdkconfig_renames_from_env = os.environ.get('COMPONENT_SDKCONFIG_RENAMES') + if sdkconfig_renames_from_env: + sdkconfig_renames += sdkconfig_renames_from_env.split(sdkconfig_renames_sep) deprecated_options = DeprecatedOptions(config.config_prefix, path_rename_files=sdkconfig_renames) if len(args.defaults) > 0: diff --git a/tools/kconfig_new/confserver.py b/tools/kconfig_new/confserver.py index 80b0e113ee..b0aed9aa35 100755 --- a/tools/kconfig_new/confserver.py +++ b/tools/kconfig_new/confserver.py @@ -74,7 +74,9 @@ def main(): def run_server(kconfig, sdkconfig, sdkconfig_rename, default_version=MAX_PROTOCOL_VERSION): config = kconfiglib.Kconfig(kconfig) sdkconfig_renames = [sdkconfig_rename] if sdkconfig_rename else [] - sdkconfig_renames += os.environ.get('COMPONENT_SDKCONFIG_RENAMES', '').split() + sdkconfig_renames_from_env = os.environ.get('COMPONENT_SDKCONFIG_RENAMES') + if sdkconfig_renames_from_env: + sdkconfig_renames += sdkconfig_renames_from_env.split(';') deprecated_options = confgen.DeprecatedOptions(config.config_prefix, path_rename_files=sdkconfig_renames) f_o = tempfile.NamedTemporaryFile(mode='w+b', delete=False) try: @@ -157,8 +159,8 @@ def run_server(kconfig, sdkconfig, sdkconfig_rename, default_version=MAX_PROTOCO # V2+ response, separate visibility values response = {'version': req['version'], 'values': values_diff, 'ranges': ranges_diff, 'visible': visible_diff} if error: - for e in error: - print('Error: %s' % e, file=sys.stderr) + for err in error: + print('Error: %s' % err, file=sys.stderr) response['error'] = error json.dump(response, sys.stdout) print('\n') diff --git a/tools/kconfig_new/prepare_kconfig_files.py b/tools/kconfig_new/prepare_kconfig_files.py index 1198467c3b..6ab38f6e43 100644 --- a/tools/kconfig_new/prepare_kconfig_files.py +++ b/tools/kconfig_new/prepare_kconfig_files.py @@ -1,18 +1,7 @@ #!/usr/bin/env python # -# Copyright 2019 Espressif Systems (Shanghai) PTE LTD -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http:#www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. +# SPDX-FileCopyrightText: 2019-2021 Espressif Systems (Shanghai) CO LTD +# SPDX-License-Identifier: Apache-2.0 from __future__ import print_function, unicode_literals @@ -22,7 +11,7 @@ import sys from io import open -def _prepare_source_files(env_dict): +def _prepare_source_files(env_dict, list_separator): """ Prepares source files which are sourced from the main Kconfig because upstream kconfiglib doesn't support sourcing a file list. The inputs are the same environment variables which are used by kconfiglib: @@ -37,18 +26,27 @@ def _prepare_source_files(env_dict): After running this function, COMPONENT_KCONFIGS_SOURCE_FILE and COMPONENT_KCONFIGS_PROJBUILD_SOURCE_FILE will contain a list of source statements based on the content of COMPONENT_KCONFIGS and COMPONENT_KCONFIGS_PROJBUILD, - respectively. For example, if COMPONENT_KCONFIGS="var1 var2 var3" and + respectively. For example, if COMPONENT_KCONFIGS="var1;var2;var3" and COMPONENT_KCONFIGS_SOURCE_FILE="/path/file.txt" then the content of file /path/file.txt will be: source "var1" source "var2" source "var3" + + The character used to delimit paths in COMPONENT_KCONFIGS* variables is determined based on + presence of 'IDF_CMAKE' variable in the env_dict. + GNU Make build system uses a space, CMake build system uses a semicolon. """ def _dequote(var): return var[1:-1] if len(var) > 0 and (var[0], var[-1]) == ('"',) * 2 else var def _write_source_file(config_var, config_file): - new_content = '\n'.join(['source "{}"'.format(path) for path in _dequote(config_var).split()]) + dequoted_var = _dequote(config_var) + if dequoted_var: + new_content = '\n'.join(['source "{}"'.format(path) for path in dequoted_var.split(list_separator)]) + else: + new_content = '' + try: with open(config_file, 'r', encoding='utf-8') as f: old_content = f.read() @@ -71,8 +69,7 @@ def _prepare_source_files(env_dict): sys.exit(1) -if __name__ == '__main__': - +def main(): parser = argparse.ArgumentParser(description='Kconfig Source File Generator') parser.add_argument('--env', action='append', default=[], @@ -82,6 +79,10 @@ if __name__ == '__main__': help='Optional file to load environment variables from. Contents ' 'should be a JSON object where each key/value pair is a variable.') + parser.add_argument('--list-separator', choices=['space', 'semicolon'], + default='space', + help='Separator used in environment list variables (COMPONENT_KCONFIGS, COMPONENT_KCONFIGS_PROJBUILD)') + args = parser.parse_args() try: @@ -93,4 +94,10 @@ if __name__ == '__main__': if args.env_file is not None: env.update(json.load(args.env_file)) - _prepare_source_files(env) + list_separator = ';' if args.list_separator == 'semicolon' else ' ' + + _prepare_source_files(env, list_separator) + + +if __name__ == '__main__': + main() From 50799e3026c0f51d7204c15a65321e4ebcdc4513 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 9 Oct 2020 17:01:01 +0200 Subject: [PATCH 3/7] esptool_py: fix passing arguments to cmake -P Fixes WORKING_DIRECTORY argument being passed with spaces escaped `\ ` which causes failure in run_cmd.cmake, since the WORKING_DIRECTORY is interpreted as having a literal backslash character in it. --- components/esptool_py/project_include.cmake | 34 ++++++++++++--------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/components/esptool_py/project_include.cmake b/components/esptool_py/project_include.cmake index c80f3e3b66..8333c54b5f 100644 --- a/components/esptool_py/project_include.cmake +++ b/components/esptool_py/project_include.cmake @@ -148,23 +148,25 @@ endif() add_custom_target(erase_flash COMMAND ${CMAKE_COMMAND} - -D IDF_PATH="${idf_path}" - -D SERIAL_TOOL="${ESPTOOLPY}" - -D SERIAL_TOOL_ARGS="erase_flash" + -D "IDF_PATH=${idf_path}" + -D "SERIAL_TOOL=${ESPTOOLPY}" + -D "SERIAL_TOOL_ARGS=erase_flash" -P run_serial_tool.cmake WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR} USES_TERMINAL + VERBATIM ) add_custom_target(monitor COMMAND ${CMAKE_COMMAND} - -D IDF_PATH="${idf_path}" - -D SERIAL_TOOL="${ESPMONITOR}" - -D SERIAL_TOOL_ARGS="--target;${target};${monitor_rev_args};${elf_dir}/${elf}" - -D WORKING_DIRECTORY="${build_dir}" + -D "IDF_PATH=${idf_path}" + -D "SERIAL_TOOL=${ESPMONITOR}" + -D "SERIAL_TOOL_ARGS=--target;${target};${monitor_rev_args};${elf_dir}/${elf}" + -D "WORKING_DIRECTORY=${build_dir}" -P run_serial_tool.cmake WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR} USES_TERMINAL + VERBATIM ) set(esptool_flash_main_args "--before=${CONFIG_ESPTOOLPY_BEFORE}") @@ -347,13 +349,14 @@ function(esptool_py_flash_target target_name main_args sub_args) add_custom_target(${target_name} COMMAND ${CMAKE_COMMAND} - -D IDF_PATH="${idf_path}" - -D SERIAL_TOOL="${ESPTOOLPY}" - -D SERIAL_TOOL_ARGS="${main_args};write_flash;@${target_name}_args" - -D WORKING_DIRECTORY="${build_dir}" + -D "IDF_PATH=${idf_path}" + -D "SERIAL_TOOL=${ESPTOOLPY}" + -D "SERIAL_TOOL_ARGS=${main_args};write_flash;@${target_name}_args" + -D "WORKING_DIRECTORY=${build_dir}" -P ${esptool_py_dir}/run_serial_tool.cmake WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR} USES_TERMINAL + VERBATIM ) set_target_properties(${target_name} PROPERTIES SUB_ARGS "${sub_args}") @@ -388,13 +391,14 @@ $,\n>") if(${encrypted}) add_custom_target(encrypted-${target_name} COMMAND ${CMAKE_COMMAND} - -D IDF_PATH="${idf_path}" - -D SERIAL_TOOL="${ESPTOOLPY}" - -D SERIAL_TOOL_ARGS="${main_args};write_flash;@encrypted_${target_name}_args" - -D WORKING_DIRECTORY="${build_dir}" + -D "IDF_PATH=${idf_path}" + -D "SERIAL_TOOL=${ESPTOOLPY}" + -D "SERIAL_TOOL_ARGS=${main_args};write_flash;@encrypted_${target_name}_args" + -D "WORKING_DIRECTORY=${build_dir}" -P ${esptool_py_dir}/run_serial_tool.cmake WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR} USES_TERMINAL + VERBATIM ) # Generate the parameters for esptool.py command From 6b361d923ce80d6d20cf552e72ac533aab25a77d Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 29 Sep 2021 21:15:35 +0200 Subject: [PATCH 4/7] esptool_py: fix quoting issues in run_serial_tool.cmake --- components/esptool_py/run_serial_tool.cmake | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/components/esptool_py/run_serial_tool.cmake b/components/esptool_py/run_serial_tool.cmake index 9cfe03901e..0a0b499b94 100644 --- a/components/esptool_py/run_serial_tool.cmake +++ b/components/esptool_py/run_serial_tool.cmake @@ -22,6 +22,8 @@ if(DEFINED ENV{IDF_ENV_FPGA}) message("Note: IDF_ENV_FPGA is set, propagating to esptool with ESPTOOL_ENV_FPGA = 1") endif() +set(serial_tool_cmd ${SERIAL_TOOL}) + # Main purpose of this script: we can't expand these environment variables in the main IDF CMake build, # because we want to expand them at flashing time not at CMake runtime (so they can change # without needing a CMake re-run) @@ -30,7 +32,7 @@ if(NOT ESPPORT) message("Note: ${SERIAL_TOOL} will search for a serial port. " "To specify a port, set the ESPPORT environment variable.") else() - set(port_arg "-p ${ESPPORT}") + list(APPEND serial_tool_cmd -p ${ESPPORT}) endif() set(ESPBAUD $ENV{ESPBAUD}) @@ -38,13 +40,10 @@ if(NOT ESPBAUD) message("Note: ${SERIAL_TOOL} will attempt to set baud rate automatically. " "To specify a baud rate, set the ESPBAUD environment variable.") else() - set(baud_arg "-b ${ESPBAUD}") + list(APPEND serial_tool_cmd -b ${ESPBAUD}) endif() -set(serial_tool_cmd "${SERIAL_TOOL} ${port_arg} ${baud_arg} ${SERIAL_TOOL_ARGS}") - -include("${IDF_PATH}/tools/cmake/utilities.cmake") -spaces2list(serial_tool_cmd) +list(APPEND serial_tool_cmd ${SERIAL_TOOL_ARGS}) execute_process(COMMAND ${serial_tool_cmd} WORKING_DIRECTORY "${WORKING_DIRECTORY}" From 29489a3303f2bb7621ecb3e6f25e680fa8e6dc74 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 9 Oct 2020 19:06:33 +0200 Subject: [PATCH 5/7] build system: fix quoting of fragments list passed to ldgen --- tools/cmake/ldgen.cmake | 19 ++++++++++--------- tools/ldgen/ldgen.py | 20 +++++++++++++++++--- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/tools/cmake/ldgen.cmake b/tools/cmake/ldgen.cmake index d8c5d0eb7d..54b10bc91f 100644 --- a/tools/cmake/ldgen.cmake +++ b/tools/cmake/ldgen.cmake @@ -53,23 +53,24 @@ function(__ldgen_process_template template output) if($ENV{LDGEN_CHECK_MAPPING}) set(ldgen_check "--check-mapping" - "--check-mapping-exceptions=${idf_path}/tools/ci/check_ldgen_mapping_exceptions.txt") + "--check-mapping-exceptions" "${idf_path}/tools/ci/check_ldgen_mapping_exceptions.txt") message(STATUS "Mapping check enabled in ldgen") endif() add_custom_command( OUTPUT ${output} - COMMAND ${python} ${idf_path}/tools/ldgen/ldgen.py - --config ${sdkconfig} - --fragments "$" - --input ${template} - --output ${output} - --kconfig ${root_kconfig} + COMMAND ${python} "${idf_path}/tools/ldgen/ldgen.py" + --config "${sdkconfig}" + --fragments-list "${ldgen_fragment_files}" + --input "${template}" + --output "${output}" + --kconfig "${root_kconfig}" --env-file "${config_env_path}" - --libraries-file ${build_dir}/ldgen_libraries - --objdump ${CMAKE_OBJDUMP} + --libraries-file "${build_dir}/ldgen_libraries" + --objdump "${CMAKE_OBJDUMP}" ${ldgen_check} DEPENDS ${template} ${ldgen_fragment_files} ${ldgen_depends} ${SDKCONFIG} + VERBATIM ) get_filename_component(_name ${output} NAME) diff --git a/tools/ldgen/ldgen.py b/tools/ldgen/ldgen.py index aba52f2c28..a621c12423 100755 --- a/tools/ldgen/ldgen.py +++ b/tools/ldgen/ldgen.py @@ -50,11 +50,20 @@ def main(): help='Linker template file', type=argparse.FileType('r')) - argparser.add_argument( + fragments_group = argparser.add_mutually_exclusive_group() + + fragments_group.add_argument( '--fragments', '-f', type=argparse.FileType('r'), help='Input fragment files', - nargs='+') + nargs='+' + ) + + fragments_group.add_argument( + '--fragments-list', + help='Input fragment files as a semicolon-separated list', + type=str + ) argparser.add_argument( '--libraries-file', @@ -102,13 +111,18 @@ def main(): args = argparser.parse_args() input_file = args.input - fragment_files = [] if not args.fragments else args.fragments libraries_file = args.libraries_file config_file = args.config output_path = args.output kconfig_file = args.kconfig objdump = args.objdump + fragment_files = [] + if args.fragments_list: + fragment_files = args.fragments_list.split(';') + elif args.fragments: + fragment_files = args.fragments + check_mapping = args.check_mapping if args.check_mapping_exceptions: check_mapping_exceptions = [line.strip() for line in args.check_mapping_exceptions] From 09e50b27edbd87b855e1e6a7021c83c268160613 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 6 Oct 2021 09:43:05 +0200 Subject: [PATCH 6/7] cmake: handling of space-separated EXTRA_COMPONENT_DIRS COMPONENT_DIRS and EXTRA_COMPONENT_DIRS should be defined as CMake lists, using 'set' or 'list' commands. Some applications written for earlier versions of ESP-IDF used to define these variables as space separated strings. For example, the following is correct: set(EXTRA_COMPONENT_DIRS path/to/components path/to/more/components) The following is not correct: set(EXTRA_COMPONENT_DIRS "${EXTRA_COMPONENT_DIRS} component1") set(EXTRA_COMPONENT_DIRS "${EXTRA_COMPONENT_DIRS} component2") The string "component1 component2" may indicate a single directory name with a space, or two directory names separated by space. However due to the fact that such way of defining EXTRA_COMPONENT_DIRS was supported in IDF 4.3 and earlier, we need to provide backward compatibility for it. This commit introduces a new script, split_paths_by_spaces.py, which is invoked if EXTRA_COMPONENT_DIRS or COMPONENT_DIRS variable contains spaces. The script tries to determine if each space should be interpreted as a separator or as part of the directory name. When this cannot be done unambiguously, the script reports an error. In all cases when space separators are detected, the script reports a warning, and prints instructions for fixing the CMakeLists.txt. Breaking change in this commit: specifying non-existent directories in COMPONENT_DIRS or EXTRA_COMPONENT_DIRS is no longer allowed. --- .gitlab/ci/host-test.yml | 6 + tools/ci/test_build_system_cmake.sh | 14 +- tools/cmake/project.cmake | 44 +++- tools/cmake/utilities.cmake | 4 +- tools/split_paths_by_spaces.py | 333 ++++++++++++++++++++++++++++ 5 files changed, 396 insertions(+), 5 deletions(-) create mode 100644 tools/split_paths_by_spaces.py diff --git a/.gitlab/ci/host-test.yml b/.gitlab/ci/host-test.yml index fee373b499..2eca5b01e8 100644 --- a/.gitlab/ci/host-test.yml +++ b/.gitlab/ci/host-test.yml @@ -339,6 +339,12 @@ test_detect_python: - "zsh -c '. tools/detect_python.sh && echo Our Python: ${ESP_PYTHON?Python is not set}'" - "fish -c 'source tools/detect_python.fish && echo Our Python: $ESP_PYTHON'" +test_split_path_by_spaces: + extends: .host_test_template + script: + - cd ${IDF_PATH}/tools + - python -m unittest split_paths_by_spaces.py + test_nvs_page: extends: .host_test_template script: diff --git a/tools/ci/test_build_system_cmake.sh b/tools/ci/test_build_system_cmake.sh index 849404a293..9d6371072c 100755 --- a/tools/ci/test_build_system_cmake.sh +++ b/tools/ci/test_build_system_cmake.sh @@ -468,6 +468,17 @@ function run_tests() mv main/main/main/* main rm -rf main/main + print_status "Non-existent paths in EXTRA_COMPONENT_DIRS are not allowed" + clean_build_dir + ! idf.py -DEXTRA_COMPONENT_DIRS="extra_components" reconfigure || failure "Build should fail when non-existent component path is added" + + print_status "Component names may contain spaces" + clean_build_dir + mkdir -p "extra component" + echo "idf_component_register" > "extra component/CMakeLists.txt" + idf.py -DEXTRA_COMPONENT_DIRS="extra component;main" || failure "Build should succeed when a component name contains space" + rm -rf "extra component" + print_status "sdkconfig should have contents of all files: sdkconfig, sdkconfig.defaults, sdkconfig.defaults.IDF_TARGET" idf.py clean > /dev/null idf.py fullclean > /dev/null @@ -662,9 +673,10 @@ endmenu\n" >> ${IDF_PATH}/Kconfig # idf.py subcommand options, (using monitor with as example) print_status "Can set options to subcommands: print_filter for monitor" + clean_build_dir mv ${IDF_PATH}/tools/idf_monitor.py ${IDF_PATH}/tools/idf_monitor.py.tmp echo "import sys;print(sys.argv[1:])" > ${IDF_PATH}/tools/idf_monitor.py - idf.py build || "Failed to build project" + idf.py build || failure "Failed to build project" idf.py monitor --print-filter="*:I" -p tty.fake | grep "'--print_filter', '\*:I'" || failure "It should process options for subcommands (and pass print-filter to idf_monitor.py)" mv ${IDF_PATH}/tools/idf_monitor.py.tmp ${IDF_PATH}/tools/idf_monitor.py diff --git a/tools/cmake/project.cmake b/tools/cmake/project.cmake index 2d56126a09..a4f7da8db4 100644 --- a/tools/cmake/project.cmake +++ b/tools/cmake/project.cmake @@ -63,6 +63,38 @@ function(__project_get_revision var) set(${var} "${PROJECT_VER}" PARENT_SCOPE) endfunction() + +# paths_with_spaces_to_list +# +# Replacement for spaces2list in cases where it was previously used on +# directory lists. +# +# If the variable doesn't contain spaces, (e.g. is already a CMake list) +# then the variable is unchanged. Otherwise an external Python script is called +# to try to split the paths, and the variable is updated with the result. +# +# This feature is added only for compatibility. Please do not introduce new +# space separated path lists. +# +function(paths_with_spaces_to_list variable_name) + if("${${variable_name}}" MATCHES "[ \t]") + idf_build_get_property(python PYTHON) + idf_build_get_property(idf_path IDF_PATH) + execute_process( + COMMAND ${python} + "${idf_path}/tools/split_paths_by_spaces.py" + "--var-name=${variable_name}" + "${${variable_name}}" + WORKING_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}" + OUTPUT_VARIABLE result + RESULT_VARIABLE ret) + if(NOT ret EQUAL 0) + message(FATAL_ERROR "Failed to parse ${variable_name}, see diagnostics above") + endif() + set("${variable_name}" "${result}" PARENT_SCOPE) + endif() +endfunction() + # # Output the built components to the user. Generates files for invoking idf_monitor.py # that doubles as an overview of some of the more important build properties. @@ -182,9 +214,13 @@ function(__project_init components_var test_components_var) # extra directories, etc. passed from the root CMakeLists.txt. if(COMPONENT_DIRS) # User wants to fully override where components are pulled from. - spaces2list(COMPONENT_DIRS) + paths_with_spaces_to_list(COMPONENT_DIRS) idf_build_set_property(__COMPONENT_TARGETS "") foreach(component_dir ${COMPONENT_DIRS}) + get_filename_component(component_abs_path ${component_dir} ABSOLUTE) + if(NOT EXISTS ${component_abs_path}) + message(FATAL_ERROR "Directory specified in COMPONENT_DIRS doesn't exist: ${component_abs_path}") + endif() __project_component_dir(${component_dir}) endforeach() else() @@ -192,8 +228,12 @@ function(__project_init components_var test_components_var) __project_component_dir("${CMAKE_CURRENT_LIST_DIR}/main") endif() - spaces2list(EXTRA_COMPONENT_DIRS) + paths_with_spaces_to_list(EXTRA_COMPONENT_DIRS) foreach(component_dir ${EXTRA_COMPONENT_DIRS}) + get_filename_component(component_abs_path ${component_dir} ABSOLUTE) + if(NOT EXISTS ${component_abs_path}) + message(FATAL_ERROR "Directory specified in EXTRA_COMPONENT_DIRS doesn't exist: ${component_abs_path}") + endif() __project_component_dir("${component_dir}") endforeach() diff --git a/tools/cmake/utilities.cmake b/tools/cmake/utilities.cmake index 49342699d2..e4756c67b5 100644 --- a/tools/cmake/utilities.cmake +++ b/tools/cmake/utilities.cmake @@ -22,8 +22,8 @@ endfunction() # Take a variable whose value was space-delimited values, convert to a cmake # list (semicolon-delimited) # -# Note: if using this for directories, keeps the issue in place that -# directories can't contain spaces... +# Note: do not use this for directories or full paths, as they may contain +# spaces. # # TODO: look at cmake separate_arguments, which is quote-aware function(spaces2list variable_name) diff --git a/tools/split_paths_by_spaces.py b/tools/split_paths_by_spaces.py new file mode 100644 index 0000000000..5a1b25637f --- /dev/null +++ b/tools/split_paths_by_spaces.py @@ -0,0 +1,333 @@ +#!/usr/bin/env python +# coding=utf-8 +# +# SPDX-FileCopyrightText: 2021 Espressif Systems (Shanghai) CO LTD +# +# SPDX-License-Identifier: Apache-2.0 +# +# This script converts space-separated EXTRA_COMPONENT_DIRS and COMPONENT_DIRS +# CMake variables into semicolon-separated lists. +# +# IDF versions <=v4.3 didn't support spaces in paths to ESP-IDF or projects. +# Therefore it was okay to use spaces as separators in EXTRA_COMPONENT_DIRS, +# same as it was done in the legacy GNU Make based build system. +# CMake build system used 'spaces2list' function to convert space-separated +# variables into semicolon-separated lists, replacing every space with a +# semicolon. +# +# In IDF 4.4 and later, spaces in project path and ESP-IDF path are supported. +# This means that EXTRA_COMPONENT_DIRS and COMPONENT_DIRS variables now should +# be semicolon-separated CMake lists. +# +# To provide compatibility with the projects written for older ESP-IDF versions, +# this script attempts to convert these space-separated variables into semicolon- +# separated ones. Note that in general this cannot be done unambiguously, so this +# script will still report an error if there are multiple ways to interpret the +# variable, and ask the user to fix the project CMakeLists.txt file. +# + + +import argparse +import os +import pprint +import sys +import textwrap +import typing +import unittest + + +class PathSplitError(RuntimeError): + pass + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument('--var-name', required=True, help='Name of CMake variable, for printing errors and warnings') + parser.add_argument('in_variable', help='Input variable, may contain a mix of spaces and semicolons as separators') + args = parser.parse_args() + + # Initially split the paths by semicolons + semicolon_separated_parts = args.in_variable.split(';') + + # Every resulting part may contain space separators. Handle each part: + paths = [] + ctx = dict(warnings=False) + errors = False + for part in semicolon_separated_parts: + def warning_cb(warning_str: str) -> None: + print('\n '.join( + textwrap.wrap('Warning: in CMake variable {}: {}'.format(args.var_name, warning_str), width=120, + break_on_hyphens=False)), file=sys.stderr) + ctx['warnings'] = True + + try: + paths += split_paths_by_spaces(part, warning_cb=warning_cb) + except PathSplitError as e: + print('\n '.join(textwrap.wrap('Error: in CMake variable {}: {}'.format(args.var_name, str(e)), width=120, + break_on_hyphens=False)), file=sys.stderr) + errors = True + + if errors or ctx['warnings']: + print(textwrap.dedent(""" + Note: In ESP-IDF v4.4 and later, COMPONENT_DIRS and EXTRA_COMPONENT_DIRS should be defined + as CMake lists, not as space separated strings. + + Examples: + * set(EXTRA_COMPONENT_DIRS path/to/components path/to/more/components) + # Correct, EXTRA_COMPONENT_DIRS is defined as a CMake list, with two paths added + + * list(APPEND EXTRA_COMPONENT_DIRS path/to/component) + list(APPEND EXTRA_COMPONENT_DIRS path/to/more/components) + # Correct, use when building EXTRA_COMPONENT_DIRS incrementally + + * set(EXTRA_COMPONENT_DIRS path/to/components "another/path with space/components") + # Literal path with spaces has to be quoted + + * set(EXTRA_COMPONENT_DIRS $ENV{MY_PATH}/components dir/more_components) + # Correct, even if MY_PATH contains spaces + + * set(EXTRA_COMPONENT_DIRS ${ROOT}/component1 ${ROOT}/component2 ${ROOT}/component3) + # Correct, even if ROOT contains spaces + + Avoid string concatenation! + set(EXTRA_COMPONENT_DIRS "${EXTRA_COMPONENT_DIRS} component1") + set(EXTRA_COMPONENT_DIRS "${EXTRA_COMPONENT_DIRS} component2") + # Incorrect. String "component1 component2" may indicate a single directory + # name with a space, or two directory names separated by space. + + Instead use: + list(APPEND component1) + list(APPEND component2) + + Defining COMPONENT_DIRS and EXTRA_COMPONENT_DIRS as CMake lists is backwards compatible + with ESP-IDF 4.3 and below. + + (If you think these variables are defined correctly in your project and this message + is not relevant, please report this as an issue.) + """), file=sys.stderr) + + print('Diagnostic info: {} was invoked in {} with arguments: {}'.format( + sys.argv[0], os.getcwd(), sys.argv[1:] + ), file=sys.stderr) + + if errors: + raise SystemExit(1) + + sys.stdout.write(';'.join(paths)) + sys.stdout.flush() + + +def split_paths_by_spaces(src: str, path_exists_cb: typing.Callable[[str], bool] = os.path.exists, + warning_cb: typing.Optional[typing.Callable[[str], None]] = None) -> typing.List[str]: + if ' ' not in src: + # no spaces, complete string should be the path + return [src] + + def path_exists_or_empty(path: str) -> bool: + return path == '' or path_exists_cb(path) + + # remove leading and trailing spaces + delayed_warnings = [] + trimmed = src.lstrip(' ') + if trimmed != src: + delayed_warnings.append("Path component '{}' contains leading spaces".format(src)) + src = trimmed + + trimmed = src.rstrip(' ') + if trimmed != src: + delayed_warnings.append("Path component '{}' contains trailing spaces".format(src)) + src = trimmed + + # Enumerate all possible ways to split the string src into paths by spaces. + # The number of these ways is equal to sum(C(n, k), 0<=k 1: + warning_cb("Path component '{}' contains a space separator. It was automatically split into {}".format( + src, pprint.pformat(result) + )) + for w in delayed_warnings: + warning_cb(w) + + return result + + if num_candidates == 0: + raise PathSplitError(("Didn't find a valid way to split path '{}'. " + 'This error may be reported if one or more paths ' + "are separated with spaces, and at least one path doesn't exist.").format(src)) + + # if num_candidates > 1 + raise PathSplitError("Found more than one valid way to split path '{}':{}".format( + src, ''.join('\n\t- ' + pprint.pformat(p) for p in valid_ways_to_split) + )) + + +def selective_join(parts: typing.List[str], n: int) -> typing.List[str]: + """ + Given the list of N+1 strings, and an integer n in [0, 2**N - 1] range, + concatenate i-th and (i+1)-th string with space inbetween if bit i is not set in n. + Examples: + selective_join(['a', 'b', 'c'], 0b00) == ['a b c'] + selective_join(['a', 'b', 'c'], 0b01) == ['a', 'b c'] + selective_join(['a', 'b', 'c'], 0b10) == ['a b', 'c'] + selective_join(['a', 'b', 'c'], 0b11) == ['a', 'b', 'c'] + + This function is used as part of finding all the ways to split a string by spaces. + + :param parts: Strings to join + :param n: Integer (bit map) to set the positions to join + :return: resulting list of strings + """ + result = [] + concatenated = [parts[0]] + for part in parts[1:]: + if n & 1: + result.append(' '.join(concatenated)) + concatenated = [part] + else: + concatenated.append(part) + n >>= 1 + if concatenated: + result.append(' '.join(concatenated)) + return result + + +class HelperTests(unittest.TestCase): + def test_selective_join(self) -> None: + self.assertListEqual(['a b c'], selective_join(['a', 'b', 'c'], 0b00)) + self.assertListEqual(['a', 'b c'], selective_join(['a', 'b', 'c'], 0b01)) + self.assertListEqual(['a b', 'c'], selective_join(['a', 'b', 'c'], 0b10)) + self.assertListEqual(['a', 'b', 'c'], selective_join(['a', 'b', 'c'], 0b11)) + + +class SplitTests(unittest.TestCase): + def test_split_paths_absolute(self) -> None: + self.check_paths_concatenated('/absolute/path/one', '/absolute/path/two') + + def test_split_paths_absolute_spaces(self) -> None: + self.check_paths_concatenated('/absolute/path with spaces') + self.check_paths_concatenated('/absolute/path with more spaces') + self.check_paths_concatenated('/absolute/path with spaces/one', '/absolute/path with spaces/two') + + self.check_paths_concatenated('/absolute/path with spaces/one', + '/absolute/path with spaces/two', + '/absolute/path with spaces/three') + + def test_split_paths_absolute_relative(self) -> None: + self.check_paths_concatenated('/absolute/path/one', 'two') + + def test_split_paths_relative(self) -> None: + self.check_paths_concatenated('one', 'two') + + def test_split_paths_absolute_spaces_relative(self) -> None: + self.check_paths_concatenated('/absolute/path with spaces/one', 'two') + + def test_split_paths_ambiguous(self) -> None: + self.check_paths_concatenated_ambiguous('/absolute/path one', 'two', + additional_paths_exist=['/absolute/path', 'one']) + + self.check_paths_concatenated_ambiguous('/path ', '/path', + additional_paths_exist=['/path /path']) + + def test_split_paths_nonexistent(self) -> None: + self.check_paths_concatenated_nonexistent('one', 'two') + + def test_split_paths_extra_whitespace(self) -> None: + paths = ['/path'] + path_exists = self.path_exists_by_list(paths) + self.assertListEqual(paths, split_paths_by_spaces(' /path', path_exists_cb=path_exists)) + self.assertListEqual(paths, split_paths_by_spaces('/path ', path_exists_cb=path_exists)) + self.assertListEqual(paths + paths, split_paths_by_spaces('/path /path', path_exists_cb=path_exists)) + + def test_split_paths_warnings(self) -> None: + paths = ['/path'] + ctx = {'warnings': []} # type: typing.Dict[str, typing.List[str]] + + def add_warning(warning: str) -> None: + ctx['warnings'].append(warning) + + path_exists = self.path_exists_by_list(paths) + + self.assertListEqual(paths, + split_paths_by_spaces(' /path', path_exists_cb=path_exists, warning_cb=add_warning)) + self.assertEqual(1, len(ctx['warnings'])) + self.assertIn('leading', ctx['warnings'][0]) + + ctx['warnings'] = [] + self.assertListEqual(paths, + split_paths_by_spaces('/path ', path_exists_cb=path_exists, warning_cb=add_warning)) + self.assertEqual(1, len(ctx['warnings'])) + self.assertIn('trailing', ctx['warnings'][0]) + + ctx['warnings'] = [] + self.assertListEqual(paths + paths, + split_paths_by_spaces('/path /path', path_exists_cb=path_exists, warning_cb=add_warning)) + self.assertEqual(1, len(ctx['warnings'])) + self.assertIn('contains a space separator', ctx['warnings'][0]) + + @staticmethod + def path_exists_by_list(paths_which_exist: typing.List[str]) -> typing.Callable[[str], bool]: + """ + Returns a function to check whether a path exists, similar to os.path.exists, but instead of checking + for files on the real filesystem it considers only the paths provided in 'paths_which_exist' argument. + :param paths_which_exist: list of paths which should be considered as existing + :return: function to check if path exists + """ + all_paths = set() + for path in paths_which_exist or []: + # for path /a/b/c, add it and also add components of the path: /a, /a/b + end = len(path) + while end > 0: + all_paths.add(path[0:end]) + end = path.rfind('/', 0, end) + + def path_exists(path: str) -> bool: + return path in all_paths + + return path_exists + + def split_paths_concatenated_base(self, paths_to_concatentate: typing.List[str], + paths_existing: typing.List[str]) -> typing.List[str]: + concatenated = ' '.join(paths_to_concatentate) + path_exists = self.path_exists_by_list(paths_existing) + return split_paths_by_spaces(concatenated, path_exists_cb=path_exists) + + def check_paths_concatenated(self, *args: str) -> None: + paths = [*args] + paths_split = self.split_paths_concatenated_base(paths_to_concatentate=paths, paths_existing=paths) + self.assertListEqual(paths, paths_split) + + def check_paths_concatenated_ambiguous(self, *args: str, + additional_paths_exist: typing.Optional[typing.List[str]] = None) -> None: + paths = [*args] + self.assertRaises(PathSplitError, self.split_paths_concatenated_base, paths_to_concatentate=paths, + paths_existing=paths + (additional_paths_exist or [])) + + def check_paths_concatenated_nonexistent(self, *args: str, + additional_paths_exist: typing.List[str] = None) -> None: + paths = [*args] + self.assertRaises(PathSplitError, self.split_paths_concatenated_base, paths_to_concatentate=paths, + paths_existing=additional_paths_exist) + + +if __name__ == '__main__': + main() From 7056fd312902d28549fd248813086acc944fa1e4 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 7 Oct 2021 16:54:34 +0200 Subject: [PATCH 7/7] bootloader: don't add nonexistent directories to EXTRA_COMPONENT_DIRS --- components/bootloader/subproject/CMakeLists.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/bootloader/subproject/CMakeLists.txt b/components/bootloader/subproject/CMakeLists.txt index f2df83fab9..f805f93eb9 100644 --- a/components/bootloader/subproject/CMakeLists.txt +++ b/components/bootloader/subproject/CMakeLists.txt @@ -37,7 +37,9 @@ set(COMPONENTS # Make EXTRA_COMPONENT_DIRS variable to point to the bootloader_components directory # of the project being compiled set(PROJECT_EXTRA_COMPONENTS "${PROJECT_SOURCE_DIR}/bootloader_components") -list(APPEND EXTRA_COMPONENT_DIRS "${PROJECT_EXTRA_COMPONENTS}") +if(EXISTS ${PROJECT_EXTRA_COMPONENTS}) + list(APPEND EXTRA_COMPONENT_DIRS "${PROJECT_EXTRA_COMPONENTS}") +endif() # Consider each directory in project's bootloader_components as a component to be compiled file(GLOB proj_components RELATIVE ${PROJECT_EXTRA_COMPONENTS} ${PROJECT_EXTRA_COMPONENTS}/*)