Change(component.cmake): check for missing component_target (IDFGH-13091) #14036

gojimmypi · 2024-06-20T11:09:38Z

When a requested component does not exist, the cmake idf_component_get_property does not currently return an intuitive error. Instead, there's just something about incorrect arguments.

For example, when enabling wolfSSL for the ESP-IDF esp-tls component without following the installation as described in the documentation, a message similar to this appears:

-- Adding linker script C:/SysGCC/esp32/esp-idf/v5.2/components/soc/esp32/ld/esp32.peripherals.ld
CMake Error at C:/SysGCC/esp32/esp-idf/v5.2/tools/cmake/component.cmake:384 (__component_get_property):
  __component_get_property Function invoked with incorrect arguments for
  function named: __component_get_property
Call Stack (most recent call first):
  C:/SysGCC/esp32/esp-idf/v5.2/components/esp-tls/CMakeLists.txt:26 (idf_component_get_property)

-- Configuring incomplete, errors occurred!
System.Exception: CMake exited with code 1
   at s24.e(c a, Object b)

This PR updates the cmake idf_component_get_property function with a more intuitive error and a suggestion how to resolve: Component esp-wolfssl not found.

-- Adding linker script C:/SysGCC/esp32-master/esp-idf/v5.4-master/components/soc/esp32/ld/esp32.peripherals.ld
-- Component esp-wolfssl needs to be installed. See api-reference/protocols/esp_tls docs.
CMake Error at C:/SysGCC/esp32-master/esp-idf/v5.4-master/tools/cmake/component.cmake:383 (message):
  Component esp-wolfssl not found
Call Stack (most recent call first):
  C:/SysGCC/esp32-master/esp-idf/v5.4-master/components/esp-tls/CMakeLists.txt:26 (idf_component_get_property)
-- Configuring incomplete, errors occurred!

This is a replacement for #14033 which has a prohibited mixed-case branch name that I could not easily resolve.

See also #13966 Improving wolfSSL integration with the Espressif ESP-IDF (IDFGH-13019)

github-actions · 2024-06-20T11:10:21Z

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message `"Change(component.cmake): check for missing component_target"`: type/action should start with a lowercase letter type/action should be one of [`change`, `ci`, `docs`, `feat`, `fix`, `refactor`, `remove`, `revert`, `test`] Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: `<type/action>(<scope/component>): <summary>`, for example `fix(esp32): Fixed startup timeout issue` allowed types are: `change,ci,docs,feat,fix,refactor,remove,revert,test` sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) `TIP:` Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello gojimmypi, we appreciate your contribution to this project!

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against 9d1bf49

igrr

LGTM aside from one comment.

tools/cmake/component.cmake

gojimmypi · 2024-06-20T19:41:54Z

Hi @igrr

TL;DR - How important is the Pre-commit Hook for ESP-IDF Project for me as as ESP-IDF contributor?

I noticed the pre_commit_check error:

Notice: It looks like the commits in this PR have been made without having pre-commit hooks installed.
Notice: Please see https://docs.espressif.com/projects/esp-idf/en/latest/esp32/contribute/install-pre-commit-hook.html for instructions.

Error: Process completed with exit code 1.

I followed the instructions, but saw this error:

"components/esp-tls/CMakeLists.txt" is not in /mnt/c/SysGCC/esp32-master/esp-idf/v5.4-master/tools/ci/executable-list.txt

Here's the full output:

trim trailing whitespace.........................................................................................................Passed
fix end of files.................................................................................................................Passed
check that executables have shebangs.............................................................................................Passed
mixed line ending................................................................................................................Passed
fix double quoted strings....................................................................................(no files to check)Skipped
Do not use more than one slash in the branch name................................................................................Passed
Do not use uppercase letters in the branch name..................................................................................Passed
flake8.......................................................................................................(no files to check)Skipped
Reorder Python imports.......................................................................................(no files to check)Skipped
codespell........................................................................................................................Passed
Check File Permissions...........................................................................................................Failed
- hook id: check-executables
- exit code: 1

"components/esp-tls/CMakeLists.txt" is not in /mnt/c/SysGCC/esp32-master/esp-idf/v5.4-master/tools/ci/executable-list.txt

