Skip to content

Update PSA tools #10010

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

Merged
merged 3 commits into from
Mar 11, 2019
Merged

Update PSA tools #10010

merged 3 commits into from
Mar 11, 2019

Conversation

orenc17
Copy link
Contributor

@orenc17 orenc17 commented Mar 8, 2019

Description

  • Update PSA tools documentation
    Add option to git commit for each platform
  • Docstrings for all functions
  • Small refactor

Pull request type

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

Reviewers

Release Notes

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 8, 2019

CI started

Copy link
Contributor

@alzix alzix left a comment

Choose a reason for hiding this comment

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

LGTM

@mbed-ci
Copy link

mbed-ci commented Mar 8, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Meets criteria, PSA feature delivery. Approved.

@adbridge
Copy link
Contributor

adbridge commented Mar 8, 2019

Moved to a fix type as this now has a code commit as well

@ghost ghost added the PM_ACCEPTED label Mar 8, 2019
@adbridge
Copy link
Contributor

adbridge commented Mar 8, 2019

CI restarted

'commit',
'-m', commit_message
]

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried this on both windows and linux ? We had problems running on linux with this format of command passing to subprocess. Hence we do it this way:

def run_cmd(command, exit_on_failure=False):
    """ Run a system command returning a status result
    
    This is just a wrapper for the run_cmd_with_output() function, but
    only returns the status of the call.
    
    Args:
    command - system command as a list of tokens
    exit_on_failure - If True exit the program on failure (default = False)
    
    Returns:
    return_code - True/False indicating the success/failure of the command
    """
    return_code, _ = run_cmd_with_output(command, exit_on_failure)       
    return return_code

def run_cmd_with_output(command, exit_on_failure=False):
    """ Run a system command returning a status result and any command output
    
    Passes a command to the system and returns a True/False result once the 
    command has been executed, indicating success/failure. If the command was 
    successful then the output from the command is returned to the caller.
    Commands are passed as a string. 
    E.g. The command 'git remote -v' would be passed in as "git remote -v"
    Args:
    command - system command as a string
    exit_on_failure - If True exit the program on failure (default = False)
    
    Returns:
    return_code - True/False indicating the success/failure of the command
    output - The output of the command if it was successful, else empty string
    """
    text = '[Exec] ' + command
    userlog.debug(text)
    returncode = 0
    output = ""
    try:
        output = subprocess.check_output(command, shell=True)
    except subprocess.CalledProcessError as e:
        text = "The command " + str(command) + "failed with return code: " + str(e.returncode)
        userlog.warning(text)
        returncode = e.returncode
        if exit_on_failure:
            sys.exit(1)
    return returncode, output


Then called by:
cmd = "git rev-parse HEAD"
return_code, sha = run_cmd_with_output(cmd, exit_on_failure=True)

This method works on both platforms .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested on Mac

Copy link
Contributor

Choose a reason for hiding this comment

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

shell=False is a default setting and considered to be more secure as it prevent shell injection. It should be our choice of preference when calling to subprocess.
It works perfectly on linux. But there are some limitations about it that you should be aware of. E.g. aliases are not honored. Environment variables passed via $ sign are not interpolated, e.t.c.
Oren's script is perfectly valid

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested on Mac

Was expecting more of a "Tested in multiple OSs"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmonr I think it's quite good considering it's my day off

Copy link
Contributor

Choose a reason for hiding this comment

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

@alzix As we are only ever running internally on the ARM network then shell injection is hardly a major concern. I didn't say his script wasn't valid, I merely pointed out that there can be a number of issues running it that way including compatibility on windows and linux, which were born out by much testing of our own scripts. Providing his script runs fine on all the different OS' (which it seems is the case) then I have no problem with it.

@adbridge
Copy link
Contributor

adbridge commented Mar 8, 2019

NOTE: https://github.com/ARMmbed/mbed-os-release/pull/82 is dependent on this going in first and being compatible.

@mbed-ci
Copy link

mbed-ci commented Mar 8, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 2
Build artifacts

@orenc17 orenc17 changed the title Update docs for PSA tools Update PSA tools Mar 8, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 8, 2019

CI restarted

@adbridge review answered?

@mbed-ci
Copy link

mbed-ci commented Mar 8, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

@NirSonnenschein
Copy link
Contributor

started CI on latest fix

@orenc17
Copy link
Contributor Author

orenc17 commented Mar 10, 2019

@adbridge Tested on windows and linux as well

@mbed-ci
Copy link

mbed-ci commented Mar 10, 2019

Test run: FAILED

Summary: 1 of 9 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC6

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 10, 2019

Can you review latest failures? And fix the latest commit typo 1a47960a2b23774c4af219722a0c5149f6e7d596 (the fist line in the commit msg)

@mbed-ci
Copy link

mbed-ci commented Mar 10, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 5
Build artifacts

@NirSonnenschein
Copy link
Contributor

@0xc0170 latest test run succeeded.
@orenc17 please fix the typo (#10010 (comment))

* Replace call with check_call to throw exception on failure
* Check if binaries actually been changes before calling git commit
* Docstrings for all functions
* Small refactor
@orenc17
Copy link
Contributor Author

orenc17 commented Mar 10, 2019

@0xc0170 @NirSonnenschein Typo fixed

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 11, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Mar 11, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 6
Build artifacts

@0xc0170 0xc0170 merged commit ba24cb2 into ARMmbed:master Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants