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

Python script to add cmsis/rtx changes in mbed-os #5407

Merged
merged 6 commits into from Nov 20, 2017

Conversation

@deepikabhavnani
Contributor

deepikabhavnani commented Oct 30, 2017

Input to script is json in which user can list all folder/files need to picked up from CMSIS repository.
Single commit is automatically created with all those new files.
On top of that SHA from mbed-os repo is cherry-picked to reflect changes in CMSIS files.

@bulislaw @sg- @c1728p9

Drawback to SHA will be, conflicts which user will have to resolve themselves

@bulislaw

Thanks Deepika, good starting point. Some comments though.

tools/importer/cmsis_importer.json Outdated
"files" : [
{
"cmsis_file" : "CMSIS/Core/Template/ARMv8-M/tz_context.c",
"mbed_file" : "platform/mbed_tz_context.c"

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

that shouldn't be in platform, but cmsis/TARGET_CORTEX_M/

tools/importer/cmsis_importer.json Outdated
"mbed_folder" : "cmsis/TARGET_CORTEX_A/"
}
],
"Mbed_sha" : [

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

Can we have better name for that? commit_sha?

tools/importer/README.md Outdated
This directory contains scripts to import latest CMSIS/RTX code into mbed-os local repository.
CMSIS/RTX fixes and new releases are imported into mbed-os on regular basis using the `cmsis_importer.py` python script input to which is `cmsis_importer.json`
Verify the files and path in `cmsis_importer.json`

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

Could we document the cmsis_importer.json format?

tools/importer/cmsis_imorter.py Outdated
clone_cmd = ['git', 'clone', repo, "-b", branch, "--depth", '1', folder]
run_cmd(clone_cmd, exit_on_failure=True)
if __name__ == "__main__":

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

Can we add help so people don't need to open the file to figure out how to run it?

tools/importer/cmsis_imorter.py Outdated
clone_cmd = ['git', 'clone', repo, "-b", branch, "--depth", '1', folder]
run_cmd(clone_cmd, exit_on_failure=True)
if __name__ == "__main__":

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

I would assume this script doesn't clone anything, instead we run it as: python cimsis_importer.py <cmsis>

tools/importer/cmsis_imorter.py Outdated
copy_folder(cmsis_folder, mbed_path)
#Remove CMSIS Repo
remove_repo(CMSIS_REPO)

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

We don't want to clone it so we don't want to delete it.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Oct 31, 2017

Contributor

We need to commit CMSIS file changes before cherry-pick is performed. If we do not clean CMSIS repo then it will be part of commit. I am fine with not cloning and picking up as command line.
But if we clone we will have to remove it.
Also, as part of command line option it should be outside mbed-os repo.

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

Let's not clone it and add a line in docs mentioning it has to be outside.

tools/importer/cmsis_imorter.py Outdated
ROOT = abspath(join(dirname(__file__), "../.."))
CMSIS_REPO = "CMSIS_Repo"
CMSIS_PATH = abspath(join(dirname(__file__), CMSIS_REPO))
RTOS_UPDATE_BRANCH = "rtos_update"

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

can we make it more useful, something like feature_cmsis_rtx_{cmsis_sha_6char}

tools/importer/cmsis_imorter.py Outdated
add_files = ['git', 'add', '-A']
run_cmd(add_files, exit_on_failure=True)
commit_branch = ['git', 'commit', '-m', "CMSIS/RTX: Update CMSIS/RTX"]

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

can we add revision of CMSIS/RTX (6chars)

tools/importer/cmsis_imorter.py Outdated
clone_cmd = ['git', 'clone', repo, "-b", branch, "--depth", '1', folder]
run_cmd(clone_cmd, exit_on_failure=True)
if __name__ == "__main__":

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

Can we also have option to skip the first part and only do the cherry-picking. I'm assuming flow of run script -> fix conflicts -> re-run script to continue cherry picking.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Oct 31, 2017

Contributor

If we want to just cherry-pick than we can keep files/folder section as empty. I will have to fix few things for that (git commit and all will fail). Or do you want some command line option?

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

In first step I want to bring new RTX/CMSIS then the script will do the cherry pick but if it fails due to a conflict I want to be able to fix the conflict and continue, that is skip the first part and just continue with the patches.

tools/importer/cmsis_imorter.py Outdated
## Apply commits specific to mbed-os changes
mbed_sha = json_data["Mbed_sha"]
for sha in mbed_sha:

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

Can we make sure that handles case when given commit is already applied? Flow: run script -> cherry pick 1, 2, conflict -> fix -> re-run script -> cherry pick 4,5,6,...

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Oct 31, 2017

Contributor

Yes, i can compare the current SHA and proceed that way

@bulislaw bulislaw requested a review from theotherjimmy Oct 31, 2017

@deepikabhavnani deepikabhavnani requested review from sg- and c1728p9 Oct 31, 2017

@theotherjimmy

theotherjimmy requested changes Oct 31, 2017 edited

Can we rename the script to tools/importer/cmsis.py? I think tools/importer/cmsis_importer.py is a little redundant.

tools/importer/README.md Outdated
@@ -0,0 +1,6 @@
## Porting CMSIS/RTX Code in Mbed OS
This directory contains scripts to import latest CMSIS/RTX code into mbed-os local repository.

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

... to import the latest ...
... into the mbed-os local repository...

tools/importer/cmsis_imorter.py Outdated
if name in files:
result.append(os.path.join(root, name))
for file in result:
print os.path.relpath(file, ROOT)

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

Is this a debug print? could you take it out?

tools/importer/cmsis_imorter.py Outdated
print os.path.relpath(file, ROOT)
os.remove(file)
def rmtree(top):

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

Could you use the tools.utils.delete_dir_files?

tools/importer/cmsis_imorter.py Outdated
os.rmdir(os.path.join(root, name))
os.rmdir(top)
def copy_file(file, path):

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

Could you use tools.utils.mkdir followed by tools.utils.copy_file?

tools/importer/cmsis_imorter.py Outdated
abs_dst_file = os.path.join(path, file)
copy_file(abs_src_file, abs_dst_file)
def run_cmd(command, exit_on_failure=False):

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

Could you use tools.utils.run_cmd?

tools/importer/cmsis_imorter.py Outdated
return return_code
def remove_repo(folder):
os.chdir(abspath(dirname(__file__)))

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

This chdir is not followed by a move back or in a context manager. This may cause unintended side effects.

tools/importer/cmsis_imorter.py Outdated
folder - folder at which repo will be cloned
"""
remove_repo(folder)
clone_cmd = ['git', 'clone', repo, "-b", branch, "--depth", '1', folder]

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

Using a list with subprocess.call with shell=True does not work on Unix like platforms.

This comment has been minimized.

@adbridge

adbridge Oct 31, 2017

Contributor

I have found it is better to use shell=True but provide the command as a string instead. That seems to work across both Linux and Windows platforms

tools/importer/cmsis_imorter.py Outdated

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

Could you remove these extra newlines?

tools/importer/cmsis_importer.json Outdated
},
"files" : [
{
"cmsis_file" : "CMSIS/Core/Template/ARMv8-M/tz_context.c",

This comment has been minimized.

@theotherjimmy

theotherjimmy Oct 31, 2017

Contributor

This is both extremely verbose and slow. Instead this should be a map from cmsis_file to mbed_file.

For example:

"files" : {
    "CMSIS/Core/Template/ARMv8-M/tz_context.c": "platform/mbed_tz_context.c"
    ...
}
@sg-

Why is this done differently than mbedtls and uvisor? Are there plans to align the way they import?

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:importer_script branch Nov 1, 2017

deepikabhavnani added some commits Oct 30, 2017

Added arguments to importer script
Repository and json file will be mandotory inputs to script file.
As part of this change we do not need to clone/remove CMSIS repository

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:importer_script branch Nov 1, 2017

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Nov 7, 2017

@bulislaw @theotherjimmy - Please review

@theotherjimmy

Many nits.

tools/importer/README.md Outdated
"folders" : [
{
"src_folder" : "CMSIS/Core/Include/",
"src_folder" : "cmsis/TARGET_CORTEX_M/"

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

This is now allowed in JSON or python dictionaries. Perhaps you meant to have "dest_folder" here?

tools/importer/README.md Outdated
importer.py script can be used to import code base from different repositories into mbed-os.
### Pre-requisties
1. Get the `required` repository clone and update it to commit required. Note: Repository should be placed outside the mbed-os.

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

Why is required between ` here? How do I know which commit is required?

tools/importer/README.md Outdated
Note: Only files present in folder will be copied, directories inside the folder will not be copied.
`commit_sha` is list of commits present in mbed-os repo. These commits will be applied after copying files and folders listed above. Each commit is cherry-picked and applied sequentially in list with `-x` option, to record SHA of otriginal commit.

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

I would reword the third sentence as follows:

Each commit in the commit_sha list is cherry-picked and applied with the -x option, which records the SHA of the source commit in the commit message.

original is spelled incorrectly.

tools/importer/README.md Outdated
Note: Only files present in folder will be copied, directories inside the folder will not be copied.
`commit_sha` is list of commits present in mbed-os repo. These commits will be applied after copying files and folders listed above. Each commit is cherry-picked and applied sequentially in list with `-x` option, to record SHA of otriginal commit.
Note: In case of conflict, you will have to resolve the conflict and make sure appended statement "(cherry picked from commit …​)" is not deleted from commit message. Re-execute the python script to apply rest of the SHA commits.

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

I would reword this first sentence into two sentences. Such as:

You must resolve any conflicts that arise during this cherry-pick process. Make sure that the "(cherry picked from commit ...)" statement is present in the commit message.

tools/importer/README.md Outdated
@@ -0,0 +1,50 @@
## Porting various repositories into mbed-os

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

Given that the script is called "importer.py" I recommend titling this section "Importing repositories into mbed-os"

tools/importer/importer.py Outdated
rel_log = logging.getLogger("Importer")
if (args.repo_path is None) or (args.config_file is None) :
rel_log.error("Repository path and config file required as input. Use \"--help\" for more info.")

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

If these arguments are required, could you mark them as required in the argument parser? add_argument takes a required=True parameter.

tools/importer/importer.py Outdated
del_file(os.path.basename(cmsis_file))
for folder in data_folders:
print os.getcwd()

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

This looks like a debugging print. Could this be rel_log.debug instead?

tools/importer/importer.py Outdated
rel_log.debug("Copied = %s", mbed_path)
## Create new branch with all changes
create_branch = ['git', 'checkout', '-b', branch]

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

Could you convert this to a string as well?

tools/importer/importer.py Outdated
add_files = "git add -A"
run_cmd_with_output(add_files, exit_on_failure=True)
commit_branch = ['git', 'commit', '-m', commit_msg]

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

This command should be a string too.

tools/importer/importer.py Outdated
if not last_sha:
## Apply commits specific to mbed-os changes
for sha in commit_sha:
cherry_pick_sha = ['git', 'cherry-pick', '-x', sha]

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 7, 2017

Contributor

Could you convert this command to a string too?

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:importer_script branch Nov 7, 2017

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:importer_script branch to 43251e1 Nov 7, 2017

@0xc0170

Why is this done differently than mbedtls and uvisor? Are there plans to align the way they import?

@deepikabhavnani Can you please provide an answer? As the example states uvisor/mbedtls could be used. Has this been synced with them, if this would satisfy their needs (they use own makefile to prepare a release that are referenced above in the comment).

@@ -0,0 +1,252 @@
import os, json, stat, sys, shutil, errno, subprocess, logging, re

This comment has been minimized.

@0xc0170

0xc0170 Nov 8, 2017

Member

do not forget about the license in the new files

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Imports should be on separate per line. Ref: https://www.python.org/dev/peps/pep-0008/#imports
Also, several of them are not needed : re, errno, stat, shutil, basename imported from os.path, run_cmd imported from tools.utils

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Nov 8, 2017

@0xc0170 - Makefiles cannot be used for CMSIS/RTX as we have multiple patches/changes on top of original files. Suggestion was to use SHA for internal changes, instead of creating patch files.

Plan is to use this script for mbed-TLS and uVisior as well, but before that I want this to be fully functional for RTX/CMSIS.

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Nov 9, 2017

@bulislaw - Please review

@Nodraak

Hi @deepikabhavnani !

First of all, thank you for your contribution!

As a Python lover, I think mbed's Python code could be greatly enhanced. Doing a huge PR at once is error prone so I try to use other people's PRs when possible to improve the quality. I have made a few suggestions to correct bugs and/or make your code more Pythonic.

PS: I don't know if your are an experienced Python dev, but I suggest you read the PEP8 which is a set of code style rules and use tools such as Pylint to check your code style and spot basic errors. This will improve the readability of your code and prevent basic bugs.

@@ -0,0 +1,252 @@
import os, json, stat, sys, shutil, errno, subprocess, logging, re

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Imports should be on separate per line. Ref: https://www.python.org/dev/peps/pep-0008/#imports
Also, several of them are not needed : re, errno, stat, shutil, basename imported from os.path, run_cmd imported from tools.utils

result.append(os.path.join(root, name))
for file in result:
os.remove(file)
rel_log.debug("Deleted: %s", os.path.relpath(file, ROOT));

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Trailing semi-colon is unneeded

sha - last commit SHA
"""
cwd = os.getcwd()
sha = None

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

This line does not seems to be usefull

os.chdir(abspath(repo_path))
cmd = "git log --pretty=format:%h -n 1"
ret, sha = run_cmd_with_output(cmd, exit_on_failure=True)

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Since ret is unused, prefer the form _, sha = [...] which makes it more explicit

Returns:
True - If branch is already present
"""
branches = []

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

This variable seems unused

# Read configuration data
with open(json_file, 'r') as config:
json_data = json.load(config)

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Bad indentation, should be 8 spaces

## Remove all files listed in .json from mbed-os repo to avoid duplications
for file in data_files:
src_file = file['src_file']

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Exactly one space required after assignment

del_file(os.path.basename(src_file))
for folder in data_folders:
dest_folder = folder['dest_folder']

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Exactly one space required after assignment

## Copy all the CMSIS files listed in json file to mbed-os
for file in data_files:
repo_file = os.path.join(repo, file['src_file'])

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Exactly one space required after assignment (L208 and 209)

rel_log.debug("Copied = %s", mbed_path)
for folder in data_folders:
repo_folder = os.path.join(repo, folder['src_folder'])

This comment has been minimized.

@Nodraak

Nodraak Nov 12, 2017

Contributor

Exactly one space required after assignment (L215 and 216)

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Nov 13, 2017

@Nodraak - Thanks for review. I have update the script with follow PEP8 in future.

@bulislaw

It looks good, I'm just slightly afraid it may lead us to some hidden errors. If our config file is slightly out of date we may end up ignoring new files/directories, but not sure we can do much about it.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 14, 2017

@theotherjimmy @c1728p9 Can you review the PR?
We then can run CI for this patch

@theotherjimmy

Naming nits asside, I would like to see the possibility of error handled in the git checkout -b handled.

tools/importer/importer.py Outdated
cmd = "git checkout " + branch
run_cmd_with_output(cmd, exit_on_failure=False)
SHA = None

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 14, 2017

Contributor

In python, ALL CAPS variable names are generally reserved for global variables. Could you change the case of this variable to be lower case?

tools/importer/importer.py Outdated
for line in lines:
if 'cherry picked from' in line:
SHA = line.split(' ')[-1]
SHA = SHA[:-1]

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 14, 2017

Contributor

Could this be return SHA[:-1]? I think there is only ever 1 or 0 lines that match this filter, so taking the last one is the same as taking the first one.

tools/importer/importer.py Outdated
we will skip all file transfer and merge operations and will
jump to cherry-pick
'''
if check_branch(branch):

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 14, 2017

Contributor

super naming nit: I have no idea what check branch is checking for from it's name. From the definition, and the logging below, I was able to gather that it checks for the presence of a branch so I think it would be better named branch_exists.

tools/importer/importer.py Outdated
## Create new branch with all changes
create_branch = "git checkout -b "+ branch
run_cmd_with_output(create_branch, exit_on_failure=False)

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 14, 2017

Contributor

The return value is not checked, so maybe exit_on_failure should be True.

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:importer_script branch to d4f7291 Nov 15, 2017

@theotherjimmy

Looks good. Thanks.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Nov 17, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 17, 2017

Build : SUCCESS

Build number : 542
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5407/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Nov 20, 2017

@theotherjimmy theotherjimmy merged commit 08f3402 into ARMmbed:master Nov 20, 2017

6 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@deepikabhavnani deepikabhavnani deleted the deepikabhavnani:importer_script branch Nov 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment