Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Run TF-PSA-Crypto components without cloning Mbed TLS #190

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

Conversation

bensze01
Copy link
Contributor

@bensze01 bensze01 commented Dec 20, 2024

Fixes Mbed-TLS/TF-PSA-Crypto#116

Test runs: TBD

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
@bensze01 bensze01 added enhancement New feature or request needs: ci needs: review needs: reviewer size-m Estimated task size: medium (~1w) labels Dec 20, 2024
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Just making myself familiar with the code.

Note: we'll want a test run showing that if a tf-psa-crypto component tries to reference an Mbed TLS file, it fails. (We'll probably want to create a faulty PR for that purpose.)

vars/common.groovy Outdated Show resolved Hide resolved
@@ -245,7 +245,7 @@ echo >&2 'Note: "clang" will run /usr/bin/clang -Wno-error=c11-extensions'
'''
}

if (info.has_min_requirements) {
if (repo == 'tls' && info.has_min_requirements) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why we need the repo == 'tls' part: currently tf-psa-crypto doesn't have scripts/min_requirements.py but I'd guess info.has_min_requirements should reflect that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it seems to me we'll need to get scripts/min_requirements.py (and supporting files) in tf-psa-crypto (or the framework) before moving python scripts that have requirements beyond what happens to be already installed by apt. @ronald-cron-arm not sure if that dependency was already on your radar?

Copy link
Contributor

Choose a reason for hiding this comment

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

No that was not on my radar. I can see that @valeriosetti has started working on Mbed-TLS/mbedtls-framework#86. Does this issue fully cover the dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should.

Choose a reason for hiding this comment

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

I can see that @valeriosetti has started working on Mbed-TLS/mbedtls-framework#86.

Yes, PRs Mbed-TLS/mbedtls-framework#105 and related ones (Mbed-TLS/mbedtls#9863 Mbed-TLS/mbedtls#9864) are ready for review. There's only the usual framework confict on development and mbedtls-3.6, but that can easily be fixed once Mbed-TLS/mbedtls-framework#105 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpg Currently the code only checks if min_requirements.py exists in the MbedTLS branch we are testing, and this is what's represented by has_min_requirements. Since we're planning to add such a script, I'll add a separate variable to track this for the framework.

Choose a reason for hiding this comment

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

For the records, I created Mbed-TLS/TF-PSA-Crypto#148 to add min_requirements.py to TF-PSA-Crypto

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're planning to add such a script, I'll add a separate variable to track this for the framework.

Not sure if this was a typo, but just to be sure: I was talking about tf-psa-crypto, not the framework. Currently the plan is for scripts/min_requirements.py to exist only in the two "top-level" repos (mbedtls, tf-psa-crypto).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes sorry, that was a typo - I meant tf-psa-crypto.

vars/gen_jobs.groovy Outdated Show resolved Hide resolved
vars/gen_jobs.groovy Outdated Show resolved Hide resolved
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request needs: ci needs: review needs: reviewer size-m Estimated task size: medium (~1w)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: run all.sh in a pure tf-psa-crypto environment
4 participants