Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Backport 3.6: Separate all.sh from its components #9444

Merged

Conversation

minosgalanakis
Copy link
Contributor

@minosgalanakis minosgalanakis commented Aug 1, 2024

Description

This is a backport of #8226

Notes to reviewers

I have recreated the original PR based on the latest reviewable/CI passing development PR with HEAD pointing at: 3d1bf4977f

The first chain of commit up until 883a3e9948 are straiftforward components that exist with the same name in dev/3.6 LTS.

I have manually compared the files, and marked the ones that have changes that need to be looked at.

git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-basic-checks.sh                 OK
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-build-system.sh                 OK
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-compiler.sh                     OK
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-compliance.sh                   OK
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-configuration-crypto.sh         diffs need to be reviewed
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-configuration-platform.sh       OK
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-configuration-tls.sh            diffs need to be reviewed
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-configuration-x509.sh           OK
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-configuration.sh                diffs need to be reviewed
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-platform.sh                     diffs need to be reviewed
git difftool 3d1bf4977f..883a3e9948 -- tests/scripts/components-sanitizers.sh                   OK

The remaining set of commits are components that did not match either because they have been removed or renamed. Please review them independantly.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

  • changelog provided | testing change only, not user facing.
  • development PR provided Separate all.sh from its components #8226
  • framework PR not required
  • 3.6 PR provided
  • 2.28 PR not required because: This is a backport
  • tests provided | not required because: changes to test framework itself

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
@minosgalanakis minosgalanakis added needs-work needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-very-high Highest priority - prioritise this over other review work labels Aug 1, 2024
@minosgalanakis minosgalanakis force-pushed the all-sh-separate-components_36bp branch from ddc4413 to d876d96 Compare August 1, 2024 22:26
@minosgalanakis minosgalanakis changed the title Backport: Separate all.sh from its componentsbp Backport: Separate all.sh from its components Aug 1, 2024
@minosgalanakis minosgalanakis added needs-review Every commit must be reviewed by at least two team members, component-test Test framework and CI scripts and removed needs-work size-s Estimated task size: small (~2d) labels Aug 1, 2024
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
@minosgalanakis minosgalanakis force-pushed the all-sh-separate-components_36bp branch from d876d96 to a4fecbe Compare August 2, 2024 01:09
@minosgalanakis minosgalanakis added needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Aug 2, 2024
@minosgalanakis minosgalanakis force-pushed the all-sh-separate-components_36bp branch from a4fecbe to 8068f17 Compare August 2, 2024 13:06
@minosgalanakis minosgalanakis added the needs-ci Needs to pass CI tests label Aug 2, 2024
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

I reviewed as follows:

The history has a couple of places where code is copied instead of moved in one commit, and the superfluous copy is removed later. That's not a blocker, but I'd prefer to rewrite the history anyway to avoid losing component_build_dhm_alt, so I'd prefer to also fix the moves to be actually moves. I'd also prefer not to lump non-moves with moves in the same commit, such as the spelling correction on component_test_tls1_2_deafult_cbc_legacy_cipher_only_use_psa.

tests/scripts/components.sh Outdated Show resolved Hide resolved
tests/scripts/components.sh Outdated Show resolved Hide resolved
tests/scripts/components-build-system.sh Outdated Show resolved Hide resolved
@@ -0,0 +1,162 @@
# components-basic-checks.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

[INFORMATIVE] I compared the component assignments here and in #8226 as of bf47cf7 and #9444 as of 8068f17.

comm -31 <(git grep -E -o '^component_\w+' upstream-public/pr/8226 -- 'tests/scripts/components*.sh' | sed 's!.*/\(.*\):\(.*\)!\2 \1!' | sort) <(git grep -E -o '^component_\w+' upstream-public/pr/9444 -- 'tests/scripts/components*.sh' | sed 's!.*/\(.*\):\(.*\)!\2 \1!' | sort)

No component present in both has moved to a different file.

The 3.6-only components are:

component_build_aes_via_padlock components-build-system.sh
component_build_full_psa_crypto_client_without_crypto_provider components-configuration-crypto.sh
component_build_psa_accel_alg_ecdh components-configuration-crypto.sh
component_build_psa_accel_alg_hkdf components-configuration-crypto.sh
component_build_psa_accel_alg_hmac components-configuration-crypto.sh
component_build_psa_accel_alg_md5 components-configuration-crypto.sh
component_build_psa_accel_alg_ripemd160 components-configuration-crypto.sh
component_build_psa_accel_alg_rsa_oaep components-configuration-crypto.sh
component_build_psa_accel_alg_rsa_pkcs1v15_crypt components-configuration-crypto.sh
component_build_psa_accel_alg_rsa_pkcs1v15_sign components-configuration-crypto.sh
component_build_psa_accel_alg_rsa_pss components-configuration-crypto.sh
component_build_psa_accel_alg_sha1 components-configuration-crypto.sh
component_build_psa_accel_alg_sha224 components-configuration-crypto.sh
component_build_psa_accel_alg_sha256 components-configuration-crypto.sh
component_build_psa_accel_alg_sha384 components-configuration-crypto.sh
component_build_psa_accel_alg_sha512 components-configuration-crypto.sh
component_build_psa_accel_key_type_rsa_key_pair components-configuration-crypto.sh
component_build_psa_accel_key_type_rsa_public_key components-configuration-crypto.sh
component_test_default_psa_crypto_client_without_crypto_provider components-configuration-crypto.sh
component_test_full_no_bignum components-configuration-crypto.sh
component_test_full_no_cipher_no_psa_crypto components-configuration-crypto.sh
component_test_full_no_cipher_with_psa_crypto components-configuration-crypto.sh
component_test_full_no_cipher_with_psa_crypto_config components-configuration-crypto.sh
component_test_psa_crypto_config_accel_hash_keep_builtins components-configuration-crypto.sh
component_test_psa_crypto_rsa_no_genprime components-configuration-crypto.sh
component_test_tls1_2_default_cbc_legacy_cbc_etm_cipher_only_use_psa components-configuration-tls.sh
component_test_tls1_2_default_cbc_legacy_cipher_only_use_psa components-configuration-tls.sh
component_test_tls1_2_default_stream_cipher_only_use_psa components-configuration-tls.sh

I agree with these assignments except component_build_aes_via_padlock as noted in another comment.

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Aug 2, 2024
@mpg
Copy link
Contributor

mpg commented Aug 5, 2024

Since Gilles would like the history rewritten, I'll wait until that's done (or you've declared that you won't do it) before reviewing.

Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM at bd6b98f

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, size-s Estimated task size: small (~2d) and removed needs-ci Needs to pass CI tests labels Aug 5, 2024
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Aug 5, 2024
6 tasks
@gilles-peskine-arm gilles-peskine-arm changed the title Backport: Separate all.sh from its components Backport 3.6: Separate all.sh from its components Aug 5, 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.

LGTM, thanks!

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 6, 2024
@mpg mpg added this pull request to the merge queue Aug 6, 2024
Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 095cf69 Aug 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-test Test framework and CI scripts priority-very-high Highest priority - prioritise this over other review work size-s Estimated task size: small (~2d)
Development

Successfully merging this pull request may close these issues.

3 participants