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

[Backport 2.28] Pass MBEDTLS_CONFIG_FILE defines through cmake #6974

Conversation

davidhorstmann-arm
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm commented Jan 27, 2023

Backport of #6914.

Conflicts:

  • library/CMakeLists.txt - the code that adds include directories and definitions has changed between 2.28 and development.
  • all.sh - 2.28 does not have the CMake as-package and as-package install tests.

Gatekeeper checklist

@davidhorstmann-arm davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Jan 27, 2023
When -DMBEDTLS_CONFIG_FILE or -DMBEDTLS_USER_CONFIG_FILE are passed to
cmake, pass them through as compile definitions. This allows different
mbedtls configs to be passed at configure time without modifying any
cmake files.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm force-pushed the 2.28-cmake-pass-through-config-defines branch from f6d1724 to 6762231 Compare July 5, 2023 13:35
@davidhorstmann-arm
Copy link
Contributor Author

Rebased, one conflict on the addition of the mbedtls_test_helpers target as with the main PR.

For the MBEDTLS_CONFIG_FILE and MBEDTLS_USER_CONFIG_FILE variables,
check that they are non-empty and defined. This means they can be
unconditionally created in the cache, simplifying the CMakeLists.txt

Signed-off-by: David Horstmann <david.horstmann@arm.com>
CMakeLists.txt Outdated
@@ -135,6 +135,14 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
FORCE)
endif()

# If set, make MBEDTLS_CONFIG_FILE and MBEDTLS_USER_CONFIG_FILE into PATHs
if(DEFINED MBEDTLS_CONFIG_FILE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these if(DEFINED ...) blocks due to an older version of CMake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not, I had just forgotten to update the backport. This works the same way according to the docs I have for CMake 2.8.12, which is the oldest supported for this branch.

Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be a faithful backport

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, faithful backport

@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jul 7, 2023
@tom-cosgrove-arm
Copy link
Contributor

OpenCI error is unrelated to this PR, and the corresponding Internal CI job passed, so CI is green

@tom-cosgrove-arm tom-cosgrove-arm removed the needs-ci Needs to pass CI tests label Jul 7, 2023
Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm removed the approved Design and code approved - may be waiting for CI or backports label Jul 7, 2023
@davidhorstmann-arm davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review and removed needs-reviewer This PR needs someone to pick it up for review labels Jul 7, 2023
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paul-elliott-arm paul-elliott-arm added the approved Design and code approved - may be waiting for CI or backports label Jul 7, 2023
@paul-elliott-arm
Copy link
Member

OpenCI break appears to be an internal problem. Internal CI passed, so all good.

@paul-elliott-arm paul-elliott-arm removed needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Jul 7, 2023
@paul-elliott-arm paul-elliott-arm merged commit dc1244d into Mbed-TLS:mbedtls-2.28 Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports size-s Estimated task size: small (~2d)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants