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

All.sh add support for tf-psa-crypto components #9720

Merged
merged 8 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions tests/scripts/all-core.sh
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,12 @@
# means that components can assume that the working directory is in a
# cleaned-up state, and don't need to perform the cleanup themselves.
# * Run `make clean`.
# * Restore `include/mbedtls/mbedtls_config.h` from a backup made before running
# the component.
# * Check out `Makefile`, `library/Makefile`, `programs/Makefile`,
# `tests/Makefile` and `programs/fuzz/Makefile` from git.
# This cleans up after an in-tree use of CMake.
# * Restore the various config files (potentially modified by config.py) from
# a backup made when starting the script.
# * If in Mbed TLS, restore the various `Makefile`s (potentially modified by
# in-tree use of CMake) from a backup made when starting the script. (Note:
# if the files look generated when starting the script, they will be
# restored from the git index before making the backup.)


################################################################
Expand Down Expand Up @@ -156,15 +157,16 @@ pre_check_environment () {
# Must be called before pre_initialize_variables which sets ALL_COMPONENTS.
pre_load_components () {
# Include the components from components.sh
test_script_dir="${0%/*}"
for file in "$test_script_dir"/components-*.sh; do
# Use a path relative to the current directory, aka project's root.
for file in tests/scripts/components-*.sh; do
source $file
done
}

pre_initialize_variables () {
if in_mbedtls_repo; then
CONFIG_H='include/mbedtls/mbedtls_config.h'
CONFIG_TEST_DRIVER_H='tests/include/test/drivers/config_test_driver.h'
if [ -d tf-psa-crypto ]; then
CRYPTO_CONFIG_H='tf-psa-crypto/include/psa/crypto_config.h'
PSA_CORE_PATH='tf-psa-crypto/core'
Expand All @@ -176,20 +178,21 @@ pre_initialize_variables () {
PSA_CORE_PATH=''
BUILTIN_SRC_PATH=''
fi
config_files="$CONFIG_H $CRYPTO_CONFIG_H $CONFIG_TEST_DRIVER_H"
else
CONFIG_H='drivers/builtin/include/mbedtls/mbedtls_config.h'
CRYPTO_CONFIG_H='include/psa/crypto_config.h'
PSA_CORE_PATH='core'
BUILTIN_SRC_PATH='drivers/builtin/src'

config_files="$CRYPTO_CONFIG_H"
fi
CONFIG_TEST_DRIVER_H='tests/include/test/drivers/config_test_driver.h'

# Files that are clobbered by some jobs will be backed up. Use a different
# suffix from auxiliary scripts so that all.sh and auxiliary scripts can
# independently decide when to remove the backup file.
backup_suffix='.all.bak'
# Files clobbered by config.py
files_to_back_up="$CONFIG_H $CRYPTO_CONFIG_H $CONFIG_TEST_DRIVER_H"
files_to_back_up="$config_files"
if in_mbedtls_repo; then
# Files clobbered by in-tree cmake
files_to_back_up="$files_to_back_up Makefile library/Makefile programs/Makefile tests/Makefile programs/fuzz/Makefile"
Expand Down Expand Up @@ -616,7 +619,7 @@ pre_parse_command_line () {
pre_check_git () {
if [ $FORCE -eq 1 ]; then
rm -rf "$OUT_OF_SOURCE_DIR"
git checkout-index -f -q $CONFIG_H
git checkout-index -f -q $config_files
cleanup
else

Expand All @@ -627,12 +630,14 @@ pre_check_git () {
exit 1
fi

if ! git diff --quiet "$CONFIG_H"; then
err_msg "Warning - the configuration file '$CONFIG_H' has been edited. "
echo "You can either delete or preserve your work, or force the test by rerunning the"
echo "script as: $0 --force"
exit 1
fi
for config in $config_files; do
if ! git diff --quiet "$config"; then
err_msg "Warning - the configuration file '$config' has been edited. "
echo "You can either delete or preserve your work, or force the test by rerunning the"
echo "script as: $0 --force"
exit 1
fi
done
fi
}

Expand Down Expand Up @@ -864,7 +869,8 @@ pre_check_tools () {
set "$@" ARMC5_CC="$ARMC5_CC" ARMC6_CC="$ARMC6_CC" RUN_ARMCC=1;;
*) set "$@" RUN_ARMCC=0;;
esac
"$@" scripts/output_env.sh
# Use a path relative to the currently-sourced file.
"$@" "${BASH_SOURCE%/*}"/../../scripts/output_env.sh
}

pre_generate_files() {
Expand All @@ -879,8 +885,8 @@ pre_generate_files() {
}

pre_load_helpers () {
# The path is going to change when this is moved to the framework
test_script_dir="${0%/*}"
# Use a path relative to the currently-sourced file.
test_script_dir="${BASH_SOURCE%/*}"
source "$test_script_dir"/all-helpers.sh
}

Expand Down
111 changes: 104 additions & 7 deletions tests/scripts/all.sh
Original file line number Diff line number Diff line change
@@ -1,15 +1,112 @@
#! /usr/bin/env bash

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this conflicts with #9293, which I hope we can merge soon since all it's missing is one approval on one backport.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also conflicts with #9286, but that's not quite ready yet, so I suggest that we go 9293, then 9720, then 9286.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm not seeing the conflict with 9293 - it seems to me that the two PRs are not touching the same files. What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, there's no merge conflict with 9293 and I don't think there's a semantic conflict either. There's a potential semantic conflict with #9286 since it will error out if a component name is duplicated between mbedtls and tf-psa-crypto.

I ran a merge of something that conflicted but that must have been some different branches or with a non-up-to-date local copy and I can't find exactly what I ran yesterday anymore. I have a conflict checker script and it reports no conflict between the current state of this PR and any other PR opened in the last 6 months. #9293 only conflicts in .gitignore with a priority-medium PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a potential semantic conflict with #9286 since it will error out if a component name is duplicated between mbedtls and tf-psa-crypto.

Well, currently my PR also forbids component names that are duplicated between mbedtls and tf-psa-crypto, so that shouldn't be a problem. As I said earlier, I don't think we should allow components with the same name on both sides until the sides are completely separated.

# all.sh
# all.sh (transitional wrapper)
#
# Copyright The Mbed TLS Contributors
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later

# This file is executable; it is the entry point for users and the CI.
# See "Files structure" in all-core.sh for other files used.
# This is a transitional wrapper that's only meant for the CI.
# Developers should directly invoke on or two of:
# - tests/scripts/mbedtls-all.sh ...
# - (cd tf-psa-crypto && tests/scripts/all.sh ...)
#
# During the transition, it's illegal for a tf-psa-crypto component to have
# the same name as an mbedtls components; since this wrapper handles both
# sides at once, component names need to be globally unique. Once the
# transition period is over, unicity on each side will be enough.
#
# For context, here are the steps of the transition:
# 1. We have an all.sh in tf-psa-crypto but for now we don't invoke it directly
# on the CI, only through this transitional wrapper in mbedtls. (tf-psa-crypto
# doesn't have its own CI initially and runs Mbed TLS's instead.)
# 2. We move all relevant components to tf-psa-crypto so that it gets the level of
# coverage we want. We need to make sure the new names are unique.
# 3. We change the CI job on tf-psa-crypto to stop checking out mbedtls and running
# its all.sh - instead we do the normal thing of checking out tf-psa-crypto and
# running its all.sh. (In two steps: (a) add the new job, (b) remove the old
# one.)
# 4. We remove the transitional wrapper in mbedtls and we're now free to rename
# tf-psa-crypto components as we want. If we followed a consistent naming
# pattern, this can be as simple as s/_tf_psa_crypto// in components-*.sh.

# This script must be invoked from the project's root.

# There are exactly 4 ways this is invoked in the CI:
# 1. tests/scripts/all.sh --help
# 2. tests/scripts/all.sh --list-all-components
# 3. tests/scripts/all.sh --list-components
# 4. tests/scripts/all.sh --seed 4 --keep-going single_component_name
# This wrapper does not support other invocations.

set -eu

# Cases 1-3
if [ "$#" -eq 1 ]; then
if [ "$1" = '--help' ]; then
# It doesn't matter which one we use, they're the same
tests/scripts/mbedtls-all.sh "$1"
exit 0
fi
if [ "$1" = '--list-all-components' -o "$1" = '--list-components' ]; then
# Invoke both
tests/scripts/mbedtls-all.sh "$1"
(cd tf-psa-crypto && tests/scripts/all.sh "$1")
exit 0
fi
fi

if [ "$#" -ne 4 -o "$1" != '--seed' -o "$3" != '--keep-going' ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

If I attempt to run the script using the following command ./tests/scripts/all.sh test_cmake_tf_psa_crypto_out_of_source I encounter the following error:

./tests/scripts/all.sh: line 58: $3: unbound variable

I believe this is incorrect and I should either be greeted with the error or the command executes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, good point, that's poor error handling. I'll fix it.

echo "This invocation is not supported by the transitional wrapper." >&2
echo "See the comments at the top of $0." >&2
exit 1
fi

# Case 4: invoke the right all.sh for this component
comp_name=$4

# Get the list of components available on each side.
COMP_MBEDTLS=$(tests/scripts/mbedtls-all.sh --list-all-components | tr '\n' ' ')
COMP_CRYPTO=$(cd tf-psa-crypto && tests/scripts/all.sh --list-all-components | tr '\n' ' ')

# tell if $1 is in space-separated list $2
is_in() {
needle=$1
haystack=$2
case " $haystack " in
*" $needle "*) echo 1;;
*) echo 0;;
esac
}

is_crypto=$(is_in "$comp_name" "$COMP_CRYPTO")
is_mbedtls=$(is_in "$comp_name" "$COMP_MBEDTLS")

# Component should be on exactly one side (see comment near the top).
if [ "$is_crypto" -eq 1 -a "$is_mbedtls" -eq 1 ]; then
echo "Component '$comp_name' is both in crypto and Mbed TLS". >&2
echo "See the comments at the top of $0." >&2
exit 1
fi
if [ "$is_crypto" -eq 0 -a "$is_mbedtls" -eq 0 ]; then
echo "Component '$comp_name' is neither in crypto nor in Mbed TLS". >&2
echo "See the comments at the top of $0." >&2
exit 1
fi

# The path is going to change when this is moved to the framework
test_script_dir="${0%/*}"
source "$test_script_dir"/all-core.sh

main "$@"
# Invoke the real thing
if [ "$is_crypto" -eq 1 ]; then
# Make sure the path to the outcomes file is absolute. This is done by
# pre_prepare_outcome_file() however by the time it runs we've already
# changed the working directory, so do it now.
if [ -n "${MBEDTLS_TEST_OUTCOME_FILE+set}" ]; then
case "$MBEDTLS_TEST_OUTCOME_FILE" in
[!/]*) MBEDTLS_TEST_OUTCOME_FILE="$PWD/$MBEDTLS_TEST_OUTCOME_FILE";;
esac
export MBEDTLS_TEST_OUTCOME_FILE
fi
cd tf-psa-crypto
exec tests/scripts/all.sh "$@"
else
exec tests/scripts/mbedtls-all.sh "$@"
fi
20 changes: 0 additions & 20 deletions tests/scripts/components-build-system.sh
Original file line number Diff line number Diff line change
Expand Up @@ -85,26 +85,6 @@ component_test_cmake_out_of_source () {
rm -rf "$OUT_OF_SOURCE_DIR"
}

component_test_cmake_tf_psa_crypto_out_of_source () {
# Remove existing generated files so that we use the ones cmake
# generates
make neat
msg "build: cmake tf-psa-crypto 'out-of-source' build"
MBEDTLS_ROOT_DIR="$PWD"
cd tf-psa-crypto
TF_PSA_CRYPTO_ROOT_DIR="$PWD"
mkdir "$OUT_OF_SOURCE_DIR"
cd "$OUT_OF_SOURCE_DIR"
# 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
cd "$TF_PSA_CRYPTO_ROOT_DIR"
rm -rf "$OUT_OF_SOURCE_DIR"
cd "$MBEDTLS_ROOT_DIR"
}

component_test_cmake_as_subdirectory () {
# Remove existing generated files so that we use the ones CMake
# generates
Expand Down
16 changes: 16 additions & 0 deletions tests/scripts/mbedtls-all.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#! /usr/bin/env bash

# all.sh (mbedtls part)
#
# Copyright The Mbed TLS Contributors
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later

# This file is executable; it is the entry point for users and the CI.
# See "Files structure" in all-core.sh for other files used.

# This script must be invoked from the project's root.

# The path is going to change when this is moved to the framework
source tests/scripts/all-core.sh

main "$@"
23 changes: 23 additions & 0 deletions tf-psa-crypto/tests/scripts/all.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#! /usr/bin/env bash

# all.sh
#
# Copyright The Mbed TLS Contributors
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later

# This file is executable; it is the entry point for users and the CI.
# See "Files structure" in all-core.sh for other files used.

# This script must be invoked from the project's root.

# Prevent silly mistakes when people would invoke this from mbedtls
if [ -d tf-psa-crypto -a -d library ]; then
echo "When invoking this script from an mbedtls checkout," >&2
echo "you must change the working directory to tf-psa-crypto." >&2
exit 255
fi

# The path is going to change when this is moved to the framework
source ../tests/scripts/all-core.sh

main "$@"
24 changes: 24 additions & 0 deletions tf-psa-crypto/tests/scripts/components-build-system.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# components-build-system.sh
#
# Copyright The Mbed TLS Contributors
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later

# This file contains test components that are executed by all.sh

################################################################
#### Build System Testing
################################################################

component_test_cmake_tf_psa_crypto_out_of_source () {
ronald-cron-arm marked this conversation as resolved.
Show resolved Hide resolved
msg "build: cmake tf-psa-crypto 'out-of-source' build"
TF_PSA_CRYPTO_ROOT_DIR="$PWD"
mkdir "$OUT_OF_SOURCE_DIR"
cd "$OUT_OF_SOURCE_DIR"
# 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Echoing Harry's concern in #9755 (comment) : we currently prevent component_test_cmake_out_of_source from running on Ubuntu 16.04, but now component_test_cmake_tf_psa_crypto_out_of_source will run on Ubuntu 16.04. Why is this not a problem? I don't see it discussed in the review here.

The problem is a race condition. Just having a small number of successful CI runs is not enough to gain confidence.

Maybe the cmake file in tf-psa-crypto is better written and is not susceptible to the race condition. That's quite plausible, but do we actually know that?

Copy link
Contributor

Choose a reason for hiding this comment

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

The component component_test_cmake_tf_psa_crypto_out_of_source is not added by this PR. This PR moves it from tests/scripts/components-build-system.sh to tf-psa-crypto/tests/scripts/components-build-system.sh. The component was added by the PR 9445 without protection against Ubuntu 16.04 (we just missed that point I think) which was merged October the 3rd. Thus it has been around for more that a month now. Enough to gain confidence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gilles-peskine-arm I think your concern is valid in general, but quite unrelated to this PR, as Ronald explained.

Feel free to override my change request if my concern is invalid.

Since you offered, I'm gonna do just that. I'm also going to start a discussion on slack about the potential issue and things we could do about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'd missed that it was a move and not new. I agree that a month of CI is enough to have confidence that this isn't a problem with the tf-psa-crypto cmake scripts.

make
msg "test: cmake tf-psa-crypto 'out-of-source' build"
make test
cd "$TF_PSA_CRYPTO_ROOT_DIR"
rm -rf "$OUT_OF_SOURCE_DIR"
}