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

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

Open
mpg opened this issue Nov 5, 2024 · 5 comments · May be fixed by #9767 or Mbed-TLS/TF-PSA-Crypto#121
Open

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

mpg opened this issue Nov 5, 2024 · 5 comments · May be fixed by #9767 or Mbed-TLS/TF-PSA-Crypto#121
Assignees
Labels
enhancement priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Comments

@mpg
Copy link
Contributor

mpg commented Nov 5, 2024

Depends on: #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.

@mpg mpg added enhancement size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Nov 5, 2024
@mpg mpg moved this to TF-PSA-Crypto all.sh components in Mbed TLS Epics Nov 5, 2024
@Harry-Ramsey Harry-Ramsey self-assigned this Nov 5, 2024
@Harry-Ramsey
Copy link
Contributor

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
Copy link
Contributor

gilles-peskine-arm commented Nov 6, 2024

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
Copy link
Contributor Author

mpg commented Nov 7, 2024

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
Copy link
Contributor Author

mpg commented Nov 7, 2024

@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
Copy link
Contributor

Checking back, the issue where we documented the problem was #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 #5429 fixed it? We didn't make the link at the time, but the symptoms of #5374 (comment) and #5223 look similar.

@Harry-Ramsey Harry-Ramsey linked a pull request Nov 11, 2024 that will close this issue
5 tasks
@Harry-Ramsey Harry-Ramsey linked a pull request Dec 17, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Status: TF-PSA-Crypto all.sh components
3 participants