Add test job for crypto repo #123

bensze01 · 2023-09-20T17:10:47Z

Changes and fixes needed to add test jobs for the crypto repo

Test Runs:

Internal CI
- Runs with a psa crypto version override enabled
  - Mbed-TLS/mbedtls-restricted@a23e11c7b14458a5e17755c545cdac681b20260d + Mbed-TLS/TF-PSA-Crypto@4fd370f
    - mbed-tls-pr-test-parametrized
    - mbedtls-release-ci-testing
- Runs with a psa crypto and framework version overrides enabled
- Mbed-TLS/mbedtls-restricted@a23e11c7b14458a5e17755c545cdac681b20260d + Mbed-TLS/TF-PSA-Crypto@4fd370f + Mbed-TLS/mbedtls-framework@f1119c2
  - mbed-tls-pr-test-parametrized
  - mbedtls-release-ci-testing
- Runs without a submodule version override
  - Mbed-TLS/mbedtls-restricted@a23e11c7b14458a5e17755c545cdac681b20260d
    - mbed-tls-pr-test-parametrized
    - mbedtls-release-ci-testing
  - development
    - PR-head
    - PR-merge
  - mbedtls-3.6
    - PR-head
    - PR-merge
  - mbedtls-2.28
    - PR-head
    - PR-merge
OpenCI
- Runs with a psa crypto version override enabled
  - Mbed-TLS/mbedtls-restricted@a23e11c7b14458a5e17755c545cdac681b20260d + Mbed-TLS/TF-PSA-Crypto@4fd370f
    - mbed-tls-restricted-pr-test-parametrized
    - mbedtls-release-ci-testing
- Runs with a psa crypto and framework version overrides enabled
- Mbed-TLS/mbedtls-restricted@a23e11c7b14458a5e17755c545cdac681b20260d + Mbed-TLS/TF-PSA-Crypto@4fd370f + Mbed-TLS/mbedtls-framework@f1119c2
  - mbed-tls-restricted-pr-test-parametrized
  - mbedtls-release-ci-testing
- Runs without a submodule version override
  - Mbed-TLS/mbedtls-restricted@a23e11c7b14458a5e17755c545cdac681b20260d
    - mbed-tls-restricted-pr-test-parametrized
    - mbedtls-release-ci-testing
  - development
    - PR-head
    - PR-merge
  - mbedtls-3.6
    - PR-head
    - PR-merge
  - mbedtls-2.28
    - PR-head
    - PR-merge

ronald-cron-arm

This looks sensible to me. A few suggestions and one question.

vars/environ.groovy

vars/psa_crypto.groovy

ronald-cron-arm

Just one question, otherwise this looks good to me.

vars/psa_crypto.groovy

ronald-cron-arm

LGTM, thanks.

davidhorstmann-arm

LGTM

gilles-peskine-arm · 2023-09-25T13:12:45Z

vars/psa_crypto.groovy

