cpu/esp32: move ESP32_SDK_DIR definition here #18717
Conversation
Murdock results✔️ PASSED c1a62f3 cpu/esp32: move ESP32_SDK_DIR definition here
ArtifactsThis only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now. |
f993b12 to
d964e15
Compare
|
@aabadie: I also dropped the |
Hm, I'm a bit confused why this change is necessary, it has been working all the time as it was. Why is there now the problem that |
|
It does work also fine for me with The issue is that if it works diff --git a/cpu/esp32/Makefile.include b/cpu/esp32/Makefile.include
index d1123dfd75..010eb72d77 100644
--- a/cpu/esp32/Makefile.include
+++ b/cpu/esp32/Makefile.include
@@ -48,6 +48,8 @@ PSEUDOMODULES += esp_spi_ram
PSEUDOMODULES += esp_spi_oct
PSEUDOMODULES += esp_wifi_enterprise
+$(info INCLUDES (cpu): $(origin INCLUDES), $(flavor INCLUDES))
+
INCLUDES += -I$(RIOTCPU)/$(CPU)/esp-idf/include
INCLUDES += -I$(RIOTCPU)/$(CPU)/esp-idf/include/log
INCLUDES += -I$(RIOTCPU)/$(CPU)/vendor/include
diff --git a/pkg/esp32_sdk/Makefile.include b/pkg/esp32_sdk/Makefile.include
index d028aa7943..2c017c0f72 100644
--- a/pkg/esp32_sdk/Makefile.include
+++ b/pkg/esp32_sdk/Makefile.include
@@ -1,3 +1,6 @@
export ESP32_SDK_DIR ?= $(PKGDIRBASE)/esp32_sdk
+$(info INCLUDES (pkg): $(origin INCLUDES), $(flavor INCLUDES))
+$(info INCLUDES = $(INCLUDES))
+
PSEUDOMODULES += esp32_sdkWhen it fails In Docker, it stays recursively: |
The definition in `pkg/esp32_sdk/Makefile.include` was evaluated by
`make` after the include paths were already set, resulting in
`ESP32_SDK_DIR` being empty in
INCLUDES += -I$(ESP32_SDK_DIR)/components
[...]
This in turn resulted in
cc1: error: /components: No such file or directory [-Werror=missing-include-dirs]
[...]
8b587a4 to
c1a62f3
Compare
This causes issues in building the ESP32 bootloader, which uses |
I fully agree! |
That was exactly the reason why it was exported. Using RIOT's make files didn't work for building the bootloader, so it defines its own rules. |
I agree that this makes sense. I now know why The difference between the docker and my local toolchain is that my local toolchain provides a nano flavor of newlib, which results in this being triggered: Lines 92 to 102 in 2476a3b which converts IMO we should make sure that the build process does not rely on a variable expending recursively. Even GNU Make Manual agrees with me that recursively expended variables are a pain in the ass:
If we agree that we want to address the issue, I only see two options:
I would advise to the latter, as previous experience showed that changing the order of how Makefiles are processed is a pain in the ass: Not only does it require quite some work to get it in a state that it is merged upstream, it often results in bugs popping up weeks after being merged :/ |
I encountered exactly the same problem a few months ago when I switched from my self-compiled toolchain to the vendor version of the toolchain. The Espressif toolchain provides the newlib-nano version, but does not use a separate include directory for the newlib-nano version. That's why I provided PR #17553 (commit d6f86a9) which overrides |
I am a little confused, you prefer option 2 despite of those problems? |
|
There is an "against" missing, sorry :) |
But the The test output of this PR still shows compilation issues. Are they solved with PR #18719? |
Indeed. The |
|
thx :) |
Contribution description
The definition in
pkg/esp32_sdk/Makefile.includewas evaluated bymakeafter the include paths were already set, resulting inESP32_SDK_DIRbeing empty inThis in turn resulted in
Testing procedure
In
masterThis PR:
Issues/PRs references
None