Validate executable-list.txt.....................................................................................................Passed
Check if any Kconfig Options Deprecated......................................................................(no files to check)Skipped
Check CMake Files Format.........................................................................................................Passed
Validate Codeowner File..........................................................................................................Passed
Check rules are generated (based on .gitlab/ci/dependencies/dependencies.yml)................................(no files to check)Skipped
Check type annotations in python files.......................................................................(no files to check)Skipped
Check requirement files......................................................................................(no files to check)Skipped
Check tools dir files patterns...............................................................................(no files to check)Skipped
check patterns-build_components in rules.yml.....................................................................................Passed
Check soc caps kconfig files are generated (based on components/soc/IDF_TARGET/include/soc/soc_caps.h).......(no files to check)Skipped
Check if all apps readme files match given .build-test-rules.yml files. Modify the supported target tables.......................Passed
sort yaml files..............................................................................................(no files to check)Skipped
sort yaml test...............................................................................................(no files to check)Skipped
check path in .build-test-rules.yml exists.......................................................................................Passed
Remove non-existing patterns from ignore lists...................................................................................Passed
Check gitlab yaml files......................................................................................(no files to check)Skipped
File Contents Sorter.........................................................................................(no files to check)Skipped
Check copyright notices......................................................................................(no files to check)Skipped
astyle formatter.............................................................................................(no files to check)Skipped
shellcheck bash..............................................................................................(no files to check)Skipped
shellcheck dash (export.sh)..................................................................................(no files to check)Skipped
Lint rST files in docs folder using Sphinx Lint..............................................................(no files to check)Skipped
Check ESP-IDF KConfig Files..................................................................................(no files to check)Skipped

Note that I use the same toolchain for either Windows development in Visual Studio / VisualGDB (thus a path of C:\.. and also WSL, thus a path of /mnt/c/...)

I then followed the instructions I originally used in WSL also in a DOS prompt:

pre-commit install --allow-missing-config -t pre-commit -t commit-msg

It takes Visual Studio really quite a long time to process that pre-commit script on just a single CMakeLists.txt.

Now I see this error in Visual Studio:

The full error:

