Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.57k stars 2.61k forks source link

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

Open mpg opened 4 weeks ago

mpg commented 4 weeks ago

Depends on: https://github.com/Mbed-TLS/mbedtls/pull/9720

As of 9720, component in tf-psa-crypto that use out-of-tree CMake build have to manage the build directory themselves:

    TF_PSA_CRYPTO_ROOT_DIR="$PWD"
    mkdir "$OUT_OF_SOURCE_DIR"
    cd "$OUT_OF_SOURCE_DIR"
    # (build and run tests)
    cd "$TF_PSA_CRYPTO_ROOT_DIR"
    rm -rf "$OUT_OF_SOURCE_DIR"

Since most components in tf-psa-crypto are expected to build out-of-tree (actually, all of them except one to check that in-tree builds work), we want to move the above boilerplate to all-core.sh so that the component definition can only include the "build and run tests" part.

Harry-Ramsey commented 3 weeks ago

Sorry, just want to make sure my understanding is correct. Currently it seems that only support_test_cmake_out_of_source for Mbed TLS and component_test_cmake_tf_psa_crypto_out_of_source for tf-psa-crypto. There are other tests ran using cmake.

Is the goal to consolidate all cmake tests to build out of source and have those which do not build in source such as make?

gilles-peskine-arm commented 3 weeks ago

support_test_cmake_out_of_source is there because older versions of CMake can't do our out-of-source builds. Specifically, we do our testing on Ubuntu 16.04 by default, and we would need at least Ubuntu 18.04. (We have images for Ubuntu 16.04, 18.04, 20.04 and 22.04. There's a draft 24.04 docker file in https://github.com/gilles-peskine-arm/mbedtls-test/tree/ubuntu-24.04-dockerfile but I don't remember if I've tested it.)

We could switch our default image to a more modern Ubuntu. In fact we should do that, it's long overdue, but it's not straightforward: some of our tests fail with newer compilers (new warnings, new minor problems reported by UBSan), check-python-files fails with recent Python, maybe more problems. At least 22.04 was a whole epic's worth of work (I didn't investigate fully). 18.04 might be easy to do: I think its Python and compilers are old enough not to cause problems. The only components that are tied to Ubuntu 16.04 are component_test_{gcc,clang}_earliest_opt, which should keep running with old compilers by design. The interface stability check job (scriptsabi_check.py) is known to break on Ubuntu 20.04, but it's now running on 18.04.

The Groovy code has a priority order for which Ubuntu vintage to use for each component, among the ones for which the support_xxx function succeeds. (If the function is not present, it's assumed to always succeed.) This priority order is the same for all supported branches. If we want to change the priority order only for development, we have to make the Groovy code recognize whether it's running a different branch. We've done that in the past, e.g. to recognize branches that needed C99 vs branches where we wanted to test compatibility with pre-C99 compilers. It may be easier to switch everything to using Ubuntu 18.04 by default. That would be acceptable in terms of test coverage. The only reason not to switch all the branches is if it turns out to require significant effort in the older LTS branches.

P.S. It's also possible that we've inadvertently fixed whatever problem our CMake scripts caused with older CMake.

mpg commented 3 weeks ago

Sorry, just want to make sure my understanding is correct. Currently it seems that only support_test_cmake_out_of_source for Mbed TLS and component_test_cmake_tf_psa_crypto_out_of_source for tf-psa-crypto. There are other tests ran using cmake.

Is the goal to consolidate all cmake tests to build out of source and have those which do not build in source such as make?

My bad, I didn't provide the whole context. There's currently only one component for tf-psa-crypto but a lot of components are going to be migrated from mbedtls to tf-psa-crypto in the near future. While doing so, we plan to:

  1. migrate to CMake the components that currently use Make;
  2. while at it, build out of source. Part (1) is mandatory because CMake is the only build system in tf-psa-crypto; part (2) is just the recommended good practice with CMake.

Currently components that use CMake out-of-source manually create and remove the build directory; this makes sense since there are few of them. In tf-psa-crypto, since almost all components are going to use CMake out-of-source, it makes little sense for each component to have to copy-paste the boilerplate code for that, and it would make more sense for the core to create the build directory and cd to it right before invoking the component function, and then remove the build directory afterwards.

This issue is about that (and just that): making the core manage that build directory when in tf-psa-crypto. No change should be made to what happens in mbedtls. This task is not meant to add or remove components, but simplify the definition of component_test_cmake_tf_psa_crypto_out_of_source to look like:

component_test_cmake_tf_psa_crypto_out_of_source () {
    msg "build: cmake tf-psa-crypto 'out-of-source' build"
    # 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"
    make
    msg "test: cmake tf-psa-crypto 'out-of-source' build"
    make test
}

(boilerplate removed and handled by the core).

I hope it's clearer now. Please tell if anything's not clear yet.

mpg commented 3 weeks ago

@gilles-peskine-arm I wasn't aware that the version of cmake on Ubuntu 16.04 had issues with out-of-source builds, thanks for sharing the info. As Ronald pointed out, the tf-psa-crypto component has existed for a bit more than a month now and we haven't observed failures, which is encouraging. Perhaps the issue does not manifest with the tf-psa-crypto CMake files.

If it turns out it does (or if we want to be careful), we still have a variety of practical options (by practical, I mean, that don't have CI modernization as a pre-requisite). But I'd rather discuss them somewhere else, because I think this is quite orthogonal to this issue. (It is related to the context that motivates this issue, but not to this issue itself IMO.)

gilles-peskine-arm commented 3 weeks ago

Checking back, the issue where we documented the problem was https://github.com/Mbed-TLS/mbedtls/issues/5223.

Since we didn't know the exact cause, it's quite possible that we've fixed it. If the problem doesn't come back, it's all good.

Actually, maybe https://github.com/Mbed-TLS/mbedtls/pull/5429 fixed it? We didn't make the link at the time, but the symptoms of https://github.com/Mbed-TLS/mbedtls/issues/5374#issuecomment-1003783958 and https://github.com/Mbed-TLS/mbedtls/issues/5223 look similar.