All.sh add support for tf-psa-crypto components #9720

mpg · 2024-10-24T08:18:02Z

Description

Add tf-psa-crypto/tests/scripts/all.sh that can be invoked independently and will source components from tf-psa-crypto/tests/scripts/component-*.sh. Ultimately this is going to be the main entry point for tf-psa-crypto's CI when all relevant components have been migrated.
Turn tests/scripts/all.sh into a wrapper that handles both mbedtls and tf-psa-crypto. This is a transitional wapper so that the CI can just keep calling the same single entry point, and still get all components, while we move relevant components to tf-psa-crypto, and until tf-psa-crypto gets its own autonomous CI.

PR checklist

changelog not required because: internal changes, tests only
development PR here
framework PR not required
3.6 PR provided [Backport 3.6] All.sh changes to support tf-psa-crypto components #9754
2.28 PR not required because: 2.28 is too old for that
tests not required because: manual testing, see below

Manual testing

Once the CI is passing, check the outcome file to confirm we're still running the same number of components.
Anything else reviewers can think of - time to be creative :)

In preparation for adding tf-psa-crypto/test/scripts/all.sh which will run from tf-psa-crypto. Use paths relative to the currently sourced file when including common files (ie, those that will soon be moved to the framework). Otherwise, use paths relative to the current directory, aka project's root. Document that test/script/all.sh must be invoked from the project's root (that was already the case, but implicit so far). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

The existing check only took care of CONFIG_H. This was both not enough and too much: - not enough because config.py can also modify CRYPTO_CONFIG_H and we want to know about it just as much as CONFIG_H; - too much because CONFIG_H does not exist in tf-psa-crypto. Check a list of files instead of a single one, and adjust that list. Also update an outdated comment about Makefiles Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

With a first component. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

In order to avoid changing the CI job's definition (which fewer team members understand, compared to shell scripts), just have a wrapper so that all.sh does both mbedtls and tf-psa-crypto under the hood for now. When tf-psa-crypto has its own CI running its own all.sh (with enough components migrated to get sufficient coverage) we can remove this hack. Rejected strategy: modify all-core.sh so that when running in mbedtls, it also sources components-*.sh from tf-psa-crypto, remembers which components come from Mbed TLS and which come from crypto, and magically adjust the environment for each component it runs. Rejected because it's hard to be confident we're adjusting everything that needs adjusting in the environment. Having separate processes seems much safer. The downside is we get more complexity around error handling (check for components requested but not available, --keep-going). When using --keep-going with failing component(s) in mbedtls and some components in tf-psa-crypto, the output is not satisfying for humans as we don't have a nice summary of all errors at the end like we normally would. IMO this is acceptable since: - this wrapper is transitional and should be removed in a few months; - it is mainly for the benefit of the CI; humans can always invoke the underlying commands directly. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

ronald-cron-arm

It looks promising to me. However ./tests/scripts/all.sh test_cmake_tf_psa_crypto_out_of_source does not seem to work (list of arguments empty for mbedtls-all.sh thus running all the components?). As I said in a comment, it would be more convenient if possible to support components with the same name in Mbed TLS and TF-PSA-Crypto. Thinking about it, we would probably need to change something in outcome.csv to differentiate the test results of such components.

tf-psa-crypto/tests/scripts/components-build-system.sh

tests/scripts/all.sh

gilles-peskine-arm · 2024-10-24T17:51:18Z

tests/scripts/all.sh

@@ -1,15 +1,96 @@
 #! /usr/bin/env bash



Note that this conflicts with #9293, which I hope we can merge soon since all it's missing is one approval on one backport.

This also conflicts with #9286, but that's not quite ready yet, so I suggest that we go 9293, then 9720, then 9286.

Sorry, I'm not seeing the conflict with 9293 - it seems to me that the two PRs are not touching the same files. What am I missing?

My bad, there's no merge conflict with 9293 and I don't think there's a semantic conflict either. There's a potential semantic conflict with #9286 since it will error out if a component name is duplicated between mbedtls and tf-psa-crypto.

I ran a merge of something that conflicted but that must have been some different branches or with a non-up-to-date local copy and I can't find exactly what I ran yesterday anymore. I have a conflict checker script and it reports no conflict between the current state of this PR and any other PR opened in the last 6 months. #9293 only conflicts in .gitignore with a priority-medium PR.

There's a potential semantic conflict with #9286 since it will error out if a component name is duplicated between mbedtls and tf-psa-crypto.

Well, currently my PR also forbids component names that are duplicated between mbedtls and tf-psa-crypto, so that shouldn't be a problem. As I said earlier, I don't think we should allow components with the same name on both sides until the sides are completely separated.

gilles-peskine-arm · 2024-10-24T17:53:54Z

we would probably need to change something in outcome.csv to differentiate the test results of such components.

Presumably MBEDTLS_TEST_CONFIGURATION should ensure that the component names are different.

Note that #9286 (which we plan to merge this quarter) will make it an error to run use the same value of MBEDTLS_TEST_CONFIGURATION twice on the same platform.

mpg · 2024-10-25T08:15:54Z

However ./tests/scripts/all.sh test_cmake_tf_psa_crypto_out_of_source does not seem to work (list of arguments empty for mbedtls-all.sh thus running all the components?)

Aw, silly me, forgot to test the simple case of a single component. Will fix.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2024-10-30T08:05:41Z

Internal CI fully passed, and OpenCI only failed the armcc components which are a known temporary issue.

mpg · 2024-10-30T08:39:09Z

The component that was moved to tf-psa-crypto is not showing up in outcomes.csv.xz. I checked the pipeline log and it ran, so the problem is that the outcome is not recorded. I strongly suspect that's become the path to the outcomes file is relative, so the file will be created in tf-psa-crypto where it's not going to be picked up later when all the outcomes files from each component are concatenated.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2024-10-30T11:59:39Z

The component that was moved to tf-psa-crypto is not showing up in outcomes.csv.xz. I checked the pipeline log and it ran, so the problem is that the outcome is not recorded. I strongly suspect that's become the path to the outcomes file is relative, so the file will be created in tf-psa-crypto where it's not going to be picked up later when all the outcomes files from each component are concatenated.

The last commit seems to have fixed this. I downloaded the outcomes file from the Open CI run (Internal CI has not completed yet) then extracted the list of components with cut -d';' -f2 outcomes-new.csv | sort -u and compared to the outcomes files of the nightly run from 23 Oct (when this PR was branched out), and the only difference was the various component_release that are expected to be run only in the nightly, so everything's as expected.

Harry-Ramsey · 2024-11-04T10:11:41Z

tests/scripts/all.sh

+    fi
+fi
+
+if [ "$#" -ne 4 -o "$1" != '--seed' -o "$3" != '--keep-going' ]; then


If I attempt to run the script using the following command ./tests/scripts/all.sh test_cmake_tf_psa_crypto_out_of_source I encounter the following error:

./tests/scripts/all.sh: line 58: $3: unbound variable

I believe this is incorrect and I should either be greeted with the error or the command executes?

Ah right, good point, that's poor error handling. I'll fix it.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Harry-Ramsey

Thanks, LGTM.

eleuzi01

LGTM, thanks!

gilles-peskine-arm

Gatekeeper note: I'm blocking merging because I'm concerned that this may introduce an intermittent CI failure. Feel free to override my change request if my concern is invalid.

gilles-peskine-arm · 2024-11-06T17:54:28Z

tf-psa-crypto/tests/scripts/components-build-system.sh

+    mkdir "$OUT_OF_SOURCE_DIR"
+    cd "$OUT_OF_SOURCE_DIR"
+    # Note: Explicitly generate files as these are turned off in releases
+    cmake -D CMAKE_BUILD_TYPE:String=Check -D GEN_FILES=ON "$TF_PSA_CRYPTO_ROOT_DIR"


Echoing Harry's concern in #9755 (comment) : we currently prevent component_test_cmake_out_of_source from running on Ubuntu 16.04, but now component_test_cmake_tf_psa_crypto_out_of_source will run on Ubuntu 16.04. Why is this not a problem? I don't see it discussed in the review here.

The problem is a race condition. Just having a small number of successful CI runs is not enough to gain confidence.

Maybe the cmake file in tf-psa-crypto is better written and is not susceptible to the race condition. That's quite plausible, but do we actually know that?

The component component_test_cmake_tf_psa_crypto_out_of_source is not added by this PR. This PR moves it from tests/scripts/components-build-system.sh to tf-psa-crypto/tests/scripts/components-build-system.sh. The component was added by the PR 9445 without protection against Ubuntu 16.04 (we just missed that point I think) which was merged October the 3rd. Thus it has been around for more that a month now. Enough to gain confidence?

@gilles-peskine-arm I think your concern is valid in general, but quite unrelated to this PR, as Ronald explained.

Feel free to override my change request if my concern is invalid.

Since you offered, I'm gonna do just that. I'm also going to start a discussion on slack about the potential issue and things we could do about it.

Ok, I'd missed that it was a move and not new. I agree that a month of CI is enough to have confidence that this isn't a problem with the tf-psa-crypto cmake scripts.

The point raised is pre-existing and not made any worse by this PR; if we do something about it there's not reason it should be in this PR.

mpg added 5 commits October 23, 2024 10:06

Add tf-psa-crypto's own all.sh

8bcad48

With a first component. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

all.sh: prevent a silly mistake

a4f0227

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg self-assigned this Oct 24, 2024

ronald-cron-arm reviewed Oct 24, 2024

View reviewed changes

tf-psa-crypto/tests/scripts/components-build-system.sh Show resolved Hide resolved

tests/scripts/all.sh Outdated Show resolved Hide resolved

tests/scripts/all.sh Outdated Show resolved Hide resolved

tests/scripts/all.sh Outdated Show resolved Hide resolved

gilles-peskine-arm reviewed Oct 24, 2024

View reviewed changes

davidhorstmann-arm self-requested a review October 29, 2024 10:16

mpg requested a review from bensze01 October 29, 2024 10:17

New all.sh wrapper with reduced scope (CI only)

6ffebef

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix making the path of the outcomes file absolute.

d10f42f

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg requested review from Harry-Ramsey and removed request for davidhorstmann-arm November 4, 2024 09:51

Harry-Ramsey requested changes Nov 4, 2024

View reviewed changes

all.sh: improve wrapper error reporting

dea700d

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Harry-Ramsey approved these changes Nov 4, 2024

View reviewed changes

ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Nov 4, 2024

ronald-cron-arm mentioned this pull request Nov 4, 2024

Add file checks Mbed-TLS/TF-PSA-Crypto#51

Open

2 tasks

This was referenced Nov 4, 2024

Add code style check Mbed-TLS/TF-PSA-Crypto#48

Open

Add generated file checks Mbed-TLS/TF-PSA-Crypto#50

Open

This was referenced Nov 5, 2024

[Backport 3.6] All.sh changes to support tf-psa-crypto components #9754

Merged

tf-psa-crypto all.sh: make the core manage the build directory #9755

Open

eleuzi01 self-requested a review November 6, 2024 10:47

eleuzi01 approved these changes Nov 6, 2024

View reviewed changes

gilles-peskine-arm previously requested changes Nov 6, 2024

View reviewed changes

gilles-peskine-arm added needs-work needs-backports Backports are missing or are pending review and approval. labels Nov 6, 2024

mpg removed needs-work needs-backports Backports are missing or are pending review and approval. labels Nov 6, 2024

mpg enabled auto-merge November 6, 2024 20:33

mpg disabled auto-merge November 6, 2024 21:04

mpg added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Nov 6, 2024

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Nov 7, 2024

mpg added this pull request to the merge queue Nov 7, 2024

Merged via the queue into Mbed-TLS:development with commit e248de5 Nov 7, 2024
6 checks passed

ronald-cron-arm mentioned this pull request Nov 7, 2024

TF-PSA-Crypto first all.sh component Mbed-TLS/mbedtls-framework#40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All.sh add support for tf-psa-crypto components #9720

All.sh add support for tf-psa-crypto components #9720

mpg commented Oct 24, 2024 •

edited

Loading

ronald-cron-arm left a comment

gilles-peskine-arm Oct 24, 2024

gilles-peskine-arm Oct 24, 2024

mpg Oct 25, 2024

gilles-peskine-arm Oct 25, 2024

mpg Oct 25, 2024

gilles-peskine-arm commented Oct 24, 2024

mpg commented Oct 25, 2024

mpg commented Oct 30, 2024

mpg commented Oct 30, 2024

mpg commented Oct 30, 2024

Harry-Ramsey Nov 4, 2024

mpg Nov 4, 2024

Harry-Ramsey left a comment

eleuzi01 left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Nov 6, 2024

ronald-cron-arm Nov 7, 2024

mpg Nov 7, 2024

gilles-peskine-arm Nov 7, 2024

All.sh add support for tf-psa-crypto components #9720

All.sh add support for tf-psa-crypto components #9720

Conversation

mpg commented Oct 24, 2024 • edited Loading

Description

PR checklist

Manual testing

ronald-cron-arm 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

gilles-peskine-arm commented Oct 24, 2024

mpg commented Oct 25, 2024

mpg commented Oct 30, 2024

mpg commented Oct 30, 2024

mpg commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harry-Ramsey left a comment

Choose a reason for hiding this comment

eleuzi01 left a comment

Choose a reason for hiding this comment

gilles-peskine-arm 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

mpg commented Oct 24, 2024 •

edited

Loading