[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/PyCQA/flake8.
[INFO] Initializing environment for https://github.com/asottile/reorder-python-imports.
[INFO] Initializing environment for https://github.com/codespell-project/codespell.
[INFO] Initializing environment for local.
[INFO] Initializing environment for local:cmakelint==1.4.1.
[INFO] Initializing environment for local:PyYAML == 5.3.1.
[INFO] Initializing environment for local:mypy,mypy-extensions,types-setuptools,types-PyYAML,types-requests.
[INFO] Initializing environment for local:jsonschema.
[INFO] Initializing environment for local:pyparsing.
[INFO] Initializing environment for local:PyYAML == 5.3.1,idf-build-apps~=2.0.
[INFO] Initializing environment for local:ruamel.yaml.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/espressif/check-copyright/.
[INFO] Initializing environment for https://github.com/espressif/conventional-precommit-linter.
[INFO] Initializing environment for https://github.com/espressif/astyle_py.git.
[INFO] Initializing environment for https://github.com/shellcheck-py/shellcheck-py.
[INFO] Initializing environment for https://github.com/espressif/esp-idf-sbom.git.
[INFO] Initializing environment for https://github.com/sphinx-contrib/sphinx-lint.
[INFO] Initializing environment for https://github.com/espressif/esp-idf-kconfig.git.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/PyCQA/flake8.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/asottile/reorder-python-imports.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/codespell-project/codespell.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for local.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/espressif/check-copyright/.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/espressif/astyle_py.git.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/shellcheck-py/shellcheck-py.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/sphinx-contrib/sphinx-lint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/espressif/esp-idf-kconfig.git.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
trim trailing whitespace.........................................................................................................Passed
fix end of files.................................................................................................................Passed
check that executables have shebangs.............................................................................................Passed
mixed line ending................................................................................................................Passed
fix double quoted strings....................................................................................(no files to check)Skipped
Do not use more than one slash in the branch name................................................................................Passed
Do not use uppercase letters in the branch name..................................................................................Passed
flake8.......................................................................................................(no files to check)Skipped
Reorder Python imports.......................................................................................(no files to check)Skipped
codespell........................................................................................................................Passed
Check File Permissions...........................................................................................................Passed
Validate executable-list.txt.....................................................................................................Passed
Check if any Kconfig Options Deprecated......................................................................(no files to check)Skipped
Check CMake Files Format.........................................................................................................Passed
Validate Codeowner File..........................................................................................................Passed
Check rules are generated (based on .gitlab/ci/dependencies/dependencies.yml)................................(no files to check)Skipped
Check type annotations in python files.......................................................................(no files to check)Skipped
Check requirement files......................................................................................(no files to check)Skipped
Check tools dir files patterns...............................................................................(no files to check)Skipped
check patterns-build_components in rules.yml.....................................................................................Passed
Check soc caps kconfig files are generated (based on components/soc/IDF_TARGET/include/soc/soc_caps.h).......(no files to check)Skipped
Check if all apps readme files match given .build-test-rules.yml files. Modify the supported target tables.......................Passed
sort yaml files..............................................................................................(no files to check)Skipped
sort yaml test...............................................................................................(no files to check)Skipped
check path in .build-test-rules.yml exists.......................................................................................Passed
Remove non-existing patterns from ignore lists...................................................................................Passed
Check gitlab yaml files......................................................................................(no files to check)Skipped
File Contents Sorter.........................................................................................(no files to check)Skipped
Check copyright notices......................................................................................(no files to check)Skipped
astyle formatter.............................................................................................(no files to check)Skipped
shellcheck bash..............................................................................................(no files to check)Skipped
shellcheck dash (export.sh)..................................................................................(no files to check)Skipped
Lint rST files in docs folder using Sphinx Lint..............................................................(no files to check)Skipped
Check ESP-IDF KConfig Files..................................................................................(no files to check)Skipped
[INFO] Installing environment for https://github.com/espressif/conventional-precommit-linter.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Conventional Commit......................................................Failed
- hook id: conventional-precommit-linter
- exit code: 1

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "C:\Users\gojimmypi\.cache\pre-commit\repo7s35z695\py_env-python3.11\Scripts\conventional-precommit-linter.EXE\__main__.py", line 7, in <module>
  File "C:\Users\gojimmypi\.cache\pre-commit\repo7s35z695\py_env-python3.11\Lib\site-packages\conventional_precommit_linter\hook.py", line 248, in main
    print(f'\u274c Missing colon after {_color_purple("<type>")} or {_color_blue("(<optional-scope>)")}.')
  File "C:\Users\gojimmypi\.cache\pre-commit\repo7s35z695\py_env-python3.11\Lib\site-packages\colorama\ansitowin32.py", line 47, in write
    self.__convertor.write(text)
  File "C:\Users\gojimmypi\.cache\pre-commit\repo7s35z695\py_env-python3.11\Lib\site-packages\colorama\ansitowin32.py", line 177, in write
    self.write_and_convert(text)
  File "C:\Users\gojimmypi\.cache\pre-commit\repo7s35z695\py_env-python3.11\Lib\site-packages\colorama\ansitowin32.py", line 202, in write_and_convert
    self.write_plain_text(text, cursor, start)
  File "C:\Users\gojimmypi\.cache\pre-commit\repo7s35z695\py_env-python3.11\Lib\site-packages\colorama\ansitowin32.py", line 210, in write_plain_text
    self.wrapped.write(text[start:end])
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.11_3.11.2544.0_x64__qbz5n2kfra8p0\Lib\encodings\cp1252.py", line 19, in encode
    return codecs.charmap_encode(input,self.errors,encoding_table)[0]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
UnicodeEncodeError: 'charmap' codec can't encode character '\u274c' in position 0: character maps to <undefined>

edit: for reference:

C:\SysGCC\esp32-master\esp-idf\v5.4-master>which python
/c/SysGCC/esp32-master/python_env/idf5.4_py3.11_env/Scripts/python

C:\SysGCC\esp32-master\esp-idf\v5.4-master>echo %USERPROFILE%
C:\Users\gojimmypi

fhrbata · 2024-06-21T09:06:40Z

LGTM, thank you! I think the else after the FATAL_ERROR message is unnecessary, but it doesn't cause any problems either. The failed pre-commit seems to be complaining about an old typo unrelated to this PR, as shown in the failed job
https://github.com/espressif/esp-idf/actions/runs/9597495381/job/26467619009?pr=14036#step:6:74

-    # Assume first that the paramters is an alias.
+    # Assume first that the parameters is an alias.

I think correcting this will allow the check to succeed.

gojimmypi · 2024-06-21T16:23:50Z

LGTM, thank you!

:) You're welcome! Much more to come as wolfSSL is long overdue for a robust integration with the ESP-IDF, beyond the esp-wolfssl. The local Managed Component will be very cool.

I think the else after the FATAL_ERROR message is unnecessary, but it doesn't cause any problems either.

The code after the else is the original code to get val. Are you certain it is not needed?

        if(__GENERATOR_EXPRESSION)
            set(val "$<TARGET_PROPERTY:${component_target},${property}>")
        else()
            __component_get_property(val ${component_target} ${property})
        endif()

The failed pre-commit seems to be complaining about an old typo unrelated to this PR, as shown in the failed job

Yes, but I've been unsuccessful in getting the pre-commit to work in my dual Windows / WSL environment. I'll fuss with it a bit more, perhaps only submitting an ESP-IDF PR from one or the other.

It sounds like otherwise, all is good with this PR. Thanks for taking a look.

fhrbata · 2024-06-21T17:17:50Z

Hello,

I think the else after the FATAL_ERROR message is unnecessary, but it doesn't cause any problems either.

The code after the else is the original code to get val. Are you certain it is not needed?
        if(__GENERATOR_EXPRESSION)
            set(val "$<TARGET_PROPERTY:${component_target},${property}>")
        else()
            __component_get_property(val ${component_target} ${property})
        endif()

I meant this else

function(idf_component_get_property var component property)
    cmake_parse_arguments(_ "GENERATOR_EXPRESSION" "" "" ${ARGN})
    __component_get_target(component_target ${component})
    if("${component_target}" STREQUAL "")
        message(FATAL_ERROR "Component ${component} not found")
    endif()
    if(__GENERATOR_EXPRESSION)
        set(val "$<TARGET_PROPERTY:${component_target},${property}>")
    else()
         __component_get_property(val ${component_target} ${property})
    endif()
    set(${var} "${val}" PARENT_SCOPE)
endfunction()

But it's just aesthetics and probably a matter of personal preference, so please ignore.
Again thank you for this fix!

gojimmypi · 2024-06-21T17:21:37Z

I meant this else

Ah yes, of course. You are quite correct on both style preference and usage. Cheers.

igrr · 2024-07-11T11:07:55Z

sha=9d1bf49497dbda7a2f19573176bccdf7e8debe9f

igrr requested changes Jun 20, 2024

View reviewed changes

tools/cmake/component.cmake Outdated Show resolved Hide resolved

espressif-bot added the Status: Opened Issue is new label Jun 20, 2024

github-actions bot changed the title ~~Change(component.cmake): check for missing component_target~~ Change(component.cmake): check for missing component_target (IDFGH-13091) Jun 20, 2024

Change(component.cmake): check for missing component_target

9d1bf49

gojimmypi force-pushed the pr-idf_component_get_property branch from 52c5cf7 to 9d1bf49 Compare June 20, 2024 12:32

gojimmypi requested a review from igrr June 20, 2024 12:51

igrr approved these changes Jun 20, 2024

View reviewed changes

gojimmypi mentioned this pull request Jun 20, 2024

Improving wolfSSL integration with the Espressif ESP-IDF (IDFGH-13019) #13966

Open

21 tasks

dobairoland requested a review from fhrbata June 21, 2024 08:50

fhrbata approved these changes Jun 21, 2024

View reviewed changes

espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Jun 24, 2024

igrr added the PR-Sync-Merge Pull request sync as merge commit label Jul 11, 2024

igrr approved these changes Jul 11, 2024

View reviewed changes

espressif-bot added Status: In Progress Work is in progress Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Selected for Development Issue is selected for development Status: In Progress Work is in progress labels Jul 12, 2024

espressif-bot closed this in f6a852e Jul 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change(component.cmake): check for missing component_target (IDFGH-13091) #14036

Change(component.cmake): check for missing component_target (IDFGH-13091) #14036

gojimmypi commented Jun 20, 2024 •

edited

Loading

github-actions bot commented Jun 20, 2024 •

edited

Loading

igrr left a comment

gojimmypi commented Jun 20, 2024 •

edited

Loading

fhrbata commented Jun 21, 2024

gojimmypi commented Jun 21, 2024

fhrbata commented Jun 21, 2024

gojimmypi commented Jun 21, 2024

igrr commented Jul 11, 2024

Change(component.cmake): check for missing component_target (IDFGH-13091) #14036

Change(component.cmake): check for missing component_target (IDFGH-13091) #14036

Conversation

gojimmypi commented Jun 20, 2024 • edited Loading

github-actions bot commented Jun 20, 2024 • edited Loading

igrr left a comment

Choose a reason for hiding this comment

gojimmypi commented Jun 20, 2024 • edited Loading

fhrbata commented Jun 21, 2024

gojimmypi commented Jun 21, 2024

fhrbata commented Jun 21, 2024

gojimmypi commented Jun 21, 2024

igrr commented Jul 11, 2024

gojimmypi commented Jun 20, 2024 •

edited

Loading

github-actions bot commented Jun 20, 2024 •

edited

Loading

gojimmypi commented Jun 20, 2024 •

edited

Loading