tfm-post-build: Don't capture subprocess stdout #14783

rwalton-arm · 2021-06-16T13:23:32Z

Summary of changes

subprocess.PIPE is used to enable the parent process to communicate with
the subprocess via pipes, which mean all stdout and stderr messages are
captured and returned as part of Popen.communicate's result tuple.

In our case, we want to display the error messages on the console, so we
don't need to capture the output from stdout.

Example of a typical error message before this change:

Traceback (most recent call last):
  File "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 "platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 81, in sign_and_merge_tfm_bin
    " secure binary, Error code: " + str(retcode))
Exception: Unable to sign musca_b1 secure binary, Error code: 1

Example of the error message after this change:

Traceback (most recent call last):
  File "/mbed-os/tools/psa/tfm/bin_utils/wrapper.py", line 13, in <module>
    import click
ModuleNotFoundError: No module named 'click'
Traceback (most recent call last):
  File "platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 194, in <module>
    sign_and_merge_tfm_bin(args.tfm_target, args.target_path, args.non_secure_bin, args.secure_bin)
  File "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

Impact of changes

Migration actions required

Documentation

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

LDong-Arm

@rwalton-arm Thanks for the change.

Please also make the same change to the script for CLI 1:

mbed-os/tools/targets/ARM_MUSCA.py

Lines 161 to 171 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

ciarmcom · 2021-06-16T13:30:15Z

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

rwalton-arm · 2021-06-16T13:30:52Z

@rwalton-arm Thanks for the change.

Please also make the same change to the script for CLI 1:

mbed-os/tools/targets/ARM_MUSCA.py

Lines 161 to 171 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

Thanks. I'll add it there, too. I didn't realise we had a different script for CLI 1. Good catch.

LDong-Arm · 2021-06-16T13:42:15Z

Thanks. I'll add it there, too. I didn't realise we had a different script for CLI 1. Good catch.

No problem. Yea the scripts are slightly different but the CLI 2 script was derived from the CLI 1 one.

The idea was to make the CLI 2 script + CMake hook general purpose enough for all TF-M (not just Musca), but Nuvoton told us our script took too few arguments so they ended up making their own for their TF-M target. A future improvement would be extending our generic TF-M signing script to support that too. (Slightly off topic here.)

LDong-Arm · 2021-06-16T13:54:11Z

^ Speaking of this, actually Nuvoton's script dumps the stdout only when there's an error:

mbed-os/targets/TARGET_NUVOTON/scripts/NUVOTON.py

Lines 178 to 201 in 3cf5f8e

    
           def run_cmd(cmd, directory): 
        
               # Redirect stdout/stderr to pipe, text mode 
        
               POPEN_INSTANCE = subprocess.Popen( 
        
                   cmd, 
        
                   stdout=subprocess.PIPE, 
        
                   stderr=subprocess.STDOUT, 
        
                   cwd=directory, 
        
                   universal_newlines=True 
        
               ) 
        
               # Command line 
        
               print("COMMAND: {}".format(POPEN_INSTANCE.args)) 
        
               stdout_data, stderr_data = POPEN_INSTANCE.communicate() 
        
               # stdout/stderr messages 
        
               if (stdout_data): 
        
                   print(stdout_data) 
        
               if (stderr_data): 
        
                   print(stderr_data) 
        
               # Return code 
        
               return POPEN_INSTANCE.returncode

However, the signing script isn't particularly verbose - it doesn't print anything unless an error occurs. So I'm okay with not capturing stdout.

LDong-Arm

LGTM

rwalton-arm · 2021-06-16T14:09:57Z

Looks like travis failed due to an apt-get connection error.

Err:36 http://archive.ubuntu.com/ubuntu focal InRelease

  Could not connect to archive.ubuntu.com:80 (91.189.88.152), connection timed out Could not connect to archive.ubuntu.com:80 (91.189.88.142), connection timed out

Err:37 http://archive.ubuntu.com/ubuntu focal-updates InRelease

  Unable to connect to archive.ubuntu.com:http:

Err:38 http://archive.ubuntu.com/ubuntu focal-backports InRelease

  Unable to connect to archive.ubuntu.com:http:

Fetched 4,078 kB in 31s (133 kB/s)

Reading package lists...

W: Failed to fetch http://archive.ubuntu.com/ubuntu/dists/focal/InRelease  Could not connect to archive.ubuntu.com:80 (91.189.88.152), connection timed out Could not connect to archive.ubuntu.com:80 (91.189.88.142), connection timed out

W: Failed to fetch http://archive.ubuntu.com/ubuntu/dists/focal-updates/InRelease  Unable to connect to archive.ubuntu.com:http:

W: Failed to fetch http://archive.ubuntu.com/ubuntu/dists/focal-backports/InRelease  Unable to connect to archive.ubuntu.com:http:

W: Some index files failed to download. They have been ignored, or old ones used instead.

The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends $(travis_apt_get_options) install cmake ninja-build gcovr libncursesw5 g++-7" failed and exited with 100 during .

Your build has been stopped.

ciarmcom · 2021-06-28T15:50:46Z

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

LDong-Arm · 2021-06-28T16:31:20Z

Same failure when I manually reran Travis on this PR. I don't think the error is related to the change though.

0xc0170 · 2021-06-29T11:43:10Z

Same failure when I manually reran Travis on this PR. I don't think the error is related to the change though.

@rwalton-arm Let's rebase and that will retrigger the Travis, it should pass.

subprocess.PIPE is used to enable the parent process to communicate with the subprocess via pipes, which mean all stdout and stderr messages are captured and returned as part of Popen.communicate's result tuple. In our case, we want to display the error messages on the console, so we don't need to capture the output from stdout. Example of a typical error message before this change: ``` Traceback (most recent call last): File "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 "platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 81, in sign_and_merge_tfm_bin " secure binary, Error code: " + str(retcode)) Exception: Unable to sign musca_b1 secure binary, Error code: 1 ``` Example of the error message after this change: ``` Traceback (most recent call last): File "/mbed-os/tools/psa/tfm/bin_utils/wrapper.py", line 13, in <module> import click ModuleNotFoundError: No module named 'click' Traceback (most recent call last): File "platform/FEATURE_EXPERIMENTAL_API/FEATURE_PSA/TARGET_TFM/TARGET_TFM_LATEST/scripts/generate_mbed_image.py", line 194, in <module> sign_and_merge_tfm_bin(args.tfm_target, args.target_path, args.non_secure_bin, args.secure_bin) File "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 ``` This is a significant improvement as now you can see what the reason for the failure was.

Pull request has been modified.

rwalton-arm · 2021-06-30T10:55:24Z

Frozen tools check is failing because I modified https://github.com/ARMmbed/mbed-os/blob/master/tools/targets/ARM_MUSCA.py

@Patater can we ignore the frozen tools check for this?

rwalton-arm · 2021-06-30T11:05:48Z

Thanks. I'll add it there, too. I didn't realise we had a different script for CLI 1. Good catch.

No problem. Yea the scripts are slightly different but the CLI 2 script was derived from the CLI 1 one.

The idea was to make the CLI 2 script + CMake hook general purpose enough for all TF-M (not just Musca), but Nuvoton told us our script took too few arguments so they ended up making their own for their TF-M target. A future improvement would be extending our generic TF-M signing script to support that too. (Slightly off topic here.)

We should really wrap the TF-M post build scripts up into a python package; rather than a collection of similar scripts that spin up subprocesses to reuse some "common" python code, with hard coded file paths. If we make this a pypi package, we can reuse common code easily using the python import system, manage dependencies properly, and make the interface generic enough to handle the various use cases. Maybe we could then even add some unit tests to prove it all works as expected.

jeromecoutant · 2021-06-30T12:13:03Z

Frozen tools check is failing because I modified https://github.com/ARMmbed/mbed-os/blob/master/tools/targets/ARM_MUSCA.py

