musca: Improve missing python dependencies error #14643

Patater · 2021-05-07T16:20:00Z

Summary of changes

Previously, if intelhex, cbor, cryptography, or python3 weren't
available, we'd see an error message like the following:

[2/2] Running utility command for mbed-post-build-bin-ARM_MUSCA_B1
FAILED: mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/CMakeFiles/mbed-post-build-bin-ARM_MUSCA_B1.util
cd /Users/user/Code/mbed-os-example-blinky/cmake_build/ARM_MUSCA_B1/develop/GCC_ARM/mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1 && /usr/local/Frameworks/Python.framework/Versions/3.9/bin/python3.9 /Users/user/Code/mbed-os-example-blinky/mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py --tfm-target musca_b1 --target-path /Users/user/Code/mbed-os-example-blinky/mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1 --secure-bin /Users/user/Code/mbed-os-example-blinky/mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/tfm_s.bin --non-secure-bin /Users/user/Code/mbed-os-example-blinky/cmake_build/ARM_MUSCA_B1/develop/GCC_ARM/mbed-os-example-blinky.bin
Traceback (most recent call last):
  File "/Users/user/Code/mbed-os-example-blinky/mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 197, in <module>
    sign_and_merge_tfm_bin(args.tfm_target, args.target_path, args.non_secure_bin, args.secure_bin)
  File "/Users/user/Code/mbed-os-example-blinky/mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 80, in sign_and_merge_tfm_bin
    raise Exception("Unable to sign " + target_name +
Exception: Unable to sign musca_b1 secure binary, Error code: 1
ninja: build stopped: subcommand failed.
ERROR: CMake invocation failed!

More information may be available by using the command line option '-v'.

"Error code 1" is not very helpful. To provide a more helpful error
message, detect the dependencies and panic if the dependencies aren't
available when building for the target.

The new error message looks like so:

CMake Error at mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/CMakeLists.txt:10 (message):
  Missing Python dependencies (python3, cbor, click, intelhex) so the binary
  cannot be signed.  Please install requirements with:

    python3 -m pip install -r mbed-os/requirements.txt

This is better than before.

Fixes #14781

Impact of changes

Migration actions required

Documentation

None

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2021-05-07T16:30:17Z

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

0xc0170 · 2021-05-10T10:30:32Z

targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/CMakeLists.txt

+        message(FATAL_ERROR "Missing Python dependencies (python3, cbor, "
+        "click, intelhex) so the binary cannot be signed. Please install "
+        "requirements with:\n"
+        "\tpython3 -m pip install -r mbed-os/requirements.txt")


We initially wanted to separate the deps between the old tools and CMake.
This should point to https://github.com/ARMmbed/mbed-os/blob/master/tools/cmake/requirements.txt instead?

Not all this required packages are there listed?

mbed-os/requirements.txt are needed, not just tools/cmake/requirements.txt, because of the signing tools dependencies. Maybe the right solution is to add the dependencies needed for the post build hooks to tools/cmake/requirements.txt and point there.

How do we want to handle optional dependencies? Another requirements.txt? We have if statements in other cmake files to avoid printing the map file summary if intelhex is not installed, for example. However, intelhex is required for code signing on certain targets.

Maybe the right solution is to add the dependencies needed for the post build hooks to tools/cmake/requirements.txt and point there.

+1, they are missing there once we ported post build hooks 😞 I can create an issue for this, we should fix it.

Previously, if intelhex, cbor, cryptography, or python3 weren't

I understand it now better the error we are fixing here, we really missed some of the deps!

How do we want to handle optional dependencies?

Previous mbed way it was just add to to requirements and have it installed. Shall we split requirements. We always discussed we have different needs:

requirements - to install all below (for CI, or users who want to just install everything )
requirements-basic - this would be basic requirements for targets without any post build hooks
requirements-testing - testing (build and run together in one) - including unittest/greentea tests etc?
requirements-<vendor> - I assume going down to Mbed OS target would be too many files so rather each vendor would maintain their own requirements and keep it up to date. But maybe this is not needed and just introduce requirements-optional to contain all these. We can always check with CMake if deps are installed and if not , just point to extra requirements (in this case we know a user did not install all requirements so they need to cherry-pick extra or testing explicitly)

All the requirements files seems messy to me. It's more stuff for a user to have to read and learn about. I'll add the optional requirements to tools/cmake/requirements.txt for now, even if not all targets need them.

ciarmcom · 2021-05-13T11:50:12Z

This pull request has automatically been marked as stale because it has had no recent activity. , please complete review of the changes to move the PR forward. Thank you for your contributions.

adbridge · 2021-05-24T13:45:06Z

@Patater could you please address the review comments

ciarmcom · 2021-06-03T15:50:41Z

This pull request has automatically been marked as stale because it has had no recent activity. @Patater, please carry out any necessary work to get the changes merged. Thank you for your contributions.

Some targets have post-build scripts which have additional Python library requirements. List out the possibly required dependencies in tools/cmake/requirements.txt. When all tools/cmake/requirements.txt are installed, it should be less common to run into difficult to understand error messages.

Previously, if intelhex, cbor, cryptography, or python3 weren't available, we'd see an error message like the following: [2/2] Running utility command for mbed-post-build-bin-ARM_MUSCA_B1 FAILED: mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/CMakeFiles/mbed-post-build-bin-ARM_MUSCA_B1.util cd /Users/user/Code/mbed-os-example-blinky/cmake_build/ARM_MUSCA_B1/develop/GCC_ARM/mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1 && /usr/local/Frameworks/Python.framework/Versions/3.9/bin/python3.9 /Users/user/Code/mbed-os-example-blinky/mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py --tfm-target musca_b1 --target-path /Users/user/Code/mbed-os-example-blinky/mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1 --secure-bin /Users/user/Code/mbed-os-example-blinky/mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/tfm_s.bin --non-secure-bin /Users/user/Code/mbed-os-example-blinky/cmake_build/ARM_MUSCA_B1/develop/GCC_ARM/mbed-os-example-blinky.bin Traceback (most recent call last): File "/Users/user/Code/mbed-os-example-blinky/mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 197, in <module> sign_and_merge_tfm_bin(args.tfm_target, args.target_path, args.non_secure_bin, args.secure_bin) File "/Users/user/Code/mbed-os-example-blinky/mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 80, in sign_and_merge_tfm_bin raise Exception("Unable to sign " + target_name + Exception: Unable to sign musca_b1 secure binary, Error code: 1 ninja: build stopped: subcommand failed. ERROR: CMake invocation failed! More information may be available by using the command line option '-v'. "Error code 1" is not very helpful. To provide a more helpful error message, detect the dependencies and panic if the dependencies aren't available when building for the target. The new error message looks like so: CMake Error at mbed-os/targets/TARGET_ARM_SSG/TARGET_MUSCA_B1/CMakeLists.txt:10 (message): Missing Python dependencies (python3, cbor, click, intelhex) so the binary cannot be signed. Please install requirements with: python3 -m pip install -r mbed-os/tools/cmake/requirements.txt This is better than before.

Patater · 2021-06-11T13:51:36Z

Added additional requirements to tools/cmake/requirements.txt and updated the error message to indicate which requirements.txt to use.

0xc0170 · 2021-06-14T08:52:04Z

CI started

mbed-ci · 2021-06-14T09:22:51Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	❌
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️

0xc0170 · 2021-06-14T11:37:33Z

CMake failed to build due to missing deps. I'll talk to CI team to update.

0xc0170 · 2021-06-14T13:55:08Z

tools/cmake/requirements.txt

@@ -2,3 +2,5 @@ prettytable==0.7.2
 future==0.16.0
 Jinja2>=2.10.1,<2.11
 intelhex>=2.3.0,<3.0.0
+cbor>=1.0.0
+Click>=7.0,<7.1


shouldn't we add also cryptography>=2.5,<3 as it is required by tfm (it is in the old tools requirements

LDong-Arm

@Patater Thanks for the change. While the suggestion to install python packages is a good addition, I think it's quite important to make sure generate_mbed_image.py dumps the output of the command it calls, rather than just saying "unable to sign". This script was written by ourselves - I copied it from tools/targets/ARM_MUSCA.py and adapted it for CMake. So we are free to edit/improve it.

The error is raised by:

mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py

Lines 78 to 81 in 3cf5f8e

    
           retcode = run_cmd(cmd, MBED_OS_ROOT) 
        
           if retcode: 
        
               raise Exception("Unable to sign " + target_name + 
        
                                   " secure binary, Error code: " + str(retcode))

where run_cmd() is:

mbed-os/platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py

Lines 154 to 164 in 3cf5f8e

    
           def run_cmd(cmd, directory): 
        
               popen_instance = subprocess.Popen( 
        
                   cmd, 
        
                   stdout=subprocess.PIPE, 
        
                   stderr=subprocess.STDOUT, 
        
                   cwd=directory, 
        
               ) 
        
               popen_instance.communicate() 
        
               return popen_instance.returncode

We can do stdout, _ = popen_instance.communicate() and print stdout if the command failed. This should be enough to let the user know what the actual error is.
(The existing stderr=subprocess.STDOUT redirects stderr to the subprocess's stdout, but that doesn't seem to be shown to the user? I personally expect a generic python import error, if a module is missing, if this is the case?)

0xc0170 · 2021-06-16T14:07:53Z

tools/cmake/requirements.txt

@@ -2,3 +2,5 @@ prettytable==0.7.2
 future==0.16.0
 Jinja2>=2.10.1,<2.11
 intelhex>=2.3.0,<3.0.0
+cbor>=1.0.0
+Click>=7.0,<7.1


It should be the same as in #14780: Click>=7.0,<8

Patater · 2021-07-06T10:52:19Z

I don't like that this duplicates the list of dependencies already in requirements.txt. Better solution is to print the actual error message instead of hide it, as PR #14783 does.

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label May 7, 2021

ciarmcom requested a review from a team May 7, 2021 16:30

ciarmcom added needs: review devices: arm labels May 7, 2021

0xc0170 reviewed May 10, 2021

View reviewed changes

ciarmcom added the stale Stale Pull Request label May 13, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 14, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 21, 2021

adbridge added needs: work and removed needs: review labels May 24, 2021

ciarmcom removed the stale Stale Pull Request label May 24, 2021

ciarmcom added the stale Stale Pull Request label Jun 3, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 4, 2021

Patater force-pushed the improve-musca-python-dependencies-signing-error branch from cbf9f98 to 5d95b22 Compare June 11, 2021 13:45

Patater requested a review from 0xc0170 June 11, 2021 13:45

Patater force-pushed the improve-musca-python-dependencies-signing-error branch from 5d95b22 to ca0cac7 Compare June 11, 2021 13:51

0xc0170 added needs: review and removed needs: work stale Stale Pull Request labels Jun 11, 2021

0xc0170 approved these changes Jun 11, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jun 11, 2021

mergify bot added needs: work and removed needs: CI labels Jun 14, 2021

0xc0170 reviewed Jun 15, 2021

View reviewed changes

LDong-Arm suggested changes Jun 15, 2021

View reviewed changes

Patater mentioned this pull request Jun 16, 2021

Update python requirements #14780

Merged

0xc0170 reviewed Jun 16, 2021

View reviewed changes

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 24, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 2, 2021

Patater closed this Jul 6, 2021

mergify bot removed devices: arm needs: work release-type: patch Indentifies a PR as containing just a patch stale Stale Pull Request labels Jul 6, 2021

	retcode = run_cmd(cmd, MBED_OS_ROOT)
	if retcode:
	raise Exception("Unable to sign " + target_name +
	" secure binary, Error code: " + str(retcode))

	def run_cmd(cmd, directory):

	popen_instance = subprocess.Popen(
	cmd,
	stdout=subprocess.PIPE,
	stderr=subprocess.STDOUT,
	cwd=directory,
	)

	popen_instance.communicate()
	return popen_instance.returncode

musca: Improve missing python dependencies error #14643

musca: Improve missing python dependencies error #14643

Uh oh!

Conversation

Patater commented May 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented May 7, 2021

Uh oh!

0xc0170 May 10, 2021

Choose a reason for hiding this comment

Uh oh!

Patater Jun 10, 2021 • 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!

Patater Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jun 10, 2021 • 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!

Patater Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

ciarmcom commented May 13, 2021

Uh oh!

adbridge commented May 24, 2021

Uh oh!

ciarmcom commented Jun 3, 2021

Uh oh!

Patater commented Jun 11, 2021

Uh oh!

0xc0170 commented Jun 14, 2021

Uh oh!

mbed-ci commented Jun 14, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

0xc0170 commented Jun 14, 2021

Uh oh!

0xc0170 Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

LDong-Arm left a comment • 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!

0xc0170 Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

Patater commented Jul 6, 2021

Uh oh!

Uh oh!

Patater commented May 7, 2021 •

edited

Loading

Patater Jun 10, 2021 •

edited

Loading

0xc0170 Jun 10, 2021 •

edited

Loading

LDong-Arm left a comment •

edited

Loading