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

Alternative configuration files in CMake #4496

Closed
marekmosna opened this issue May 12, 2021 · 7 comments · Fixed by #6914
Closed

Alternative configuration files in CMake #4496

marekmosna opened this issue May 12, 2021 · 7 comments · Fixed by #6914
Labels
enhancement help-wanted This issue is not being actively worked on, but PRs welcome. size-s Estimated task size: small (~2d)

Comments

@marekmosna
Copy link

Enhancement / Feature Request

Suggested enhancement
Set the default value of MBEDTLS_CONFIG_FILE in the CMakeLists.txt and pass it to the build as compile flag

Justification - why does the library need this feature?
When you try to build the mbedtls as standalone library without any changes in the origin sources, you have to pass your own config file into the building process. Because there is no reference in the origin CMakeLists.txt at all, the CMake print out the warning about unused variable and do not pass it to the compilation as you are expecting.

Personally, I was forced to pass this parameter as a part of CMAKE_C_FLAGS which I find as pretty dirty solution.
I can make a pull request when you consider it as wanted enhancement(it can be even marked as the bug regarding to the how to configure document).

@tom-daubney-arm
Copy link
Contributor

tom-daubney-arm commented May 24, 2021

Hi @marekmosna !
Apologies for the late response on this, it must have slipped through our net.

I would like to take you up on that offer for a PR on this issue if you are happy to make it? It would give us a chance to properly see what change(s) you want and then make a proper decision regarding merging it into the repository.

Thanks a lot for your interest!
Tom

@tom-daubney-arm tom-daubney-arm added Community enhancement help-wanted This issue is not being actively worked on, but PRs welcome. size-s Estimated task size: small (~2d) labels May 24, 2021
@gilles-peskine-arm
Copy link
Contributor

Note that not all compilers support defining MBEDTLS_CONFIG_FILE (#include MBEDTLS_CONFIG_FILE is not standard C, but it's supported by an extension by some compilers). So it must not be passed unconditionally. In fact it should only be passed if it's explicitly enabled on the CMake command line.

@marekmosna
Copy link
Author

Hello guys,

please apologize my late response but job is job and money is money. I can propose 2 different approaches how we can improve the custom configuration file for mbedtls.

  1. Improve building scripts as CMakeListst.tx and Makefile and define MBEDTLS_CONFIG_FILE as command line declared stand-alone parameter -DMBEDTLS_CONFIG_FILE . It will not be part of the CFLAGS but scripts will be dealing with it and at the end they will pass it to the build.
  • pros:
    • Pull request doesn't change any line of C/C++ code
    • existing library deployments don't need to change the building system
  • cons:
  1. The approach that is widely used and which you can find in many libraries (for instance LwIP ) and it needs wrap every record in include/mbedtls_config.h by #ifndef and include there #include "mbedtls_opts.h at the beginning (name is up to you) where customer can make his own configuration. Now it gives customers opportunity to define their own include path where the mbedtls library will be searching for such a config.
  • pros:
    • easy to understand and manage you own deployment configuration by placing various configs to different folders
    • straightforward so chance to make a silly mistake turns to zero
  • cons:
    • deploy library with default custom config file (it will be empty file end it means that it will not redefine any record in include/mbedtls_config.h )which can by placed in configs/mbedtls_opts.h and ensure that /configs will be part of the include paths

Let me know your opinions.
Regards
@marekmosna

@gilles-peskine-arm
Copy link
Contributor

We already have part of (2): in addition to MBEDTLS_CONFIG_FILE, which replaces the default configuration, there's also MBEDTLS_USER_CONFIG_FILE, which is included after the default configuration. You can write a mbedtls_user_config.h containing #undef for features you want to turn off and #define for features you want to turn on, and compile with -DMBEDTLS_USER_CONFIG_FILE.

The advantage of making the file name configurable is that you can make different build trees with different configuration files, by using different -DMBEDTLS_(USER_)CONFIG_FILE options for the different builds. The downside is that not all compilers support this.

In any case the cmakefiles don't have any particular support for this, you have to pass -D… in CMAKE_C_FLAGS.

@marekmosna
Copy link
Author

marekmosna commented Sep 2, 2021

I see. Ok so I show you what I was forced to do to support custom configuration on arm platform and maybe you just point me to the right direction.

The application CMakeLists.txt contains such a code
`
set(MCU_FLAGS -mthumb -mcpu=cortex-m4 -mfloat-abi=hard -mfpu=fpv4-sp-d16)
list(
APPEND
MBED_COMPILE_OPTIONS
"-DMBEDTLS_CONFIG_FILE=<${CMAKE_CURRENT_SOURCE_DIR}/../../include/mbedtls_config.h>;${MCU_FLAGS}"
)
set(MBEDTLS_BINARY_DIR ${CMAKE_BINARY_DIR}/lib/mbedtls-release)

add_custom_target(
mbedtls-release
COMMAND
${CMAKE_COMMAND} -DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
-DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE} -DCMAKE_BUILD_TYPE=Release
-DCUSTOM_COMPILE_OPTIONS="${MBED_COMPILE_OPTIONS}"
-DCMAKE_STATIC_LINKER_FLAGS_RELEASE="${CMAKE_STATIC_LINKER_FLAGS_RELEASE}" -DENABLE_PROGRAMS=OFF
-DENABLE_TESTING=OFF -S ${CMAKE_CURRENT_SOURCE_DIR}/../SSL -B ${MBEDTLS_BINARY_DIR} -G Ninja
COMMAND ${CMAKE_COMMAND} --build ${MBEDTLS_BINARY_DIR} --target mbedtls
COMMENT "MbedTLS v2.26.0 relase build" USES_TERMINAL
)`

but unfortunately because of the Linux build I was forced to modify your CMakelists.txt such way:

`set(CUSTOM_COMPILE_OPTIONS
""
CACHE STRING "Allows adding custom C/C++ flags"
)

if(CUSTOM_COMPILE_OPTIONS)
string(REPLACE " " ";" CUSTOM_COMPILE_OPTIONS "${CUSTOM_COMPILE_OPTIONS}")
message(STATUS "Additional compile options: ${CUSTOM_COMPILE_OPTIONS}")
add_compile_options(${CUSTOM_COMPILE_OPTIONS})
endif(CUSTOM_COMPILE_OPTIONS)`

@gilles-peskine-arm
Copy link
Contributor

I'm sorry, I know very little about cmake. Some other maintainers know a bit more and may have an idea. It might be worth asking on the mailing list to see if other people have run into the same issue and how they resolved it.

@davidhorstmann-arm
Copy link
Contributor

davidhorstmann-arm commented Jul 3, 2023

Noting that this will be addressed by #6914.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help-wanted This issue is not being actively worked on, but PRs welcome. size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants