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

bensze01 · 2024-12-20T16:51:23Z

Internal CI:
- mbed-tls-pr-test-parametrized
  - Mbed TLS + TF-PSA-Crypto
  - Mbed TLS
- mbedtls-release-ci-testing
  - Mbed TLS + TF-PSA-Crypto
  - Mbed TLS
OpenCI:
- mbed-tls-restricted-pr-test-parametrized
  - Mbed TLS + TF-PSA-Crypto
  - Mbed TLS
- mbedtls-release-ci-testing
  - Mbed TLS + TF-PSA-Crypto
  - Mbed TLS

Older runs:

9b82184
- Internal CI:
  - mbed-tls-pr-test-parametrized
    - Mbed TLS + TF-PSA-Crypto
    - Mbed TLS
  - mbedtls-release-ci-testing
    - Mbed TLS + TF-PSA-Crypto
    - Mbed TLS
- OpenCI:
  - mbed-tls-restricted-pr-test-parametrized
    - Mbed TLS + TF-PSA-Crypto
    - Mbed TLS
  - mbedtls-release-ci-testing
    - Mbed TLS + TF-PSA-Crypto
    - Mbed TLS

mpg

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

mpg · 2025-01-07T11:30:56Z

vars/gen_jobs.groovy

@@ -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) {


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?

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?

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?

I think it should.

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.

@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.

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

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).

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

vars/gen_jobs.groovy

bensze01 · 2025-02-12T04:54:30Z

@mpg I think I've addressed all the feedback.
Adding support for min_requirements.py required a major refactor that brought the design back closer to the original way get_branch_information() was set up, so I've squashed the back-and-forth changes to hopefully make reviewing this PR easier.

bensze01 · 2025-02-13T07:49:54Z

All the test runs completed successfully.

ronald-cron-arm · 2025-02-12T14:54:04Z

vars/gen_jobs.groovy

    if (env.RUN_ALL_SH == "true") {
-        info.all_all_sh_components.each({component, platform ->
+        info.all_sh_components.each({ component, platform ->


Suggested change

info.all_sh_components.each({ component, platform ->

info.all_sh_components.each({component, platform ->

vars/common.groovy

ronald-cron-arm · 2025-02-21T09:34:05Z

@bensze01 have you run new tests with the last version?

yanesca

I left a couple of remarks inline.

Also, commit "Test tf-psa-crypto components without Mbed TLS" a lot of things are happening:

Adds a function for unified handling of repo checkout
Adds another level of loop for dealing with multiple repos when collecting branch information
Restricts non-all.sh jobs to the tls repo (with the exception of analyse outcomes which follows in the next commit)
Extends prefix handling to handle multiple repos

I am not asking for any changes here, just for future reference, in similar cases breaking something like this down into more commits helps review and future development.

Overall I didn't find any blockers, the only reason I am not approving yet because I have a question about the test runs:
The reason for failure seems unrelated to this PR, still it makes me wonder if we understand what is going on there and why we don't want to get a green CI?

yanesca · 2025-02-21T13:35:09Z

vars/mbedtls.groovy

                    def jobs = infos.collectEntries { info ->
-                        String prefix = branches.size() > 1 ? "$info.branch-" : ''
+                        String prefix = ''


minor: This prefix calculation is repeated at least 4 times, maybe it would worth extracting it into a function.

yanesca · 2025-02-21T15:16:29Z

vars/common.groovy

-    }
+    Map<String, Collection<String>> repos = ['tls': tls_branches, 'tf-psa-crypto': tf_psa_crypto_branches]
+    // Filter out repos with no branches
+    repos = repos.findAll({repo, branches -> branches})


nitpick:
The explanation is there in the comment, but for my untrained eyes something like this would give a useful hint:

repos = repos.findAll{repo, branches -> branches != null}

(I would find the lack of brackets and the explicit instruction helpful pointers to parse the contents as a closure and not as a parameter list. The -> operator is already there, and should be enough, but for some reason it takes time end effort for me to pick up on that.)

On the other hand, since we have the comment there already, we could go for idiomatic groovy, something like this should work I believe:

repos = repos.findAll{it}

vars/mbedtls.groovy

yanesca

the only reason I am not approving yet because I have a question about the test runs

Oh, never mind, it is the ongoing CI incident.

Signed-off-by: Bence Szépkúti <[email protected]>

The Groovy CPS transformer doesn't support the use of the spread-dot operator on methods. Signed-off-by: Bence Szépkúti <[email protected]>

All Jenkins parameters are strings, which are not implicitly parsed when considering Groovy truthiness. Signed-off-by: Bence Szépkúti <[email protected]>

bensze01 · 2025-02-25T07:53:38Z

@mpg @yanesca I've rebased the PR on top of main, and all the CI runs passed.

yanesca

Did the rebase and arrived at the same result.

bensze01 added enhancement New feature or request needs: ci needs: review needs: reviewer size-m Estimated task size: medium (~1w) labels Dec 20, 2024

mpg reviewed Jan 7, 2025

View reviewed changes

bensze01 force-pushed the dev/bensze01/psa-crypto branch 12 times, most recently from 7b358ca to 15000e9 Compare February 12, 2025 04:40

bensze01 requested a review from mpg February 12, 2025 04:52

ronald-cron-arm self-requested a review February 12, 2025 14:48

bensze01 removed the needs: ci label Feb 13, 2025

ronald-cron-arm requested changes Feb 18, 2025

View reviewed changes

bensze01 force-pushed the dev/bensze01/psa-crypto branch from 29a641a to b745c14 Compare February 19, 2025 17:15

ronald-cron-arm requested review from yanesca and removed request for mpg February 20, 2025 09:25

ronald-cron-arm removed the needs: reviewer label Feb 20, 2025

bensze01 requested a review from mpg February 23, 2025 20:38

yanesca reviewed Feb 24, 2025

View reviewed changes

yanesca approved these changes Feb 24, 2025

View reviewed changes

bensze01 added 12 commits February 25, 2025 02:19

Rename all_all_sh_components

3049807

Signed-off-by: Bence Szépkúti <[email protected]>

Update wording of log message

42b9851

Signed-off-by: Bence Szépkúti <[email protected]>

Break out submodule checkout into separate methods

7e7a6a8

Signed-off-by: Bence Szépkúti <[email protected]>

Set RUN_TF_PSA_CRYPTO_ALL_SH in PSA Crypto PR jobs

b414ca9

Signed-off-by: Bence Szépkúti <[email protected]>

Test tf-psa-crypto components without Mbed TLS

559c967

Signed-off-by: Bence Szépkúti <[email protected]>

Skip running analyze_outcomes.py in TF-PSA-Crypto

e71c1ca

Signed-off-by: Bence Szépkúti <[email protected]>

Fix whitespace

d673ab9

Signed-off-by: Bence Szépkúti <[email protected]>

Add argument for PSA Crypto branches in get_branch_information()

36455d1

Signed-off-by: Bence Szépkúti <[email protected]>

Enable PSA-Crypto-by-itself tests in the framework PR jobs

03c94ca

Signed-off-by: Bence Szépkúti <[email protected]>

Fix usage of RUN_TF_PSA_CRYPTO_ALL_SH

41a7410

Signed-off-by: Bence Szépkúti <[email protected]>

Replace uses of the spread-dot operator with collect()

c5dbe5b

The Groovy CPS transformer doesn't support the use of the spread-dot operator on methods. Signed-off-by: Bence Szépkúti <[email protected]>

Fix check of boolean parameter

1cfc68d

All Jenkins parameters are strings, which are not implicitly parsed when considering Groovy truthiness. Signed-off-by: Bence Szépkúti <[email protected]>

bensze01 force-pushed the dev/bensze01/psa-crypto branch from 8687a61 to 1cfc68d Compare February 25, 2025 01:19

yanesca approved these changes Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bensze01 commented Dec 20, 2024 •

edited

Loading

mpg left a comment

mpg Jan 7, 2025

mpg Jan 7, 2025

ronald-cron-arm Jan 7, 2025

mpg Jan 8, 2025

valeriosetti Jan 9, 2025

bensze01 Jan 13, 2025

valeriosetti Jan 13, 2025

mpg Jan 13, 2025

bensze01 Jan 13, 2025

bensze01 commented Feb 12, 2025 •

edited

Loading

bensze01 commented Feb 13, 2025

ronald-cron-arm Feb 12, 2025

ronald-cron-arm commented Feb 21, 2025

yanesca left a comment •

edited

Loading

yanesca Feb 21, 2025

yanesca Feb 21, 2025

yanesca left a comment

bensze01 commented Feb 25, 2025

yanesca left a comment

	info.all_sh_components.each({ component, platform ->
	info.all_sh_components.each({component, platform ->

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

Are you sure you want to change the base?

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

Conversation

bensze01 commented Dec 20, 2024 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bensze01 commented Feb 12, 2025 • edited Loading

bensze01 commented Feb 13, 2025

Choose a reason for hiding this comment

ronald-cron-arm commented Feb 21, 2025

yanesca left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

bensze01 commented Feb 25, 2025

yanesca left a comment

Choose a reason for hiding this comment

bensze01 commented Dec 20, 2024 •

edited

Loading

bensze01 commented Feb 12, 2025 •

edited

Loading

yanesca left a comment •

edited

Loading