From 09e50b27edbd87b855e1e6a7021c83c268160613 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 6 Oct 2021 09:43:05 +0200 Subject: [PATCH] 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()