@Patater can we ignore the frozen tools check for this?

Move script in targets/TARGET_ARM_SSG...

Patater

Tools changes only to files used by targets not supported in the online compiler.

Patater · 2021-07-06T10:56:25Z

CI started

mbed-ci · 2021-07-06T11:32:57Z

Jenkins CI Test : ✔️ SUCCESS

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	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_tfm-integration	✔️

LDong-Arm suggested changes Jun 16, 2021

View reviewed changes

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

ciarmcom requested a review from a team June 16, 2021 13:30

ciarmcom added needs: review component: security component: core labels Jun 16, 2021

rwalton-arm force-pushed the dont_capture_stdout branch from 38553db to b20ac90 Compare June 16, 2021 13:32

LDong-Arm previously approved these changes Jun 16, 2021

View reviewed changes

0xc0170 previously approved these changes Jun 16, 2021

View reviewed changes

mergify bot added needs: work and removed needs: review labels Jun 16, 2021

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

rwalton-arm force-pushed the dont_capture_stdout branch from b20ac90 to 6d78f93 Compare June 29, 2021 14:38

ciarmcom removed the stale Stale Pull Request label Jun 29, 2021

rwalton-arm linked an issue Jun 30, 2021 that may be closed by this pull request

TF-M post build hooks produce opaque error messages #14781

Closed

0xc0170 approved these changes Jul 1, 2021

View reviewed changes

0xc0170 added needs: review and removed needs: work labels Jul 1, 2021

0xc0170 requested a review from Patater July 1, 2021 09:07

Patater approved these changes Jul 1, 2021

View reviewed changes

LDong-Arm approved these changes Jul 5, 2021

View reviewed changes

This was referenced Jul 6, 2021

musca: Improve missing python dependencies error #14643

Closed

TF-M post build hooks produce opaque error messages #14781

Closed

Patater merged commit 75808ea into ARMmbed:master Jul 6, 2021

mbedmain added release-version: 6.13.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tfm-post-build: Don't capture subprocess stdout #14783

tfm-post-build: Don't capture subprocess stdout #14783

rwalton-arm commented Jun 16, 2021

LDong-Arm left a comment

ciarmcom commented Jun 16, 2021

rwalton-arm commented Jun 16, 2021

LDong-Arm commented Jun 16, 2021 •

edited

Loading

LDong-Arm commented Jun 16, 2021

LDong-Arm left a comment

rwalton-arm commented Jun 16, 2021

ciarmcom commented Jun 28, 2021

LDong-Arm commented Jun 28, 2021

0xc0170 commented Jun 29, 2021

rwalton-arm commented Jun 30, 2021

rwalton-arm commented Jun 30, 2021

jeromecoutant commented Jun 30, 2021

Patater left a comment

Patater commented Jul 6, 2021

mbed-ci commented Jul 6, 2021

	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

tfm-post-build: Don't capture subprocess stdout #14783

tfm-post-build: Don't capture subprocess stdout #14783

Conversation

rwalton-arm commented Jun 16, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

LDong-Arm left a comment

Choose a reason for hiding this comment

ciarmcom commented Jun 16, 2021

rwalton-arm commented Jun 16, 2021

LDong-Arm commented Jun 16, 2021 • edited Loading

LDong-Arm commented Jun 16, 2021

LDong-Arm left a comment

Choose a reason for hiding this comment

rwalton-arm commented Jun 16, 2021

ciarmcom commented Jun 28, 2021

LDong-Arm commented Jun 28, 2021

0xc0170 commented Jun 29, 2021

rwalton-arm commented Jun 30, 2021

rwalton-arm commented Jun 30, 2021

jeromecoutant commented Jun 30, 2021

Patater left a comment

Choose a reason for hiding this comment

Patater commented Jul 6, 2021

mbed-ci commented Jul 6, 2021

Jenkins CI Test : ✔️ SUCCESS

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

LDong-Arm commented Jun 16, 2021 •

edited

Loading