TF-M: Python script to clone TF-M #11651

urutva · 2019-10-08T10:48:35Z

Description

We are working on automating building TF-M for multiple targets. This PR is first step towards that and implements cloning TF-M repository and its dependencies.
The script also creates components/TARGET_PSA/TARGET_TFM/VERSION.TXT which contains the git version of cloned TF-M repository.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@Patater @jainvikas8

Release Notes

ciarmcom · 2019-10-08T11:00:31Z

@Devran01, thank you for your changes.
@Patater @jainvikas8 @ARMmbed/mbed-os-maintainers please review.

0Grit · 2019-10-10T06:53:08Z

@0xc0170, @mark-edgeworth

Is this breaking any sort of convention as to how external repositories are synced into Mbed OS?

I want to start seeing a standard tool used for maintaining external dependencies.
Mbed-Cli is too intrusive to our version control process for us to use and trust .libs.

@theotherjimmy

jainvikas8 · 2019-10-11T11:09:43Z

tools/psa/build_tfm.py

+    Current version of the script only clones TF-M git repo and dependencies
+    and creates a VERSION.TXT file under 'components/TARGET_PSA/TARGET_TFM'
+    """
+    if not isdir(TF_M_BUILD_DIR):


It would be ideal to delete all the files if the TF_M_BUILD_DIR is present. Because we are not checking which version of cloned repo is present when calling check_clone_repo

@jainvikas8 It might be possible that users might have changes in the tf-m build directory. So it is not safe to assume that it can be deleted. Added a check to verify the version of cloned repos with the expected ones if the tf-m build directory already exists and warn the user if the versions don't match.

Well, this would imply users can only have their changes saved locally, but can't pull in their git branches as the tag/version would change to a different HEAD. Is my understanding right?

jainvikas8 · 2019-10-11T11:19:46Z

tools/psa/build_tfm.py

+import sys
+import subprocess
+import logging
+import argparse


Do we need this import?

jainvikas8 · 2019-10-11T11:43:57Z

tools/psa/build_tfm.py

+sys.path.insert(0, ROOT)
+from tools.targets import Target, TARGET_MAP, TARGET_NAMES
+
+TF_M_BUILD_DIR = join(ROOT, os.pardir, 'tfm_build_dir')


Wouldn't it be good to use the mbed-os/BUILD directory to have everything there, as it is a dedicated build folder?

We are planning to use the mbed-os importer to bring in TF-M PSA APIs for non-secure side. Therefore we cannot use mbed-os/BUILD directory. The importer script expects the source to be outside of mbed-os.
Another reason is not be interfere with mbed-cli building non-secure side (mbed-os). If we use mbed-os/BUILD then mbed-cli will try to compie tf-m sources when non-secure side is being built.

jainvikas8 · 2019-10-11T11:46:49Z

tools/psa/build_tfm.py

+    """
+    cmd = ['git', '-C', tfm_dir, 'describe', '--tags', '--abbrev=12', '--dirty', '--always']
+    tfm_version = run_cmd_return_output(cmd)
+    logger.debug('TF-M version: %s', tfm_version)


Please make it logger.info

mark-edgeworth · 2019-10-14T12:43:39Z

Is this breaking any sort of convention as to how external repositories are synced into Mbed OS?

Not that I am aware of, no.

Patater · 2019-10-16T12:12:49Z

tools/psa/build_tfm.py

+sys.path.insert(0, ROOT)
+
+TF_M_BUILD_DIR = join(ROOT, os.pardir, 'tfm_build_dir')
+VERSION_FILE_PATH = join(ROOT, 'features/FEATURE_PSA/FEATURE_TFM')


The commit message needs updating. It still says "components".

@Patater fixed.

Patater · 2019-10-16T12:13:28Z

tools/psa/build_tfm.py

+    ['https://git.trustedfirmware.org/trusted-firmware-m.git',
+     'feature-twincpu'],
+    "mbedtls": ['https://github.com/ARMmbed/mbedtls.git',
+                'mbedtls-2.7.9'],


Wow, that's a really old Mbed TLS.

Patater · 2019-10-16T12:46:33Z

tools/psa/build_tfm.py

+    Clone TF-M git repos and it's dependencies
+    """
+    check_and_clone_repo('trusted-firmware-m', dependencies)
+    check_and_clone_repo('mbedtls', dependencies)


So, this puts tf-m and all dependencies side by side into a single directly. How does TF-M pick up the dependencies and use them?

Yeah, that's the preferred way of building TF-M. The dependencies are cloned outside the TF-M source tree and the cmake scripts handle the inclusion of dependent modules.

OK, so we can point TF-M's build system to any filesystem location for the dependencies.

I need to check the cmake files but I think it is parent folder of tf-m is where the cmake will look for dependencies.

Patater

LGTM

mark-edgeworth · 2019-10-16T14:36:15Z

tools/psa/build_tfm.py

+                     _out.strip('\n'), deps.get(name)[1])
+        logger.error('check and remove folder %s',
+                     join(basedir, name))
+        sys.exit(1)


Do not call sys.exit(1). This will crash out of the program and will do no cleanup. It also will fail in the online system.

@mark-edgeworth In this case we need to exit from the script without doing any cleanup since the cloned repos might contain user changes. Therefore, the only thing we do here is to warn the user and let them take the action.
Having said that, this path will never be triggered in the online system as there won't be any changes done by it.

mark-edgeworth · 2019-10-16T14:36:19Z

tools/psa/build_tfm.py

+    except subprocess.CalledProcessError as e:
+        logger.error("The command %s failed with return code: %s",
+                     (' '.join(command)), e.returncode)
+        sys.exit(1)


Do not call sys.exit(1).

mark-edgeworth · 2019-10-16T14:43:16Z

tools/psa/build_tfm.py

+    output = ''
+    logger.debug('[Exec] %s', ' '.join(command))
+    try:
+        fnull = open(os.devnull, 'w')


fnull is not closed; use with open() as fnull: instead

mark-edgeworth · 2019-10-16T14:46:23Z

tools/psa/build_tfm.py

+    basedir = TF_M_BUILD_DIR
+    if not isdir(join(basedir, name)):
+        logger.info('Cloning %s repo', name)
+        cmd = ['git', '-C', basedir, 'clone', '-b',


This (and other command calls to git) make assumptions that git is installed on the user's machine. None of this will work in the online build system either.

Added is_git_installed() and is_git_lfs_installed() method to check the availability of required tools.

jainvikas8 · 2019-10-18T13:01:25Z

tools/psa/build_tfm.py

+        cmd = ['git', '-C', join(basedir, name),
+               'describe', '--tags']
+    _out = run_cmd_and_return_output(cmd)
+    if _out.strip('\n') != deps.get(name)[1]:


Getting an error when executing this script
mbed-os$ /usr/bin/python3 /mbed-os/tools/psa/build_tfm.py [TF-M-Builder] 12:44:27: trusted-firmware-m repo exists, checking git version.... Traceback (most recent call last): File "/mbed-os/tools/psa/build_tfm.py", line 181, in <module> main() File "/mbed-os/tools/psa/build_tfm.py", line 173, in main clone_tfm_repo() File "/mbed-os/tools/psa/build_tfm.py", line 149, in clone_tfm_repo check_and_clone_repo('trusted-firmware-m', dependencies) File "/mbed-os/tools/psa/build_tfm.py", line 143, in check_and_clone_repo check_repo_version(name, deps) File "/mbed-os/tools/psa/build_tfm.py", line 119, in check_repo_version if _out.strip('\n') != deps.get(name)[1]: TypeError: a bytes-like object is required, not 'str'

Is it related to python3, if yes are we not supporting Python 3 as Python 2.7 support will be ending soon?

@jainvikas8 Added support for python3

jainvikas8

Looks fine...

0xc0170 · 2019-10-21T13:49:43Z

One question: how this scripts are being tested?

mark-edgeworth

I have deferred this to Graham who has more python experience than I.

urutva · 2019-10-22T08:10:12Z

One question: how this scripts are being tested?

@0xc0170 Currently only manual validation, from mbed-os root folder python3 tools/psa/build_tfm.py.
The script only clones tf-m and it's dependencies and writes tf-m version into version file. But in future, we are going to extend this script to compile tf-m (out of mbed-os tree) and commit the secure binary into mbed-os.

Wil be updated by Graham

0xc0170 · 2019-10-29T09:16:13Z

It can't checkout the branch. @jamesbeyond Is this related to the changes to master to examples? If yes, would rebase help ?

urutva · 2019-10-30T11:44:07Z

@0xc0170 The issue is related to python3 and examples scripts. It looks like the python version in CI is python3 and the example script in this feature branch is not compatible with it. This feature branch is created from a commit which is older and had the support for PSoC62 which is required to validate TF-M integration. Therefore rebasing to master is not an option.

I can push a patch to resolve this error. But I'm not sure if we'll encounter other issues.

@Patater Thoughts?

0xc0170 · 2019-10-30T11:51:38Z

I can push a patch to resolve this error. But I'm not sure if we'll encounter other issues.

If you can patch it on this branch, will be cleaned once this is rebased (needs to be before the integration to master).

urutva · 2019-10-30T14:12:55Z

@0xc0170 Cherry picked @jamesbeyond's changes from master.

urutva · 2019-10-30T14:46:38Z

@0xc0170 All travis checks have passed. Can you please start the CI?

0xc0170 · 2019-10-30T15:01:40Z

Ci started

urutva · 2019-10-30T16:18:59Z

@0xc0170 The required patch was added before the merge commit I cherry picked. So the CI failed again. Fixed it now. Can you please restart the CI?

urutva · 2019-10-31T13:59:38Z

CI Started

mbed-ci · 2019-10-31T15:56:20Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM

mbed-ci · 2019-10-31T16:49:08Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-IAR

mbed-ci · 2019-11-01T11:47:50Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 7
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-IAR

0xc0170 · 2019-11-05T09:21:37Z

@Devran01 you should rebase feature branch rather than your fork (your branch now contains all commits from rebase)

Create a script (tools/psa/build_tfm.py) to build Trusted Firmware M (TF-M) image. Current version of the script only clones the TF-M git repo and its dependencies and computes the version information of TF-M repo and write it to 'features/FEATURE_PSA/FEATURE_TFM/VERSION.txt' Signed-off-by: Devaraj Ranganna <[email protected]>

urutva · 2019-11-05T14:38:52Z

@0xc0170 Done. Can you please start the CI?

0xc0170 · 2019-11-08T12:26:42Z

CI restarted

mbed-ci · 2019-11-08T14:18:44Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 8
Build artifacts

ciarmcom requested review from jainvikas8, Patater and a team October 8, 2019 11:00

ciarmcom added the needs: review label Oct 8, 2019

jainvikas8 reviewed Oct 11, 2019

View reviewed changes

Patater reviewed Oct 16, 2019

View reviewed changes

urutva force-pushed the feature-twincpu-build branch from 57469a2 to 70194f3 Compare October 16, 2019 14:27

Patater approved these changes Oct 16, 2019

View reviewed changes

mark-edgeworth previously requested changes Oct 16, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Oct 17, 2019

jainvikas8 reviewed Oct 18, 2019

View reviewed changes

urutva force-pushed the feature-twincpu-build branch 2 times, most recently from 3135d0f to a27c877 Compare October 18, 2019 14:54

jainvikas8 approved these changes Oct 21, 2019

View reviewed changes

0xc0170 requested a review from mark-edgeworth October 21, 2019 13:37

0xc0170 added needs: review release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: work labels Oct 21, 2019

0xc0170 approved these changes Oct 21, 2019

View reviewed changes

mark-edgeworth requested a review from madchutney October 21, 2019 13:54

jainvikas8 approved these changes Oct 21, 2019

View reviewed changes

mark-edgeworth reviewed Oct 21, 2019

View reviewed changes

0xc0170 requested a review from mark-edgeworth October 22, 2019 08:53

0xc0170 added the release-version: feature branch label Oct 29, 2019

0xc0170 added needs: work and removed needs: CI labels Oct 29, 2019

urutva force-pushed the feature-twincpu-build branch from 2b72d27 to ab1ac95 Compare October 30, 2019 15:40

urutva force-pushed the feature-twincpu-build branch from ab1ac95 to bd51999 Compare October 31, 2019 18:18

urutva force-pushed the feature-twincpu-build branch from bd51999 to 1c0796b Compare November 4, 2019 14:21

urutva force-pushed the feature-twincpu-build branch from 1c0796b to dfe5351 Compare November 5, 2019 14:38

0xc0170 added needs: CI and removed needs: work labels Nov 8, 2019

adbridge added ready for merge and removed needs: CI labels Nov 11, 2019

adbridge merged commit 8f28e3e into ARMmbed:feature_twincpu Nov 11, 2019

adbridge removed the ready for merge label Nov 11, 2019

urutva deleted the feature-twincpu-build branch November 11, 2019 10:42

TF-M: Python script to clone TF-M #11651

TF-M: Python script to clone TF-M #11651

Uh oh!

Conversation

urutva commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Oct 8, 2019

Uh oh!

0Grit commented Oct 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-edgeworth commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urutva Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urutva Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jainvikas8 Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

urutva commented Oct 8, 2019 •

edited

Loading

mark-edgeworth commented Oct 14, 2019 •

edited

Loading

urutva Oct 16, 2019 •

edited

Loading

urutva Oct 17, 2019 •

edited

Loading

jainvikas8 Oct 18, 2019 •

edited

Loading

urutva commented Oct 30, 2019 •

edited

Loading