Run Crypto abi-check in TF-PSA-Crypto PRs #241

bensze01 · 2025-12-12T17:15:46Z

Description

Run Crypto abi-check in TF-PSA-Crypto PRs.
Running the TLS ABI check doesn't do much, since both HEAD and FETCH_HEAD point to the same commit in a Crypto PR-merge job.

PR checklist

framework PR: Update abi_check.py to support standalone tf-psa-crypto mbedtls-framework#229
mbedtls-test PR: Run Crypto abi-check in TF-PSA-Crypto PRs #241
TF-PSA-Crypto PR: Update framework submodule TF-PSA-Crypto#541
development PR: Move abi_check.py to the framework mbedtls#10464
3.6 PR: [Backport 3.6] Move abi_check.py to the framework mbedtls#10465

CI runs

TF-PSA-Crypto ABI-check

gilles-peskine-arm · 2025-12-15T17:22:03Z

You link to https://ci.trustedfirmware.org/view/Mbed-TLS/job/mbed-tls-tf-psa-crypto-restricted-ci-testing/view/change-requests/job/PR-12-merge/1/ as a CI run, but that doesn't seem to have run the abi-check?

bensze01 · 2025-12-15T17:26:04Z

Ah - I didn't update the test branch with my changes that added abi_check.py to TF-PSA-Crypto. One moment.

gilles-peskine-arm

If I understand correctly, the problem is that currently, when we run the “Interface stability tests” in TF-PSA-Crypto, it's actually comparing the interface between the current version and the current version? That's a really nasty bug, thanks for spotting it!

I don't understand the fix, however.

src/org/mbed/tls/jenkins/BranchInfo.groovy

vars/common.groovy

vars/mbedtls.groovy

Running the TLS ABI check doesn't do much, since both HEAD and FETCH_HEAD point to the same commit in a Crypto PR-merge job. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

bensze01 · 2025-12-16T12:09:00Z

I went ahead and removed all of the file presence detection.

Since this reduced the PR size by a lot, I've squashed the back-and-forth changes.

TARGET_REPO is more generic and better named for what it actually does. eg. This allows us to run abi_check.py on TF-PSA-Crypto branches without enabling RUN_TF_PSA_CRYPTO_ALL_SH. Also plumb TARGET_REPO for run_release_job(). Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

minosgalanakis · 2025-12-16T15:37:47Z

vars/mbedtls.groovy

-void run_release_job(String tls_branches, String tf_psa_crypto_branches) {
+void run_release_job(String target_repo, String tls_branches, String tf_psa_crypto_branches) {
    def (tls_split,    tf_psa_crypto_split) =
        [tls_branches, tf_psa_crypto_branches].collect({branches -> branches.split(',').findAll()})


Not a blocker but would it make sense to filter out whitespace?

def (tls_split, tf_psa_crypto_split) = [tls_branches, tf_psa_crypto_branches].collect { s -> (s ?: '').split(/\s*,\s*/).findAll { it } }

minosgalanakis

Looking closer at it, it depends on the state of all the other prs in the group.

gilles-peskine-arm

I think this is correct, but I want better documentation.

gilles-peskine-arm · 2025-12-16T18:15:12Z

vars/mbedtls-release-Jenkinsfile

 * This script takes the following parameters:
 *
 * Repos and branches
+ *  - TARGET_REPO


Looking it up in https://review.trustedfirmware.org/plugins/gitiles/ci/mbedtls/mbed-tls-job-configs/+/refs/heads/openci-migration/mbed-tls-restricted-pr-test-parametrized.yaml the possible values of TARGET_REPO are tls, tf-psa-crypto and framework. Can we make them consistently the actual repo names, or consistently short versions? Either mbedtls, TF-PSA-Crypto, mbedtls-framework or tls, crypto, framework.

And how do restricted repos fit into this?

Can we make them consistently the actual repo names, or consistently short versions? Either mbedtls, TF-PSA-Crypto, mbedtls-framework or tls, crypto, framework.

I'd like to move us to using enums (perhaps in a separate PR), but unfortunately - is not valid in groovy identifiers.

Just to orient myself towards the eventual goal - would you prefer the style RepoType.crypto or RepoType.TF_PSA_CRYPTO?

It's not something we'll have to type on a daily basis, so I'd prefer to just use the repository name.

I don't really mind if it's a string. We don't really need the whole Groovy code to know about all the possible repositories.

Can we make them consistently the actual repo names, or consistently short versions? Either mbedtls, TF-PSA-Crypto, mbedtls-framework or tls, crypto, framework.

I'd like to move us to using enums (perhaps in a separate PR), but unfortunately - is not valid in groovy identifiers.

Just to orient myself towards the eventual goal - would you prefer the style RepoType.crypto or RepoType.TF_PSA_CRYPTO?

I would advise against enums at this stage with groovy. I agree with Gilles to keep in String format.

And how do restricted repos fit into this?

TEST_BRANCH specifies the "type" of the repo, ie. the value of which variable we use out of MBED_TLS_REPO, TF_PSA_CRYPTO_REPO and FRAMEWORK_REPO.

I've detailed the exact effects it has at the moment in the Jenkinsfiles.

vars/mbedtls-release-Jenkinsfile

gilles-peskine-arm · 2025-12-16T18:20:06Z

vars/mbedtls-release-Jenkinsfile

 * This script takes the following parameters:
 *
 * Repos and branches
+ *  - TARGET_REPO


Note that this is new and currently only available in the mbed-tls-restricted-pr-test-parametrized job, from the openci-migration branch of the job configs which is still the one that the new CI uses.

I need to push a change to openci-migration anyways to adapt to the new values of TARGET_REPO - I'll just add it to the other parametrized jobs as well instead of documenting this as a limitation.

Done: https://review.trustedfirmware.org/c/ci/mbedtls/mbed-tls-job-configs/+/46425

This way we don't need to add the shell script to the TF-PSA-Crypto repo. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

The intent is to mimic the behaviour of the three CIs, so don't skip TF-PSA-Crypto tests if the target is 'framework'. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

gilles-peskine-arm

LGTM on code inspection except for a typo.

We'll need to do test runs for all three values of TARGET_REPO, both release and pr-merge, maybe also some test runs in restricted repositories, to make sure that we haven't forgotten a place where a repo name needs to be updated. (Admittedly an enum would give us more confidence there. Note that I'm not against an enum, I just don't think it's in scope here.)

vars/mbedtls-pr-test-Jenkinsfile

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

minosgalanakis

LGTM

gilles-peskine-arm

LGTM on code inspection

bensze01 added this to Roadmap pull requests (new board) Dec 12, 2025

bensze01 added enhancement New feature or request needs: ci needs: review needs: reviewer priority-high size-xs Estimated task size: extra small (a few hours at most) labels Dec 12, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Dec 12, 2025

gilles-peskine-arm requested changes Dec 15, 2025

View reviewed changes

src/org/mbed/tls/jenkins/BranchInfo.groovy Outdated Show resolved Hide resolved

vars/common.groovy Outdated Show resolved Hide resolved

vars/mbedtls.groovy Outdated Show resolved Hide resolved

gilles-peskine-arm added needs: work and removed needs: review needs: reviewer labels Dec 15, 2025

bensze01 added 2 commits December 16, 2025 13:06

Run Crypto abi-check in TF-PSA-Crypto PRs

67cb2b3

Running the TLS ABI check doesn't do much, since both HEAD and FETCH_HEAD point to the same commit in a Crypto PR-merge job. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

Allow TARGET_REPO to be parametrized

1d08642

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

bensze01 force-pushed the dev/bensze01/crypto-abi-check branch from 7b89051 to 1d08642 Compare December 16, 2025 12:07

bensze01 requested a review from gilles-peskine-arm December 16, 2025 12:09

minosgalanakis self-requested a review December 16, 2025 14:52

minosgalanakis reviewed Dec 16, 2025

View reviewed changes

minosgalanakis previously approved these changes Dec 16, 2025

View reviewed changes

minosgalanakis self-requested a review December 16, 2025 17:02

gilles-peskine-arm requested changes Dec 16, 2025

View reviewed changes

Call list_internal_identifiers.py directly

cd02e62

This way we don't need to add the shell script to the TF-PSA-Crypto repo. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

bensze01 added 3 commits December 17, 2025 15:42

Fix TARGET_REPO logic for filtering tests

b739688

The intent is to mimic the behaviour of the three CIs, so don't skip TF-PSA-Crypto tests if the target is 'framework'. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

Use exact repo names in TARGET_REPO

d3d4a4f

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

Document TEST_REPO in Jenkinsfiles

bfa3990

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

bensze01 dismissed minosgalanakis’s stale review via bfa3990 December 17, 2025 15:33

bensze01 requested a review from gilles-peskine-arm December 17, 2025 16:33

bensze01 removed the needs: work label Dec 17, 2025

gilles-peskine-arm requested changes Dec 17, 2025

View reviewed changes

vars/mbedtls-pr-test-Jenkinsfile Outdated Show resolved Hide resolved

gilles-peskine-arm added the needs: work label Dec 17, 2025

Fix typo in Jenkinsfiles

f271e51

Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

bensze01 requested a review from gilles-peskine-arm December 17, 2025 17:02

bensze01 removed the needs: work label Dec 17, 2025

minosgalanakis approved these changes Dec 17, 2025

View reviewed changes

gilles-peskine-arm approved these changes Dec 17, 2025

View reviewed changes

github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Dec 17, 2025

gilles-peskine-arm added the approved Approved in review. May need additional CI. label Dec 17, 2025

Run Crypto abi-check in TF-PSA-Crypto PRs #241

Are you sure you want to change the base?

Run Crypto abi-check in TF-PSA-Crypto PRs #241

Uh oh!

Conversation

bensze01 commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

CI runs

Uh oh!

gilles-peskine-arm commented Dec 15, 2025

Uh oh!

bensze01 commented Dec 15, 2025

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bensze01 commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bensze01 commented Dec 12, 2025 •

edited

Loading

minosgalanakis left a comment •

edited

Loading