mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

Separate `all.sh` from its components

Open davidhorstmann-arm opened this issue 1 year ago • 3 comments

Separate the components of all.sh from the main script. This makes it more convenient to change the components used.

Fixes Mbed-TLS/psa-crypto#31

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • [x] changelog not required - testing change only, not user facing
  • [ ] backport done, or not required
  • [x] tests not required - changes to test framework itself

davidhorstmann-arm avatar Sep 19 '23 17:09 davidhorstmann-arm

Internal CI failures appear spurious.

tom-daubney-arm avatar Apr 19 '24 10:04 tom-daubney-arm

I would like to request a high-level design review from @ronald-cron-arm and @gilles-peskine-arm please. The PR is not in it's final state but I would like to know if what I have done thus far looks like I am on the right track or not. I am aware there are conflicts but I will sort these out later. The git history is also a little messy which I will also take care of before this gets reviewed in full.

So far I have split things out inline with what I think has been requested. No doubt I have categorised some components incorrectly but hopefully things are broadly correct.

I plan to do the following things next:

  • Add more documentation to the components-xxx.sh files inline with what is written in components.sh.
  • Delete components.sh
  • Split the new components-xxx.sh files up to three ways such that we potentially will have components_xxx_psa.sh, components_xxx_mbedtls.sh and then keep the current components_xxx.sh file where there are components that are common to both repos.
  • General tidy up etc before marking ready for review.

I would welcome any comments on what I have done and what I plan to do please.

tom-daubney-arm avatar Apr 23 '24 14:04 tom-daubney-arm

I think this is now in a state where it is worthy of review, so am marking it as such.

tom-daubney-arm avatar May 14 '24 10:05 tom-daubney-arm

Fully recreated the commit from fresh base, replaying the choices made in the previous for-review revision https://github.com/Mbed-TLS/mbedtls/commit/2a2c4cbd5d6553f1e33d98cb6d1598a8a08b3024

Notes to reviewers

  • I have structured the commits per components, making each commit easy to review using git diff --color-moved=dimmed-zebra
  • The commits up to https://github.com/Mbed-TLS/mbedtls/pull/8226/commits/781c823255a7bd09e0812addbad5c57029f21c14 are replicating the split of the previous version.
  • Commits after that are moving newly added components have been address based on estimate. If the choice is wrong I can easily amend the specific commit.
  • component_test_memory_buffer_allocator_backtrace in https://github.com/Mbed-TLS/mbedtls/commit/2a2c4cbd5d6553f1e33d98cb6d1598a8a08b3024 was placed in two files in components-configuration-platform and components-configuration-tls. I assumed that was accidental and did not replicate it.
  • I have added @davidhorstmann-arm and @tom-daubney-arm as having signed-off in the comit range since that was meant to replicate the original PR in an easy to review chain. Resolutions of newly introduced components are single sign-off.
  • (Optional) The python regex to track the components is r'(?m)^\S+ ?\(\) {', data)

I have finally compared the components as sets between the two revisions and printed the symmetric difference bellow:

{'components-build-system.sh': ['support_build_cmake_programs_no_testing',                                    # Newly-Added
                                'component_build_cmake_programs_no_testing'],                                 # Newly-Added
 'components-configuration-crypto.sh': ['component_test_no_rsa_key_pair_generation',                          # Newly-Added
                                        'component_build_dhm_alt',                                            # Removed
                                        'component_test_full_no_cipher_no_psa_crypto',                        # Removed
                                        'common_check_mbedtls_missing_symbols',                               # Removed
                                        'component_build_aes_via_padlock',                                    # Removed
                                        'common_test_full_no_cipher_with_psa_crypto',                         # Removed
                                        'support_build_aes_via_padlock_only',                                 # Removed
                                        'component_test_full_no_cipher_with_psa_crypto',                      # Removed
                                        'component_test_full_no_bignum',                                      # Removed
                                        'component_test_psa_crypto_rsa_no_genprime',                          # Removed
                                        'component_build_full_psa_crypto_client_without_crypto_provider',     # Removed
                                        'component_test_default_psa_crypto_client_without_crypto_provider',   # Removed
                                        'component_test_full_no_cipher_with_psa_crypto_config',               # Removed
                                        'component_test_full_no_cipher'],                                     # Newly-Added
 'components-configuration-platform.sh': ['component_test_memory_buffer_allocator_backtrace'],                # Duplicated
 'components-configuration-tls.sh': ['component_test_memory_buffer_allocator_backtrace',                      # Duplicated
                                     'component_test_tls1_2_default_cbc_legacy_cbc_etm_cipher_only_use_psa',  # Removed
                                     'component_test_tls1_2_deafult_cbc_legacy_cipher_only_use_psa',          # Removed
                                     'component_test_tls1_2_default_stream_cipher_only_use_psa'],             # Removed
 'components-psasim.sh': ['helper_psasim_server',                                                             # Newly-Added
                          'component_test_psasim',                                                            # Newly-Added
                          'component_test_suite_with_psasim']}                                                # Newly-Added

Everything that is marked as Removed has been tested against git history to no longer exit in latest development. Newly-Added files have been placed in estimated componenets. The duplication has not been reproduced.

minosgalanakis avatar Jul 27 '24 13:07 minosgalanakis

As discussed, setting priority-very-high because of the potential for very painful conflicts in all.sh.

gilles-peskine-arm avatar Jul 29 '24 15:07 gilles-peskine-arm

Looks good to me at bf47cf7, but please remove 3d1bf49 where I realize I made a mistake (sorry about that).

Thats ok, that is precicely they were placed in separate commits. I have removed https://github.com/Mbed-TLS/mbedtls/commit/3d1bf4977fdb184a0df4c9a98e805b5930269c57 in both here and the backport.

minosgalanakis avatar Aug 02 '24 13:08 minosgalanakis