-
Notifications
You must be signed in to change notification settings - Fork 7.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change(component.cmake): check for missing component_target (IDFGH-13091) #14036
Change(component.cmake): check for missing component_target (IDFGH-13091) #14036
Conversation
👋 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 ...
Review and merge process you can expect ...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from one comment.
52c5cf7
to
9d1bf49
Compare
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:
I followed the instructions, but saw this error:
Here's the full output:
Note that I use the same toolchain for either Windows development in Visual Studio / VisualGDB (thus a path of I then followed the instructions I originally used in WSL also in a DOS prompt:
It takes Visual Studio really quite a long time to process that pre-commit script on just a single Now I see this error in Visual Studio: The full error:
|
LGTM, thank you! I think the
I think correcting this will allow the check to succeed. |
:) You're welcome! Much more to come as wolfSSL is long overdue for a robust integration with the ESP-IDF, beyond the
The code after the else is the original code to get
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. |
Hello,
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. |
Ah yes, of course. You are quite correct on both style preference and usage. Cheers. |
sha=9d1bf49497dbda7a2f19573176bccdf7e8debe9f |
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: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.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)