Skip to content

Move private headers to a private directory #175

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

amtkarm1
Copy link

Description

Fixes Mbed-TLS/mbedtls#10087.

PR checklist

@amtkarm1 amtkarm1 self-assigned this Jun 16, 2025
@amtkarm1 amtkarm1 added priority-high High priority - will be reviewed soon needs-work needs-review Every commit must be reviewed by at least two team members, size-s Estimated task size: small (~2d) labels Jun 16, 2025
@gilles-peskine-arm gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Jun 16, 2025
@amtkarm1 amtkarm1 force-pushed the task-private-headers branch 3 times, most recently from fa973f5 to 73f84ca Compare June 16, 2025 23:29
@@ -17,7 +17,7 @@

#if defined(MBEDTLS_BIGNUM_C)

#include <mbedtls/bignum.h>
#include <mbedtls/private/bignum.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good for development, but the framework needs to keep working with 3.6 as well. I suggest

#if MBEDTLS_VERSION_MAJOR >= 4
#include <mbedtls/private/bignum.h>
#else
#include <mbedtls/bignum.h>
#endif

(Here and in all similar places obviously.) (Edit: MBEDTLS_VERSION_MAJOR is defined by including build_info.h which should already be included everywhere.)

I also suggest you create a "shadow" 3.6 PR where the only thing you do is change the framework pointer to the head of this PR.

Your goal is now to get a green CI both on the mbedtls development PR and the shadow 3.6 PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also suggest you create a "shadow" 3.6 PR where the only thing you do is change the framework pointer to the head of this PR.

Based on the results of CI for this PR it seems to me that:

  1. development of tf-psa-crypto: OK
  2. development of mbedtls repo: FAIL
  3. mbedtls-3.6 of mbedtls: OK

As far as I can tell 3.6 testing should be fine, but then I'm a bit surprised that tf-psa-crypto is also OK with the files being moved while development of mbedtls repo fails. I would expect both to fail for the same reason, i.e. files being moved. This made me think on the proposed suggestion of #if MBEDTLS_VERSION_MAJOR >= 4: shouldn't we also check PSA_CRYPTO_API_VERSION_MAJOR for the case in which tf-psa-crypto repo is built as standalone?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not PSA_CRYPTO_API_VERSION_xxx: that's the API version which is irrelevant. We'd need TF_PSA_CRYPTO_VERSION_xxx (or whatever the name is supposed to be) which doesn't exist yet.

Copy link
Contributor

@valeriosetti valeriosetti Jun 24, 2025

Choose a reason for hiding this comment

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

OK, thanks for the details!
Therefore I suspect that we might have issues with this PR series as follows. CI tests in this PR for standalone tf-psa-crypto here are passing just because the the current default branch of tf-psa-crypto is used and the old headers' path is picked. But there are tests on Mbed-TLS/TF-PSA-Crypto#318 which are failing due to missing test headers (I didn't check them all, but at least test_tf_psa_crypto_out_of_source does):

[2025-06-24T10:46:42.522Z] [  1%] Building C object CMakeFiles/tf_psa_crypto_test.dir/framework/tests/src/bignum_helpers.c.o
[2025-06-24T10:46:42.522Z] In file included from /var/lib/build/framework/tests/src/bignum_helpers.c:14:
[2025-06-24T10:46:42.522Z] /var/lib/build/framework/tests/include/test/bignum_helpers.h:23:10: fatal error: 'mbedtls/bignum.h' file not found
[2025-06-24T10:46:42.522Z] #include <mbedtls/bignum.h>

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sorry, my suggestion was not correct, I noticed and discussed it on Slack but failed to report the outcome of the discussion here. I think while waiting for TF_PSA_CRYPTO_VERSION_xxx I suggest

#if !defined(MBEDTLS_VERSION_MAJOR) || MBEDTLS_VERSION_MAJOR >= 4
#include <mbedtls/private/bignum.h>
#else
#include <mbedtls/bignum.h>
#endif

In tf-psa-crypto standalone, MBEDTLS_VERSION_MAJOR will not be defined. In mbedtls development, it will be defined and >= 4. Those are the two cases where we want the new path. In mbedtls 3.6, it will be defined and < 4; this is the only case where we want the old path.

(Alternatively, we could make this whole series of PRs depend on c323 but I'd rather not: this is a very conflict-prone series of PR so we should try not to delay it if we can avoid it.

@valeriosetti valeriosetti self-requested a review June 23, 2025 07:55
amtkarm1 added 4 commits June 24, 2025 11:44
…n the tf-psa-crypto section)

Signed-off-by: Anton Matkin <[email protected]>
Signed-off-by: Anton Matkin <[email protected]>
…namely, now the inclusioin of header files in the private/ folder is conditional

Signed-off-by: Anton Matkin <[email protected]>
…on-private) path is only taken in case of mbedtls 3.6

Signed-off-by: Anton Matkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-work priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Development

Successfully merging this pull request may close these issues.

Move private headers to a private subdirectory
4 participants