Merge branch 'bugfix/build_spaces_in_path_docs_tests' into 'master'

ci, tools: add build tests for spaces in paths, fix regression in export.sh, add migration guide

See merge request espressif/esp-idf!15431

.gitlab/ci/build.yml

@@ -434,6 +434,11 @@ test_build_system_cmake_macos:
   variables:
     SHELL_TEST_SCRIPT: test_build_system_cmake.sh
 
+test_build_system_spaces:
+  extends: .test_build_system_template
+  variables:
+    SHELL_TEST_SCRIPT: test_build_system_spaces.py
+
 build_docker:
   extends:
     - .before_script_minimal

.gitlab/ci/rules.yml

@@ -51,6 +51,7 @@
   - "tools/tools.json"
   - "tools/requirements.json"
   - "tools/ci/test_build_system*.sh"
+  - "tools/ci/test_build_system*.py"
 
 .patterns-custom_test: &patterns-custom_test
   - "components/espcoredump/**/*"

docs/en/migration-guides/build-system.rst

@@ -1,8 +1,8 @@
 Migrate Build System to ESP-IDF 5.0
 ===================================
 
-Migrating from make to cmake
+Migrating from GNU Make build system
-----------------------------
+------------------------------------
 
 Please follow the :ref:`build system <migrating_from_make>` guide for migrating make-based projects no longer supported in ESP-IDF v5.0.
 
@@ -34,3 +34,24 @@ This behavior was caused by transitive dependencies of various common components
 In ESP-IDF v5.0, this behavior is fixed and these components are no longer added as public requirements by default.
 
 Every component depending on one of the components which isn't part of common requirements has to declare this dependency explicitly. This can be done by adding ``REQUIRES <component_name>`` or ``PRIV_REQUIRES <component_name>`` in ``idf_component_register`` call inside component's ``CMakeLists.txt``. See :ref:`Component Requirements` for more information on specifying requirements.
+
+Setting ``COMPONENT_DIRS`` and ``EXTRA_COMPONENT_DIRS`` variables
+-----------------------------------------------------------------
+
+.. highlight:: cmake
+
+ESP-IDF 5.0 includes a number of improvements to support building projects with space characters in their paths. To make that possible, there are some changes related to setting ``COMPONENT_DIRS`` and ``EXTRA_COMPONENT_DIRS`` variables in project CMakeLists.txt files.
+
+Adding non-existent directories to ``COMPONENT_DIRS`` or ``EXTRA_COMPONENT_DIRS`` is no longer supported and will result in an error.
+
+Using string concatenation to define ``COMPONENT_DIRS`` or ``EXTRA_COMPONENT_DIRS`` variables is now deprecated. These variables should be defined as CMake lists, instead. For example, use::
+
+    set(EXTRA_COMPONENT_DIRS path1 path2)
+    list(APPEND EXTRA_COMPONENT_DIRS path3)
+
+instead of::
+
+    set(EXTRA_COMPONENT_DIRS "path1 path2")
+    set(EXTRA_COMPONENT_DIRS "${EXTRA_COMPONENT_DIRS} path3")
+
+Defining these variables as CMake lists is compatible with previous IDF versions.

export.sh

@@ -18,7 +18,7 @@ __verbose() {
 }
 
 __script_dir(){
-    # shellcheck disable=SC2169,SC2169,SC2039  # unreachable with 'dash'
+    # shellcheck disable=SC2169,SC2169,SC2039,SC3010,SC3028  # unreachable with 'dash'
     if [[ "$OSTYPE" == "darwin"* ]]; then
         # convert possibly relative path to absolute
         script_dir="$(__realpath "${self_path}")"
@@ -48,11 +48,11 @@ __main() {
     # The file doesn't have executable permissions, so this shouldn't really happen.
     # Doing this in case someone tries to chmod +x it and execute...
 
-    # shellcheck disable=SC2128,SC2169,SC2039 # ignore array expansion warning
+    # shellcheck disable=SC2128,SC2169,SC2039,SC3054 # ignore array expansion warning
     if [ -n "${BASH_SOURCE-}" ] && [ "${BASH_SOURCE[0]}" = "${0}" ]
     then
         echo "This script should be sourced, not executed:"
-        # shellcheck disable=SC2039  # reachable only with bash
+        # shellcheck disable=SC2039,SC3054  # reachable only with bash
         echo ". ${BASH_SOURCE[0]}"
         return 1
     fi
@@ -68,9 +68,9 @@ __main() {
         self_path="${(%):-%x}"
     fi
 
-    script_dir=$(__script_dir)
+    script_dir="$(__script_dir)"
     # Since sh or dash shells can't detect script_dir correctly, check if script_dir looks like an IDF directory
-    is_script_dir_esp_idf=$(__is_dir_esp_idf ${script_dir})
+    is_script_dir_esp_idf=$(__is_dir_esp_idf "${script_dir}")
 
     if [ -z "${IDF_PATH}" ]
     then
@@ -95,7 +95,7 @@ __main() {
             export IDF_PATH="${script_dir}"
         fi
         # Check if this path looks like an IDF directory
-        is_idf_path_esp_idf=$(__is_dir_esp_idf ${IDF_PATH})
+        is_idf_path_esp_idf=$(__is_dir_esp_idf "${IDF_PATH}")
         if [ -n "${is_idf_path_esp_idf}" ]
         then
             echo "IDF_PATH is set to '${IDF_PATH}', but it doesn't look like an ESP-IDF directory."
@@ -130,7 +130,7 @@ __main() {
     IDF_ADD_PATHS_EXTRAS="${IDF_ADD_PATHS_EXTRAS}:${IDF_PATH}/components/partition_table"
     IDF_ADD_PATHS_EXTRAS="${IDF_ADD_PATHS_EXTRAS}:${IDF_PATH}/components/app_update"
 
-    idf_exports=$("$ESP_PYTHON" "${IDF_PATH}/tools/idf_tools.py" export --add_paths_extras=${IDF_ADD_PATHS_EXTRAS}) || return 1
+    idf_exports=$("$ESP_PYTHON" "${IDF_PATH}/tools/idf_tools.py" export "--add_paths_extras=${IDF_ADD_PATHS_EXTRAS}") || return 1
     eval "${idf_exports}"
     export PATH="${IDF_ADD_PATHS_EXTRAS}:${PATH}"
 
@@ -166,7 +166,7 @@ __main() {
         __verbose ""
         __verbose "Detected installed tools that are not currently used by active ESP-IDF version."
         __verbose "${uninstall}"
-        __verbose "For free up even more space, remove installation packages of those tools. Use option '${ESP_PYTHON} ${IDF_PATH}/tools/idf_tools.py uninstall --remove-archives'."
+        __verbose "To free up even more space, remove installation packages of those tools. Use option '${ESP_PYTHON} ${IDF_PATH}/tools/idf_tools.py uninstall --remove-archives'."
         __verbose ""
     fi
 
@@ -209,7 +209,7 @@ __cleanup() {
 
 __enable_autocomplete() {
     click_version="$(python -c 'import click; print(click.__version__.split(".")[0])')"
-    if [[ click_version -lt 8 ]]
+    if [ "${click_version}" -lt 8 ]
     then
         SOURCE_ZSH=source_zsh
         SOURCE_BASH=source_bash
@@ -224,7 +224,8 @@ __enable_autocomplete() {
     elif [ -n "${BASH_SOURCE-}" ]
     then
         WARNING_MSG="WARNING: Failed to load shell autocompletion for bash version: $BASH_VERSION!"
-        [[ ${BASH_VERSINFO[0]} -lt 4 ]] && { echo "$WARNING_MSG"; return; }
+        # shellcheck disable=SC3028,SC3054,SC2086  # code block for 'bash' only
+        [ ${BASH_VERSINFO[0]} -lt 4 ] && { echo "$WARNING_MSG"; return; }
         eval "$(env LANG=en _IDF.PY_COMPLETE=$SOURCE_BASH idf.py)"  || echo "$WARNING_MSG"
     fi
 }

tools/ci/executable-list.txt

@@ -77,6 +77,7 @@ tools/ci/multirun_with_pyenv.sh
 tools/ci/push_to_github.sh
 tools/ci/test_autocomplete.py
 tools/ci/test_build_system_cmake.sh
+tools/ci/test_build_system_spaces.py
 tools/ci/test_check_kconfigs.py
 tools/ci/test_configure_ci_environment.sh
 tools/ci/test_reproducible_build.sh

tools/ci/test_build_system_spaces.py

@@ -0,0 +1,220 @@
+#!/usr/bin/env python
+# SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD
+# SPDX-License-Identifier: Apache-2.0
+import os
+import shutil
+import subprocess
+import sys
+import typing
+import unittest
+
+try:
+    IDF_PATH = os.environ['IDF_PATH']  # type: str
+except KeyError:
+    print('IDF_PATH must be set before running this test', file=sys.stderr)
+    exit(1)
+
+# Use the current directory for the builds
+TEST_PATH = os.getcwd()
+
+
+##################
+# Helper functions
+##################
+
+def find_python(path_var: str) -> str:
+    """
+    Find python interpreter in the paths specified in the given PATH variable.
+    Returns the full path to the interpreter.
+    """
+    res = shutil.which('python', path=path_var)
+    if res is None:
+        raise ValueError('python not found')
+    return res
+
+
+def get_idf_build_env(idf_path: typing.Optional[str]) -> typing.Dict[str, str]:
+    """
+    Get environment variables (as set by export.sh) for the specific IDF copy
+    :param idf_path: if set, path of the IDF copy to use; otherwise, IDF_PATH from environment is used
+    :return: dictionary of environment variables and their values
+    """
+    if idf_path is None:
+        idf_path = IDF_PATH
+    cmd = [
+        sys.executable,
+        os.path.join(idf_path, 'tools', 'idf_tools.py'),
+        'export',
+        '--format=key-value'
+    ]
+    keys_values = subprocess.check_output(cmd).decode()
+    env_vars = {key: os.path.expandvars(value) for key, value in
+                [line.split('=') for line in keys_values.splitlines()]}
+    # not set by idf_tools.py, normally set by export.sh
+    env_vars['IDF_PATH'] = idf_path
+
+    return env_vars
+
+
+def idf_py(*args: str, env_vars: typing.Optional[typing.Dict[str, str]] = None,
+           idf_path: typing.Optional[str] = None,
+           workdir: typing.Optional[str] = None) -> None:
+    """
+    Run idf.py command with given arguments, raise an exception on failure
+    :param args: arguments to pass to idf.py
+    :param env_vars: environment variables to run the build with; if not set, the default environment is used
+    :param idf_path: path to the IDF copy to use; if not set, IDF_PATH from the environment is used
+    :param workdir: directory where to run the build; if not set, the current directory is used
+    """
+    env = dict(**os.environ)
+    if env_vars is not None:
+        env.update(env_vars)
+    if not workdir:
+        workdir = os.getcwd()
+    # order: function argument -> value in env dictionary -> system environment
+    if idf_path is not None:
+        inferred_idf_path = idf_path
+    else:
+        inferred_idf_path = str(env.get('IDF_PATH', IDF_PATH))
+
+    python = find_python(env['PATH'])
+
+    cmd = [
+        python,
+        os.path.join(inferred_idf_path, 'tools', 'idf.py')
+    ]
+    cmd += args  # type: ignore
+    subprocess.check_call(cmd, env=env, cwd=workdir)
+
+
+############
+# Test cases
+############
+
+class PathsWithSpaces(unittest.TestCase):
+    IDF_PATH_WITH_SPACES = ''
+    DO_CLEANUP = True
+    ENV: typing.Dict[str, str] = dict()
+
+    @classmethod
+    def setUpClass(cls) -> None:
+        if ' ' in IDF_PATH:
+            print('IDF_PATH already contains spaces, not making a copy')
+            cls.IDF_PATH_WITH_SPACES = IDF_PATH
+            cls.DO_CLEANUP = False
+        else:
+            # Make a copy of ESP-IDF directory, with a space in its name
+            cls.IDF_PATH_WITH_SPACES = os.path.join(TEST_PATH, 'esp idf')
+            dest = cls.IDF_PATH_WITH_SPACES
+            print('Copying esp-idf from {} to {}'.format(IDF_PATH, dest))
+            shutil.copytree(IDF_PATH, dest,
+                            # if the CWD is inside the original esp-idf directory, make sure not to go into recursion.
+                            ignore=shutil.ignore_patterns(os.path.basename(dest)))
+
+        cls.ENV = get_idf_build_env(cls.IDF_PATH_WITH_SPACES)
+
+    @classmethod
+    def tearDownClass(cls) -> None:
+        if cls.DO_CLEANUP and os.path.exists(cls.IDF_PATH_WITH_SPACES):
+            shutil.rmtree(cls.IDF_PATH_WITH_SPACES, ignore_errors=True)
+
+    def test_install_export(self) -> None:
+        env = dict(**os.environ)
+        del env['IDF_PATH']
+        if os.name == 'nt':
+            install_cmd = 'install.bat esp32'
+            export_cmd = 'export.bat'
+        else:
+            install_cmd = './install.sh esp32'
+            export_cmd = '. ./export.sh'
+
+        subprocess.check_call(install_cmd, env=env, shell=True, cwd=self.IDF_PATH_WITH_SPACES)
+
+        if os.name == 'nt':
+            subprocess.check_call(export_cmd, env=env, shell=True, cwd=self.IDF_PATH_WITH_SPACES)
+        else:
+            # The default shell used by subprocess.Popen on POSIX platforms is '/bin/sh',
+            # which in esp-env Docker image is 'dash'. The export script doesn't support
+            # IDF_PATH detection when used in dash, so we have to override the shell here.
+            subprocess.check_call(export_cmd, env=env, shell=True, cwd=self.IDF_PATH_WITH_SPACES, executable='/bin/bash')
+
+    def _copy_app_to(self, app_path: str, dest_dir_name: str) -> str:
+        """
+        Copy given app to a different directory, setting up cleanup hook.
+        Destination directory is first deleted if it already exists.
+        Returns the full path of the destination directory.
+        app_path: path relative to esp-idf directory
+        dest_dir_name: name of the destination directory, relative to the directory where the test is running
+        """
+        dest_dir = os.path.join(TEST_PATH, dest_dir_name)
+        if os.path.exists(dest_dir) and os.path.isdir(dest_dir):
+            shutil.rmtree(dest_dir, ignore_errors=True)
+        src_dir = os.path.join(self.IDF_PATH_WITH_SPACES, app_path)
+        shutil.copytree(os.path.join(src_dir), dest_dir)
+        build_dir = os.path.join(dest_dir, 'build')
+        if os.path.exists(build_dir):
+            shutil.rmtree(build_dir, ignore_errors=True)
+        self.addCleanup(shutil.rmtree, dest_dir, ignore_errors=True)
+        return dest_dir
+
+    # The tests below build different ESP-IDF apps (examples or test apps) to cover different parts
+    # of the build system and related scripts.
+    # In each test, IDF_PATH and app path both contain spaces.
+
+    def test_build(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'get-started', 'hello_world'), 'test app')
+        idf_py('build', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_build_ulp_fsm(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'system', 'ulp_fsm', 'ulp'), 'test app')
+        idf_py('build', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_build_ulp_riscv(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'system', 'ulp_riscv', 'gpio'), 'test app')
+        idf_py('-DIDF_TARGET=esp32s2', 'build', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES,
+               workdir=build_path)
+
+    def test_spiffsgen(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'storage', 'spiffsgen'), 'test app')
+        idf_py('build', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_flash_encryption(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'security', 'flash_encryption'), 'test app')
+        idf_py('build', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_secure_boot_v1(self) -> None:
+        build_path = self._copy_app_to(os.path.join('tools', 'test_apps', 'security', 'secure_boot'), 'test app')
+        idf_py('-DSDKCONFIG_DEFAULTS=sdkconfig.defaults;sdkconfig.ci.01', 'build', env_vars=self.ENV,
+               idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_secure_boot_v2(self) -> None:
+        build_path = self._copy_app_to(os.path.join('tools', 'test_apps', 'security', 'secure_boot'), 'test app')
+        idf_py('-DSDKCONFIG_DEFAULTS=sdkconfig.defaults;sdkconfig.ci.00', 'build', env_vars=self.ENV,
+               idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_app_signing(self) -> None:
+        build_path = self._copy_app_to(os.path.join('tools', 'test_apps', 'security', 'secure_boot'), 'test app')
+        idf_py('-DSDKCONFIG_DEFAULTS=sdkconfig.defaults;sdkconfig.ci.02', 'build', env_vars=self.ENV,
+               idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_secure_boot_release_mode(self) -> None:
+        build_path = self._copy_app_to(os.path.join('tools', 'test_apps', 'security', 'secure_boot'), 'test app')
+        idf_py('-DSDKCONFIG_DEFAULTS=sdkconfig.defaults;sdkconfig.ci.04', '-DIDF_TARGET=esp32s2', 'build',
+               env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_x509_cert_bundle(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'protocols', 'https_x509_bundle'), 'test app')
+        idf_py('build', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+    def test_dfu(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'get-started', 'hello_world'), 'test app')
+        idf_py('-DIDF_TARGET=esp32s2', 'dfu', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES,
+               workdir=build_path)
+
+    def test_uf2(self) -> None:
+        build_path = self._copy_app_to(os.path.join('examples', 'get-started', 'hello_world'), 'test app')
+        idf_py('uf2', env_vars=self.ENV, idf_path=self.IDF_PATH_WITH_SPACES, workdir=build_path)
+
+
+if __name__ == '__main__':
+    unittest.main()

tools/kconfig_new/prepare_kconfig_files.py

@@ -32,9 +32,8 @@ def _prepare_source_files(env_dict, list_separator):
         source "var2"
         source "var3"
 
-    The character used to delimit paths in COMPONENT_KCONFIGS* variables is determined based on
+    The character used to delimit paths in COMPONENT_KCONFIGS* variables is set using --list-separator option.
-    presence of 'IDF_CMAKE' variable in the env_dict.
+    Space separated lists are currently only used by the documentation build system (esp-docs).
-    GNU Make build system uses a space, CMake build system uses a semicolon.
     """
 
     def _dequote(var):

tools/split_paths_by_spaces.py

@@ -1,21 +1,21 @@
 #!/usr/bin/env python
 # coding=utf-8
 #
-# SPDX-FileCopyrightText: 2021 Espressif Systems (Shanghai) CO LTD
+# SPDX-FileCopyrightText: 2021-2022 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.
+# IDF versions <=v4.4 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.
+# In IDF 5.0 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.
 #
@@ -69,7 +69,7 @@ def main() -> None:
 
     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
+            Note: In ESP-IDF v5.0 and later, COMPONENT_DIRS and EXTRA_COMPONENT_DIRS should be defined
                   as CMake lists, not as space separated strings.
 
             Examples:
@@ -100,7 +100,7 @@ def main() -> None:
                 list(APPEND component2)
 
             Defining COMPONENT_DIRS and EXTRA_COMPONENT_DIRS as CMake lists is backwards compatible
-            with ESP-IDF 4.3 and below.
+            with ESP-IDF 4.4 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.)