Skip to content
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

mbedtls compile options are not longer given to external modules #69375

Closed
xhpohanka opened this issue Feb 23, 2024 · 14 comments
Closed

mbedtls compile options are not longer given to external modules #69375

xhpohanka opened this issue Feb 23, 2024 · 14 comments
Assignees
Labels
area: Build System area: Security Security bug The issue is a bug, or the PR is fixing a bug

Comments

@xhpohanka
Copy link
Contributor

Describe the bug
In current main branch there is a regression regarding mbedtls.
In 3.5 branch the -DMBEDTLS_CONFIG_FILE=\"config-tls-generic.h\" and -I/home/zephyrproject/modules/crypto/mbedtls/include were given to external modules but that is no longer the case in current main.

To Reproduce
Steps to reproduce the behavior:

  1. build the project with external module using mbedtls
  2. see errors about include files

Expected behavior
External project has the required compile options.

@xhpohanka xhpohanka added the bug The issue is a bug, or the PR is fixing a bug label Feb 23, 2024
@nashif nashif added the priority: high High impact/importance bug label Feb 27, 2024
@bonetor
Copy link

bonetor commented Mar 19, 2024

I have a similar issue after the update from Zephyr v3.5.0 to v3.6.0.

I have an external module using mbedtls via CONFIG_MBEDTLS=y. After the update to Zephyr v3.6.0 the mbedtls include files in /modules/crypto/mbedtls/include are not found anymore.

Is there any workaround or fix in progress? I cannot update to v3.6.0.

@nashif
Copy link
Member

nashif commented Apr 2, 2024

@tejlmand can you please take a look?

@aescolar
Copy link
Member

@tejlmand or @nordicjm , @57300 can you please take a look at this?

@tejlmand
Copy link
Collaborator

tejlmand commented May 23, 2024

@xhpohanka can you provide a bit more information on this ?

The mbedtls flags are passed to the Zephyr mbedTLS module, https://github.com/zephyrproject-rtos/mbedtls, which can be seen if building the tests/crypto/crypto_hash test sample.

Output when building main, but same result on Zephyr v3.6.0:

$ cmake -GNinja -Bbuild -DBOARD=nrf52840dk/nrf52840 tests/crypto/crypto_hash
$ ninja -Cbuild -v
...
[207/236] ccache /opt/zephyr-sdk-0.16.1/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc ...
 -DMBEDTLS_CONFIG_FILE=\"config-tls-generic.h\" -DNRF52840_XXAA
...
 -I/projects/github/ncs/modules/crypto/mbedtls/include
 -I/projects/github/ncs/zephyr/modules/mbedtls/configs
 -I/projects/github/ncs/zephyr/modules/mbedtls/include
...
 -o modules/mbedtls/CMakeFiles/modules__mbedtls.dir/projects/github/ncs/modules/crypto/mbedtls/library/ssl_tls.c.obj
 -c /projects/github/ncs/modules/crypto/mbedtls/library/ssl_tls.c
...

or are you talking about other external modules, and which modules ?

Is it modules hosted by Zephyr, or downstream only modules ?

To Reproduce
Steps to reproduce the behavior:

  • build the project with external module using mbedtls
  • see errors about include files

Which project are you building, and for which board ?
Please provide as many details possible in order for us to investigate.

Is the issue reproducible with any board or sample in the Zephyr repository ?

@xhpohanka
Copy link
Contributor Author

Hi,

just a very quick update now.

  • I'm building a project for stm32h743 west build -b nucleo_h743zi.
  • I noticed it first when merged the commit 5b4c927 if February
  • I'm afraid your test is not trying to reproduce my issue, it checkes only the zephyr itself, not external modules
  • check the compile command I get for different files, for external ones the include paths of mbedtls are missing
    • main.c - internal project file
    • pb_decode.c - external module, supported
    • civetweb.c - external module (currently unsupported but it is the one where mbedtls is used for me`
arm-zephyr-eabi-gcc 
-DCORE_CM7 
-DHSE_VALUE=8000000 
-DKERNEL 
-DK_HEAP_MEM_POOL_SIZE=1024 
-DMBEDTLS_CONFIG_FILE=\"config-tls-generic.h\" 
-DPB_MAX_REQUIRED_FIELDS=64 
-DSTM32H743xx 
-DUSE_FULL_LL_DRIVER 
-DUSE_HAL_DRIVER 
-D__PROGRAM_START 
-D__ZEPHYR__=1 
-I/home/honza/dev/zephyrproject2/poemodule/src 
-I/home/honza/dev/zephyrproject2/zephyr/include 
-I/home/honza/dev/zephyrproject2/poemodule/build/zephyr/include/generated 
-I/home/honza/dev/zephyrproject2/zephyr/soc/arm/st_stm32/stm32h7 
-I/home/honza/dev/zephyrproject2/zephyr/include/zephyr/posix 
-I/home/honza/dev/zephyrproject2/zephyr/lib/posix/options/getopt/. 
-I/home/honza/dev/zephyrproject2/zephyr/drivers 
-I/home/honza/dev/zephyrproject2/zephyr/soc/arm/st_stm32/common 
-I/home/honza/dev/zephyrproject2/zephyr/subsys/net/l2 
-I/home/honza/dev/zephyrproject2/zephyr/subsys/net/lib/dns/. 
-I/home/honza/dev/zephyrproject2/modules/lib/civetweb/include 
-I/home/honza/dev/zephyrproject2/modules/lib/nanopb 
-I/home/honza/dev/zephyrproject2/modules/hal/cmsis/CMSIS/Core/Include 
-I/home/honza/dev/zephyrproject2/zephyr/modules/cmsis/. 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/soc 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/Legacy 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/common_ll/include 
-I/home/honza/dev/zephyrproject2/modules/crypto/mbedtls/include 
-I/home/honza/dev/zephyrproject2/zephyr/modules/mbedtls/configs 
-I/home/honza/dev/zephyrproject2/zephyr/modules/mbedtls/include 
-isystem /home/honza/dev/zephyrproject2/zephyr/lib/libc/minimal/include 
-isystem /home/honza/dev/zephyrproject2/zephyr/lib/libc/common/include 
-isystem /home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/include 
-isystem /home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/include-fixed 
-fno-strict-aliasing 
-Os 
-imacros /home/honza/dev/zephyrproject2/poemodule/build/zephyr/include/generated/autoconf.h 
-ffreestanding 
-fno-common 
-g 
-gdwarf-4 
-fdiagnostics-color=always 
-mcpu=cortex-m7 
-mthumb 
-mabi=aapcs 
-mfp16-format=ieee 
--sysroot=/home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/arm-zephyr-eabi 
-imacros /home/honza/dev/zephyrproject2/zephyr/include/zephyr/toolchain/zephyr_stdint.h 
-Wall 
-Wformat 
-Wformat-security 
-Wno-format-zero-length 
-Wdouble-promotion 
-Wno-pointer-sign 
-Wpointer-arith 
-Wexpansion-to-defined 
-Wno-unused-but-set-variable 
-Werror=implicit-int 
-fno-pic 
-fno-pie 
-fno-asynchronous-unwind-tables 
-Werror-implicit-function-declaration 
-fno-reorder-functions 
--param=min-pagesize=0 
-fno-defer-pop 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2/poemodule=CMAKE_SOURCE_DIR 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2/zephyr=ZEPHYR_BASE 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2=WEST_TOPDIR 
-ffunction-sections 
-fdata-sections 
-D_POSIX_THREADS 
-std=c99 
-nostdinc 
-MD 
-MT CMakeFiles/app.dir/src/main.c.obj 
-MF CMakeFiles/app.dir/src/main.c.obj.d 
-o CMakeFiles/app.dir/src/main.c.obj 
-c /home/honza/dev/zephyrproject2/poemodule/src/main.c

arm-zephyr-eabi-gcc 
-DCORE_CM7 
-DHSE_VALUE=8000000 
-DKERNEL 
-DK_HEAP_MEM_POOL_SIZE=1024 
-DMG_EXTERNAL_FUNCTION_log_access 
-DMG_EXTERNAL_FUNCTION_mg_cry_internal_impl 
-DNO_ALTERNATIVE_QUEUE 
-DNO_CACHING 
-DNO_CGI 
-DNO_FILES 
-DNO_FILESYSTEMS 
-DNO_SSL 
-DOPENSSL_API_1_1 
-DPB_MAX_REQUIRED_FIELDS=64 
-DSTM32H743xx 
-DUSE_FULL_LL_DRIVER 
-DUSE_HAL_DRIVER 
-DUSE_IPV6 
-DUSE_MBEDTLS 
-DUSE_STACK_SIZE=16384 
-DZEPHYR_VERSION=\"3.6.0-rc2\" 
-D__PROGRAM_START 
-D__ZEPHYR__=1 
-I/home/honza/dev/zephyrproject2/zephyr/include 
-I/home/honza/dev/zephyrproject2/poemodule/build/zephyr/include/generated 
-I/home/honza/dev/zephyrproject2/zephyr/soc/arm/st_stm32/stm32h7 
-I/home/honza/dev/zephyrproject2/zephyr/include/zephyr/posix 
-I/home/honza/dev/zephyrproject2/zephyr/lib/posix/options/getopt/. 
-I/home/honza/dev/zephyrproject2/zephyr/drivers 
-I/home/honza/dev/zephyrproject2/zephyr/soc/arm/st_stm32/common 
-I/home/honza/dev/zephyrproject2/zephyr/subsys/net/l2 
-I/home/honza/dev/zephyrproject2/zephyr/subsys/net/lib/dns/. 
-I/home/honza/dev/zephyrproject2/modules/lib/civetweb/include 
-I/home/honza/dev/zephyrproject2/modules/lib/nanopb 
-I/home/honza/dev/zephyrproject2/modules/hal/cmsis/CMSIS/Core/Include 
-I/home/honza/dev/zephyrproject2/zephyr/modules/cmsis/. 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/soc 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/Legacy 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/common_ll/include 
-I/home/honza/dev/zephyrproject2/modules/lib/civetweb/src 
-I/home/honza/dev/zephyrproject2/poemodule/src 
-isystem /home/honza/dev/zephyrproject2/zephyr/lib/libc/minimal/include 
-isystem /home/honza/dev/zephyrproject2/zephyr/lib/libc/common/include 
-isystem /home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/include 
-isystem /home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/include-fixed 
-std=c11 
-fno-strict-aliasing 
-Os 
-imacros /home/honza/dev/zephyrproject2/poemodule/build/zephyr/include/generated/autoconf.h 
-ffreestanding 
-fno-common 
-g 
-gdwarf-4 
-fdiagnostics-color=always 
-mcpu=cortex-m7 
-mthumb 
-mabi=aapcs 
-mfp16-format=ieee 
--sysroot=/home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/arm-zephyr-eabi 
-imacros /home/honza/dev/zephyrproject2/zephyr/include/zephyr/toolchain/zephyr_stdint.h 
-Wall 
-Wformat 
-Wformat-security 
-Wno-format-zero-length 
-Wdouble-promotion 
-Wno-pointer-sign 
-Wpointer-arith 
-Wexpansion-to-defined 
-Wno-unused-but-set-variable 
-Werror=implicit-int 
-fno-pic 
-fno-pie 
-fno-asynchronous-unwind-tables 
-Werror-implicit-function-declaration 
-fno-reorder-functions 
--param=min-pagesize=0 
-fno-defer-pop 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2/poemodule=CMAKE_SOURCE_DIR 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2/zephyr=ZEPHYR_BASE 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2=WEST_TOPDIR 
-ffunction-sections 
-fdata-sections 
-D_POSIX_THREADS 
-std=c99 
-nostdinc 
-MD 
-MT modules/civetweb/CMakeFiles/..__modules__lib__civetweb.dir/src/civetweb.c.obj 
-MF modules/civetweb/CMakeFiles/..__modules__lib__civetweb.dir/src/civetweb.c.obj.d 
-o modules/civetweb/CMakeFiles/..__modules__lib__civetweb.dir/src/civetweb.c.obj 
-c /home/honza/dev/zephyrproject2/modules/lib/civetweb/src/civetweb.c

arm-zephyr-eabi-gcc 
-DCORE_CM7 
-DHSE_VALUE=8000000 
-DKERNEL 
-DK_HEAP_MEM_POOL_SIZE=1024 
-DPB_MAX_REQUIRED_FIELDS=64 
-DSTM32H743xx 
-DUSE_FULL_LL_DRIVER 
-DUSE_HAL_DRIVER 
-D__PROGRAM_START 
-D__ZEPHYR__=1 
-I/home/honza/dev/zephyrproject2/zephyr/include 
-I/home/honza/dev/zephyrproject2/poemodule/build/zephyr/include/generated 
-I/home/honza/dev/zephyrproject2/zephyr/soc/arm/st_stm32/stm32h7 
-I/home/honza/dev/zephyrproject2/zephyr/include/zephyr/posix 
-I/home/honza/dev/zephyrproject2/zephyr/lib/posix/options/getopt/. 
-I/home/honza/dev/zephyrproject2/zephyr/drivers 
-I/home/honza/dev/zephyrproject2/zephyr/soc/arm/st_stm32/common 
-I/home/honza/dev/zephyrproject2/zephyr/subsys/net/l2 
-I/home/honza/dev/zephyrproject2/zephyr/subsys/net/lib/dns/. 
-I/home/honza/dev/zephyrproject2/modules/lib/civetweb/include 
-I/home/honza/dev/zephyrproject2/modules/lib/nanopb 
-I/home/honza/dev/zephyrproject2/modules/hal/cmsis/CMSIS/Core/Include 
-I/home/honza/dev/zephyrproject2/zephyr/modules/cmsis/. 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/soc 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/Legacy 
-I/home/honza/dev/zephyrproject2/modules/hal/stm32/stm32cube/common_ll/include 
-isystem /home/honza/dev/zephyrproject2/zephyr/lib/libc/minimal/include 
-isystem /home/honza/dev/zephyrproject2/zephyr/lib/libc/common/include 
-isystem /home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/include 
-isystem /home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/include-fixed 
-fno-strict-aliasing 
-Os 
-imacros /home/honza/dev/zephyrproject2/poemodule/build/zephyr/include/generated/autoconf.h 
-ffreestanding 
-fno-common 
-g 
-gdwarf-4 
-fdiagnostics-color=always 
-mcpu=cortex-m7 
-mthumb 
-mabi=aapcs 
-mfp16-format=ieee 
--sysroot=/home/honza/zephyr-sdk-0.16.4/arm-zephyr-eabi/arm-zephyr-eabi 
-imacros /home/honza/dev/zephyrproject2/zephyr/include/zephyr/toolchain/zephyr_stdint.h 
-Wall 
-Wformat 
-Wformat-security 
-Wno-format-zero-length 
-Wdouble-promotion 
-Wno-pointer-sign 
-Wpointer-arith 
-Wexpansion-to-defined 
-Wno-unused-but-set-variable 
-Werror=implicit-int 
-fno-pic 
-fno-pie 
-fno-asynchronous-unwind-tables 
-Werror-implicit-function-declaration 
-fno-reorder-functions 
--param=min-pagesize=0 
-fno-defer-pop 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2/poemodule=CMAKE_SOURCE_DIR 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2/zephyr=ZEPHYR_BASE 
-fmacro-prefix-map=/home/honza/dev/zephyrproject2=WEST_TOPDIR 
-ffunction-sections 
-fdata-sections 
-D_POSIX_THREADS 
-std=c99 
-nostdinc 
-MD 
-MT modules/nanopb/CMakeFiles/modules__nanopb.dir/home/honza/dev/zephyrproject2/modules/lib/nanopb/pb_decode.c.obj 
-MF modules/nanopb/CMakeFiles/modules__nanopb.dir/home/honza/dev/zephyrproject2/modules/lib/nanopb/pb_decode.c.obj.d 
-o modules/nanopb/CMakeFiles/modules__nanopb.dir/home/honza/dev/zephyrproject2/modules/lib/nanopb/pb_decode.c.obj 
-c /home/honza/dev/zephyrproject2/modules/lib/nanopb/pb_decode.c

@xhpohanka
Copy link
Contributor Author

Reproduced in native_posix build as well...

ninja: Entering directory `buildp'
[1/2] ccache /usr/lib/ccache/bin/gcc -DKERNEL -DK_HEAP_MEM_POOL_SIZE=1024 -DMBEDTLS_CONFIG_FILE=\"config-tls-generic.h\" -DPB_MAX_REQUIRED_FIELDS=64 -D__ZEPHYR__=1 -I/zephyrproject2/poemodule/src -I/zephyrproject2/zephyr/include -I/zephyrproject2/poemodule/buildp/zephyr/include/generated -I/zephyrproject2/zephyr/soc/posix/inf_clock -I/zephyrproject2/zephyr/boards/posix/native_posix -I/zephyrproject2/zephyr/arch/posix/core/nsi_compat -I/zephyrproject2/zephyr/subsys/net/l2 -I/zephyrproject2/zephyr/subsys/net/lib/dns/. -I/zephyrproject2/modules/lib/nanopb -I/zephyrproject2/modules/crypto/mbedtls/include -I/zephyrproject2/zephyr/modules/mbedtls/configs -I/zephyrproject2/zephyr/modules/mbedtls/include -fno-strict-aliasing -Os -imacros /zephyrproject2/poemodule/buildp/zephyr/include/generated/autoconf.h -fno-common -g -gdwarf-4 -fdiagnostics-color=always -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wdouble-promotion -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -fno-asynchronous-unwind-tables -Werror-implicit-function-declaration -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/zephyrproject2/poemodule=CMAKE_SOURCE_DIR -fmacro-prefix-map=/zephyrproject2/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/zephyrproject2=WEST_TOPDIR -ffunction-sections -fdata-sections -m32 -msse2 -mfpmath=sse -include /zephyrproject2/zephyr/arch/posix/include/posix_cheats.h -fno-freestanding -std=c11 -MD -MT CMakeFiles/app.dir/src/main.c.obj -MF CMakeFiles/app.dir/src/main.c.obj.d -o CMakeFiles/app.dir/src/main.c.obj -c /zephyrproject2/poemodule/src/main.c
[2/2] : && ccache /usr/bin/cmake -E rm -f app/libapp.a && ccache /usr/bin/ar qc app/libapp.a  CMakeFiles/app.dir/src/libc_extensions.c.obj CMakeFiles/app.dir/src/main.c.obj CMakeFiles/app.dir/src/mheap.c.obj && ccache /usr/bin/ranlib app/libapp.a && :

$ ninja -C buildp -v modules/nanopb/all 
ninja: Entering directory `buildp'
[1/2] ccache /usr/lib/ccache/bin/gcc -DKERNEL -DK_HEAP_MEM_POOL_SIZE=1024 -DPB_MAX_REQUIRED_FIELDS=64 -D__ZEPHYR__=1 -I/zephyrproject2/zephyr/include -I/zephyrproject2/poemodule/buildp/zephyr/include/generated -I/zephyrproject2/zephyr/soc/posix/inf_clock -I/zephyrproject2/zephyr/boards/posix/native_posix -I/zephyrproject2/zephyr/arch/posix/core/nsi_compat -I/zephyrproject2/zephyr/subsys/net/l2 -I/zephyrproject2/zephyr/subsys/net/lib/dns/. -I/zephyrproject2/modules/lib/nanopb -fno-strict-aliasing -Os -imacros /zephyrproject2/poemodule/buildp/zephyr/include/generated/autoconf.h -fno-common -g -gdwarf-4 -fdiagnostics-color=always -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wdouble-promotion -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -fno-asynchronous-unwind-tables -Werror-implicit-function-declaration -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/zephyrproject2/poemodule=CMAKE_SOURCE_DIR -fmacro-prefix-map=/zephyrproject2/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/zephyrproject2=WEST_TOPDIR -ffunction-sections -fdata-sections -m32 -msse2 -mfpmath=sse -include /zephyrproject2/zephyr/arch/posix/include/posix_cheats.h -fno-freestanding -std=c11 -MD -MT modules/nanopb/CMakeFiles/modules__nanopb.dir/zephyrproject2/modules/lib/nanopb/pb_common.c.obj -MF modules/nanopb/CMakeFiles/modules__nanopb.dir/zephyrproject2/modules/lib/nanopb/pb_common.c.obj.d -o modules/nanopb/CMakeFiles/modules__nanopb.dir/zephyrproject2/modules/lib/nanopb/pb_common.c.obj -c /zephyrproject2/modules/lib/nanopb/pb_common.c
[2/2] : && ccache /usr/bin/cmake -E rm -f modules/nanopb/libmodules__nanopb.a && ccache /usr/bin/ar qc modules/nanopb/libmodules__nanopb.a  modules/nanopb/CMakeFiles/modules__nanopb.dir/

again build params

-DMBEDTLS_CONFIG_FILE=\"config-tls-generic.h\"
-I/zephyrproject2/modules/crypto/mbedtls/include
-I/zephyrproject2/zephyr/modules/mbedtls/configs
-I/zephyrproject2/zephyr/modules/mbedtls/include

are missing in external module build.

They was there in older zephyr.

@tejlmand
Copy link
Collaborator

just a very quick update now.

Thanks.

* I'm building a project for stm32h743 `west build -b nucleo_h743zi`.

Important information when having to reproduce.
Such information should always be part of the issue reporting.

* I noticed it first when merged the commit [5b4c927](https://github.com/zephyrproject-rtos/zephyr/commit/5b4c927c50eb032730767808b0304bf1043e2e91) if February

Thanks, always helpful as it can help tracing back.
But in this case this specific commit seems unrelated, but still helpful to reduce the commits to investigate during a bisect.

* I'm afraid your test is not trying to reproduce my issue, it checkes only the zephyr itself, not external modules

Then you must clarify what you mean by an external module, because mbedtls itself is an external module, and you can see the file from my investigation is located in a Zephyr module:

 -c /projects/github/ncs/modules/crypto/mbedtls/library/ssl_tls.c
    west workspace --^^^   ^^^            ^^^
       Zephyr modules ------|              |
                      mbedTLS module ----- |

and that file is being compiled with the expected flags.

* check the compile command I get for different files, for external ones the include paths of mbedtls are missing
  
  * `main.c` - internal project file
  * `pb_decode.c` - external module, supported

because the protobuf-nanopb-static is not a Zephyr library.
If the protobuf-nanopb-static should be build as a Zephyr library, then such request should actually go to the maintainer of the module.
FYI: @pdgendt

Question, why does pb_decode.c require mbedTLS headers ?

  * `civetweb.c` - external module (currently unsupported but it is the one where mbedtls is used for me`

As you say, support for civetweb was removed long ago: #46746
If you're using a new civetweb release which doesn't build the library as Zephyr libraries, then that's the reason those include paths are not part of civetweb.

The civetweb build code must ensure that include folders and flags from Zephyr is applied, either by creating a zephyr_library() or linking the civetweb library with zephyr_interface.

So you would need something like this:
https://github.com/zephyrproject-rtos/civetweb/blob/094aeb41bb93e9199d24d665ee43e9e05d6d7b1c/CMakeLists.txt#L401
https://github.com/zephyrproject-rtos/civetweb/blob/094aeb41bb93e9199d24d665ee43e9e05d6d7b1c/CMakeLists.txt#L427
https://github.com/zephyrproject-rtos/civetweb/blob/094aeb41bb93e9199d24d665ee43e9e05d6d7b1c/CMakeLists.txt#L462-L467

So far there doesn't seem to be any flaws in the build system.

What I can conclude is the following:

  • Libraries which requires include path defined by Zephyr must do one of the following:
    • Use zephyr_library for library creation, or
    • Link the CMake library with zephyr_interface.

So the reported issue is related to the library itself and it's glue code, and not related to Zephyr's mbedTLS intergration.

@tejlmand tejlmand removed the priority: high High impact/importance bug label May 24, 2024
@pdgendt
Copy link
Collaborator

pdgendt commented May 24, 2024

The nanopb sources are normally built as a library and compile options are added, see:

zephyr_library()
zephyr_library_sources(
${NANOPB_DIR}/pb_common.c
${NANOPB_DIR}/pb_encode.c
${NANOPB_DIR}/pb_decode.c
)
zephyr_include_directories(${NANOPB_DIR})
zephyr_compile_definitions(
PB_MAX_REQUIRED_FIELDS=${CONFIG_NANOPB_MAX_REQUIRED_FIELDS})
zephyr_compile_definitions_ifdef(
CONFIG_NANOPB_ENABLE_MALLOC
PB_ENABLE_MALLOC
)
zephyr_compile_definitions_ifdef(
CONFIG_NANOPB_NO_ERRMSG
PB_NO_ERRMSG
)
zephyr_compile_definitions_ifdef(
CONFIG_NANOPB_BUFFER_ONLY
PB_BUFFER_ONLY
)
zephyr_compile_definitions_ifdef(
CONFIG_NANOPB_WITHOUT_64BIT
PB_WITHOUT_64BIT
)
zephyr_compile_definitions_ifdef(
CONFIG_NANOPB_ENCODE_ARRAYS_UNPACKED
PB_ENCODE_ARRAYS_UNPACKED
)
zephyr_compile_definitions_ifdef(
CONFIG_NANOPB_VALIDATE_UTF8
PB_VALIDATE_UTF8
)

@tejlmand
Copy link
Collaborator

The nanopb sources are normally built as a library and compile options are added, see:

Sorry, my bad, I was looking here:
https://github.com/zephyrproject-rtos/nanopb/blob/7f88274070afa5edfaf608f4d8e32f3d3c1de139/CMakeLists.txt#L156

@xhpohanka
Copy link
Contributor Author

Hi @tejlmand,

I'm afraid we do not understand each other.

  • nanopb indeed does not need mbedtls, I used it just as an example of supported external module
  • I'm not aware of any external module requiring mbedtls now
  • I use civetweb with the same definitions of zephyr_library as you show in linked example
  • the issue is that mbedtls options were passed to the civetweb (or any other module as nanopb) build in older zephyr, now they are not

@xhpohanka
Copy link
Contributor Author

xhpohanka commented May 24, 2024

The zephyr build system is really really complex (that's one of the reason why many of my colleagues hate Zephyr) so I don't know if I'm fighting with issue of the module integration or the system itself.

  • if I use a module defining a zephyr_library() should the library be accessible by other modules?
  • should other modules do some work to access the other library module?

The situation has changed in time. For me before I merged the mentioned commit from master branch, build of external modules (even the nanopb) was given the options eg. -I/zephyrproject/modules/crypto/mbedtls/include from mbedtls.

@xhpohanka
Copy link
Contributor Author

Ok,

I have started to be curious what's going on and done the bisect.
The commit bringing the issue to me is the 7c72d4a.

From what I see there it seems to me that zephyr_library_link_libraries_ifdef(CONFIG_MBEDTLS mbedTLS) is important. If I understand it correctly, if library wants to use mbedtls (or any other library) it has to state this in its CMakeLists.txt. The original implementation of tls sockets made it available for whole build but probably it was just an unwanted coincidence.

@tejlmand please, can you confirm my finding is correct?

@tejlmand
Copy link
Collaborator

tejlmand commented May 24, 2024

The zephyr build system is really really complex

I agree, but considering the amount of boards, modules, integration with 3rd party modules which are all having their own way of doing things, etc. then things due become complex.
That's why we do try to keep some consistency in how things works in Zephyr

I don't know if I'm fighting with issue of the module integration or the system itself.

and that is what i'm trying to investigate, thus the reasons for my followup questions.

  • if I use a module defining a zephyr_library() should the library be accessible by other modules?

Depends on what you mean by accessible by other modules ?
But if a library provides headers which should be available to all Zephyr modules, then the module should call:

zephyr_include_directories(<dir>)

and then other libraries defined with zephyr_library() will inherit those include paths.
Thus the reason I try to narrow down exactly where / how the libs where you see this issue are created.

  • should other modules do some work to access the other library module?

If a library provides some public includes / compile flags which are not shared through zephyr_interface(), then an explicit link to the needed library is required.
For example using:

zephyr_library_link_libraries(<needed-lib>)

I have started to be curious what's going on and done the bisect.
The commit bringing the issue to me is the 7c72d4a.

From what I see there it seems to me that zephyr_library_link_libraries_ifdef(CONFIG_MBEDTLS mbedTLS) is important. If I understand it correctly, if library wants to use mbedtls (or any other library) it has to state this in its CMakeLists.txt. The original implementation of tls sockets made it available for whole build but probably it was just an unwanted coincidence.

Great. Thanks for finding.
And you're absolutely correct, the old use of zephyr_link_libraries_ifdef(CONFIG_MBEDTLS mbedTLS) would exactly make mbedTLS available to all libraries, which probably was unintended.

But going a step deeper, then this would only happen when CONFIG_NET_SOCKETS=y, because sockets code (and thus old use of zephyr_link_libraries()) is included here:

add_subdirectory_ifdef(CONFIG_NET_SOCKETS sockets)

This is exactly why it's important to have as detailed info as possible when strating an investigation.

@tejlmand please, can you confirm my finding is correct?

yes, the change of zephyr_link_libraries_ifdef() to zephyr_library_link_libraries_ifdef() would explain your observed change in behavior when CONFIG_NET_SOCKETS=y and CONFIG_MBEDTLS=y are set.

Now the question is if there are libraries which relies on mbedTLS but is not linking to it, simply because the library got it for "free".
Your inclusion of civetweb should also link to mbedTLS, now that you no longer get it for "free".

@xhpohanka
Copy link
Contributor Author

Thank you for explanation.
The correct way then is to use zephyr_library_link_libraries(MbedTLS) in mbedtls enabled civetweb CMakeLists.txt

The zephyr build system is really really complex

I agree, but considering the amount of boards, modules, integration with 3rd party modules which are all having their > > own way of doing things, etc. then things due become complex.
That's why we do try to keep some consistency in how things works in Zephyr

I agree too, but the truth is that eg. such issue with include files we met here is a show stopper for a most of my team :) Unfortunately there is no documentation or example of using zephyr_library_link_libraries() just the source code itself (well maybe zephyr/cmake/modules/extensions.cmake but I have found it only because I knew what to search for :) ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Security Security bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

No branches or pull requests

7 participants