+
+void run_pr_job() {
+    if (env.TARGET_BRANCH != 'main') {
+        echo 'PR target is not "main" branch - not building.'


Why would we refuse to build pull requests targeting another branch? We don't do it often, but we do do it sometimes, and I don't see any reason to forbid it.

Only main contains the files copied from Mbed-TLS, including all.sh. If we want to test PRs targeting development, a bit more logic is needed - but that was out of scope for the original issue.

main is the only branch that contains all.sh. The other branches are just templates that don't have everything imported from the mbedtls repo. Once we move crypto development to this repo, this condition should go away.

We do not want to build the 'development' branch (only the build system, doc and tree skeleton) and we have only two maintained branches currently thus this test does the job considering those two branches. Taking into consideration other working branches based on main, if (env.TARGET_BRANCH == 'development') is probably better though.

@ronald-cron-arm Yes, the main reason for the check is to prevent jenkins from reporting spurious errors to github.

@ronald-cron-arm Yes, the reason for the check is to prevent jenkins from reporting spurious errors to github.

okay, thanks. Then, what about if (env.TARGET_BRANCH == 'development') instead to allow the manual CI testing of "main like" branches.

Sure, that works for me as well.

This sounds like a job for fileExists(…) (plus perhaps some content check) rather than checking the target branch name.

In general probably but currently the development branch in psa-crypto is quite peculiar thus to me it is acceptable for it to be handled in a special manner for CI purposes.

gilles-peskine-arm · 2023-09-25T13:14:04Z

vars/environ.groovy

@@ -28,26 +28,23 @@ def set_common_environment() {
    env.MAKEFLAGS = '-j2'
 }

-def set_tls_pr_environment(is_production) {
+void set_pr_environment(boolean is_production, String repo='tls') {


Can we make repo some kind of enum?

gilles-peskine-arm · 2023-10-04T09:35:44Z

There's a merge conflict now.

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

mpg · 2024-12-04T09:46:05Z

@bensze01 Forgot to ask yesterday, regarding the labels: does "needs CI" mean that you want to run more test jobs, or that the test jobs so far revealed issues that need fixing? (From the absence of "needs work" I'm assuming it's the former, I'm just checking to be sure.)

This fixes issues with building commits that are not fetched by the default refspec of +refs/heads/*:refs/remotes/origin/*, eg. if the specified commit is not in the history of any extant branch, or when using shallow clones. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>

bensze01 · 2024-12-04T16:34:30Z

@mpg The former. I pushed a small drive-by fix for an issue that was hindering my testing, but aside from this no further rework should be required.

mpg

Code looks good to me. I'm just waiting for all the test runs before I formally approve.

minosgalanakis

LGTM

bensze01 · 2024-12-05T23:33:05Z

I've added all of the tests I was planning to run to the PR description.

bensze01 · 2024-12-06T09:01:05Z

@mpg All the CI runs passed.

mpg

I had a look at the tests runs, and it all looks good to me. Thanks!

bensze01 requested review from ronald-cron-arm and davidhorstmann-arm September 20, 2023 17:10

bensze01 mentioned this pull request Sep 20, 2023

Add CI support Mbed-TLS/TF-PSA-Crypto#46

Merged

ronald-cron-arm requested changes Sep 21, 2023

View reviewed changes

vars/environ.groovy Outdated Show resolved Hide resolved

vars/environ.groovy Outdated Show resolved Hide resolved

vars/psa_crypto.groovy Outdated Show resolved Hide resolved

bensze01 force-pushed the dev/bensze01/psa-crypto branch from 20d0924 to 0191782 Compare September 22, 2023 14:35

bensze01 requested a review from ronald-cron-arm September 22, 2023 14:42

ronald-cron-arm requested changes Sep 25, 2023

View reviewed changes

vars/psa_crypto.groovy Outdated Show resolved Hide resolved

ronald-cron-arm previously approved these changes Sep 25, 2023

View reviewed changes

davidhorstmann-arm previously approved these changes Sep 25, 2023

View reviewed changes

davidhorstmann-arm added the approved Approved in review. May need additional CI. label Sep 25, 2023

bensze01 added the needs: ci label Sep 25, 2023

gilles-peskine-arm reviewed Sep 25, 2023

View reviewed changes

ronald-cron-arm linked an issue Sep 27, 2023 that may be closed by this pull request

Add CI support Mbed-TLS/TF-PSA-Crypto#6

Closed

gilles-peskine-arm added needs: work and removed approved Approved in review. May need additional CI. labels Oct 4, 2023

bensze01 dismissed stale reviews from davidhorstmann-arm and ronald-cron-arm via c8f5e7e October 12, 2023 14:04

bensze01 force-pushed the dev/bensze01/psa-crypto branch from 0191782 to c8f5e7e Compare October 12, 2023 14:04

bensze01 closed this Nov 25, 2024

bensze01 force-pushed the dev/bensze01/psa-crypto branch from a2fdd0f to 4bbc4f6 Compare November 25, 2024 17:20

bensze01 reopened this Nov 25, 2024

bensze01 force-pushed the dev/bensze01/psa-crypto branch from 4834a2d to f751d16 Compare November 25, 2024 17:44

Add tf-psa-crypto CI scripts

42d198f

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

bensze01 force-pushed the dev/bensze01/psa-crypto branch from f751d16 to 42d198f Compare November 27, 2024 16:09

bensze01 added needs: review priority-high and removed needs: work labels Dec 2, 2024

mpg reviewed Dec 5, 2024

View reviewed changes

minosgalanakis approved these changes Dec 5, 2024

View reviewed changes

bensze01 added the enhancement New feature or request label Dec 5, 2024

bensze01 removed the needs: ci label Dec 6, 2024

mpg approved these changes Dec 6, 2024

View reviewed changes

mpg merged commit d736fff into main Dec 6, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test job for crypto repo #123

Add test job for crypto repo #123

bensze01 commented Sep 20, 2023 •

edited

Loading

ronald-cron-arm left a comment

ronald-cron-arm left a comment

ronald-cron-arm left a comment

davidhorstmann-arm left a comment

gilles-peskine-arm Sep 25, 2023

bensze01 Sep 25, 2023 •

edited

Loading

davidhorstmann-arm Sep 25, 2023

davidhorstmann-arm Sep 25, 2023

ronald-cron-arm Sep 25, 2023 •

edited

Loading

bensze01 Sep 25, 2023 •

edited

Loading

ronald-cron-arm Sep 25, 2023

bensze01 Sep 25, 2023

gilles-peskine-arm Sep 25, 2023 •

edited

Loading

ronald-cron-arm Sep 25, 2023

gilles-peskine-arm Sep 25, 2023

gilles-peskine-arm commented Oct 4, 2023

mpg commented Dec 4, 2024

bensze01 commented Dec 4, 2024 •

edited

Loading

mpg left a comment

minosgalanakis left a comment

bensze01 commented Dec 5, 2024

bensze01 commented Dec 6, 2024

mpg left a comment

Add test job for crypto repo #123

Add test job for crypto repo #123

Conversation

bensze01 commented Sep 20, 2023 • edited Loading

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Sep 25, 2023

Choose a reason for hiding this comment

bensze01 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

davidhorstmann-arm Sep 25, 2023

Choose a reason for hiding this comment

davidhorstmann-arm Sep 25, 2023

Choose a reason for hiding this comment

ronald-cron-arm Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

bensze01 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm Sep 25, 2023

Choose a reason for hiding this comment

bensze01 Sep 25, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm Sep 25, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Sep 25, 2023

Choose a reason for hiding this comment

gilles-peskine-arm commented Oct 4, 2023

mpg commented Dec 4, 2024

bensze01 commented Dec 4, 2024 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

minosgalanakis left a comment

Choose a reason for hiding this comment

bensze01 commented Dec 5, 2024

bensze01 commented Dec 6, 2024

mpg left a comment

Choose a reason for hiding this comment

bensze01 commented Sep 20, 2023 •

edited

Loading

bensze01 Sep 25, 2023 •

edited

Loading

ronald-cron-arm Sep 25, 2023 •

edited

Loading

bensze01 Sep 25, 2023 •

edited

Loading

gilles-peskine-arm Sep 25, 2023 •

edited

Loading

bensze01 commented Dec 4, 2024 •

edited

Loading