Skip to content
Merged
Changes from 1 commit
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
28 changes: 19 additions & 9 deletions scripts/mbedtls_framework/psa_compliance.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later

import argparse
import glob
import os
import re
import shlex
import shutil
import subprocess
import sys
Expand All @@ -25,7 +27,7 @@
#pylint: disable=too-many-branches,too-many-statements,too-many-locals
def test_compliance(library_build_dir: str,
psa_arch_tests_ref: str,
patch: str,
patch_files: List[str],
expected_failures: List[int]) -> int:
"""Check out and run compliance tests.

Expand Down Expand Up @@ -59,12 +61,12 @@ def test_compliance(library_build_dir: str,
subprocess.check_call(['git', 'fetch', PSA_ARCH_TESTS_REPO, psa_arch_tests_ref])
subprocess.check_call(['git', 'checkout', '--force', 'FETCH_HEAD'])

if patch:
if patch_files:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the logic behind running git reset only if we have a patch to apply. It seems to me that git reset is needed if we had a patch to apply the last time we ran this script (and we are re-using the repo), which might not be the same as we have one now. Also, I don't think calling git reset when not strictly needed does any harm. So, I'd be inclined to just call it unconditionally. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to minimize changes in 3.6 which doesn't need any of the new fancy stuff. Also, arguably, it makes things worse for local debugging where you might want to edit the cached psa-arch-test tree until you get it right. On the other hand, I agree with you that I would have made git reset unconditional if I was doing this from scratch. I'll defer to Bence's preference on that since he know this script's history a lot better than we do.

Copy link
Contributor Author

@gilles-peskine-arm gilles-peskine-arm Sep 30, 2025

Choose a reason for hiding this comment

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

In my local copy, I've now changed this to be controlled by the psa-arch-tests ref specification:

  • With no argument or --tests-ref=REFSPEC, check that out, reset, and apply patches.
  • With --tests-ref=, reuse whatever is in the working tree: no git checkout, no reset, no patches. (To do reset+patch but not checkout, you can use --tests-ref=HEAD.)

Does that seem sensible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

subprocess.check_call(['git', 'reset', '--hard'])
subprocess.run(['patch', '-p1'],
check=True,
encoding='utf-8',
input=patch)
for patch_file in patch_files:
abs_path = os.path.abspath(os.path.join(root_dir, patch_file))
subprocess.check_call(['patch -p1 <' + shlex.quote(abs_path)],
shell=True)

build_dir = 'api-tests/build'
try:
Expand Down Expand Up @@ -143,15 +145,15 @@ def test_compliance(library_build_dir: str,
os.chdir(root_dir)

def main(psa_arch_tests_ref: str,
patch: str = '',
expected_failures: Optional[List[int]] = None) -> None:
"""Command line entry point.

psa_arch_tests_ref: tag or sha to use for the arch-tests.
patch: patch to apply to the arch-tests with ``patch -p1``.
expected_failures: default list of expected failures.
"""
build_dir = 'out_of_source_build'
default_patch_directory = os.path.join(build_tree.guess_project_root(),
'scripts/data_files/psa-arch-tests')

# pylint: disable=invalid-name
parser = argparse.ArgumentParser()
Expand All @@ -161,6 +163,9 @@ def main(psa_arch_tests_ref: str,
help='''set the list of test codes which are expected to fail
from the command line. If omitted the list given by
EXPECTED_FAILURES (inside the script) is used.''')
parser.add_argument('--patch-directory', nargs=1,
default=default_patch_directory,
help='Directory containing patches (*.patch) to apply to psa-arch-tests')
args = parser.parse_args()

if args.build_dir is not None:
Expand All @@ -173,7 +178,12 @@ def main(psa_arch_tests_ref: str,
else:
expected_failures_list = expected_failures

if args.patch_directory:
patch_files = glob.glob(os.path.join(args.patch_directory, '*.patch'))
else:
patch_files = []

sys.exit(test_compliance(build_dir,
psa_arch_tests_ref,
patch,
patch_files,
expected_failures_list))