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

deepikabhavnani · 2017-10-30T22:22:37Z

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.

bulislaw · 2017-10-31T10:50:54Z

tools/importer/cmsis_importer.json

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

bulislaw · 2017-10-31T10:52:53Z

tools/importer/cmsis_importer.json

Can we have better name for that? commit_sha?

bulislaw · 2017-10-31T10:53:33Z

tools/importer/README.md

Could we document the cmsis_importer.json format?

bulislaw · 2017-10-31T10:54:26Z

tools/importer/cmsis_imorter.py

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

bulislaw · 2017-10-31T10:56:16Z

tools/importer/cmsis_imorter.py

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

bulislaw · 2017-10-31T11:01:28Z

tools/importer/cmsis_imorter.py

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

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.

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

bulislaw · 2017-10-31T11:02:41Z

tools/importer/cmsis_imorter.py

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

bulislaw · 2017-10-31T11:03:17Z

tools/importer/cmsis_imorter.py

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

bulislaw · 2017-10-31T11:05:34Z

tools/importer/cmsis_imorter.py

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.

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?

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.

bulislaw · 2017-10-31T11:07:02Z

tools/importer/cmsis_imorter.py

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,...

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

theotherjimmy

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

theotherjimmy · 2017-10-31T14:12:21Z

tools/importer/README.md

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

theotherjimmy · 2017-10-31T14:14:27Z

tools/importer/cmsis_imorter.py

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

theotherjimmy · 2017-10-31T14:30:45Z

tools/importer/cmsis_imorter.py

Could you use the tools.utils.delete_dir_files?

theotherjimmy · 2017-10-31T14:31:48Z

tools/importer/cmsis_imorter.py

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

theotherjimmy · 2017-10-31T14:32:22Z

tools/importer/cmsis_imorter.py

Could you use tools.utils.run_cmd?

theotherjimmy · 2017-10-31T14:33:21Z

tools/importer/cmsis_imorter.py

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

theotherjimmy · 2017-10-31T14:33:59Z

tools/importer/cmsis_imorter.py

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

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

theotherjimmy · 2017-10-31T15:48:33Z

tools/importer/cmsis_imorter.py

Could you remove these extra newlines?

theotherjimmy · 2017-10-31T15:50:13Z

tools/importer/cmsis_importer.json

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?

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 · 2017-11-07T17:08:25Z

@bulislaw @theotherjimmy - Please review

theotherjimmy

Many nits.

theotherjimmy · 2017-11-07T17:10:08Z

tools/importer/README.md

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

theotherjimmy · 2017-11-07T17:11:01Z

tools/importer/README.md

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

theotherjimmy · 2017-11-07T17:16:00Z

tools/importer/README.md

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.

theotherjimmy · 2017-11-07T17:18:29Z

tools/importer/README.md

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.

theotherjimmy · 2017-11-07T17:19:20Z

tools/importer/README.md

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

theotherjimmy · 2017-11-07T17:39:08Z

tools/importer/importer.py

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

theotherjimmy · 2017-11-07T17:40:23Z

tools/importer/importer.py

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

theotherjimmy · 2017-11-07T17:41:33Z

tools/importer/importer.py

Could you convert this to a string as well?

theotherjimmy · 2017-11-07T17:41:46Z

tools/importer/importer.py

This command should be a string too.

theotherjimmy · 2017-11-07T17:42:04Z

tools/importer/importer.py

Could you convert this command to a string too?

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).

0xc0170 · 2017-11-08T11:24:20Z

tools/importer/importer.py

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


do not forget about the license in the new files

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 · 2017-11-08T19:08:24Z

@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 · 2017-11-09T15:32:30Z

@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.

Nodraak · 2017-11-12T18:09:44Z

tools/importer/importer.py

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


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

Nodraak · 2017-11-12T18:13:52Z

tools/importer/importer.py

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


Trailing semi-colon is unneeded

Nodraak · 2017-11-12T18:16:21Z

tools/importer/importer.py

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


This line does not seems to be usefull

Nodraak · 2017-11-12T18:17:04Z

tools/importer/importer.py

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


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

Nodraak · 2017-11-12T18:18:18Z

tools/importer/importer.py

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


This variable seems unused

Nodraak · 2017-11-12T18:24:38Z

tools/importer/importer.py

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


Bad indentation, should be 8 spaces

Nodraak · 2017-11-12T18:24:51Z

tools/importer/importer.py

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


Exactly one space required after assignment

Nodraak · 2017-11-12T18:25:00Z

tools/importer/importer.py

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


Exactly one space required after assignment

Nodraak · 2017-11-12T18:25:11Z

tools/importer/importer.py

+
+        ## 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'])


Exactly one space required after assignment (L208 and 209)

Nodraak · 2017-11-12T18:25:24Z

tools/importer/importer.py

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


Exactly one space required after assignment (L215 and 216)

deepikabhavnani · 2017-11-13T22:03:15Z

@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 · 2017-11-14T14:54:14Z

@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.

theotherjimmy · 2017-11-14T14:58:48Z

tools/importer/importer.py

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

theotherjimmy · 2017-11-14T15:00:15Z

tools/importer/importer.py

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.

theotherjimmy · 2017-11-14T15:04:56Z

tools/importer/importer.py

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.

theotherjimmy · 2017-11-14T15:07:26Z

tools/importer/importer.py

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

https://www.python.org/dev/peps/pep-0008/

theotherjimmy

Looks good. Thanks.

0xc0170 · 2017-11-17T10:43:49Z

/morph build

mbed-ci · 2017-11-17T11:18:18Z

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 · 2017-11-17T12:19:27Z

Test : SUCCESS

Build number : 352
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5407/352

mbed-ci · 2017-11-17T13:12:38Z

Exporter Build : SUCCESS

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

bulislaw suggested changes Oct 31, 2017

View reviewed changes

bulislaw requested a review from theotherjimmy October 31, 2017 11:08

deepikabhavnani requested review from c1728p9 and sg- October 31, 2017 14:34

theotherjimmy suggested changes Oct 31, 2017

View reviewed changes

sg- reviewed Oct 31, 2017

View reviewed changes

adbridge added the needs: work label Oct 31, 2017

deepikabhavnani force-pushed the importer_script branch from 34692d9 to 99b07bb Compare November 1, 2017 22:04

deepikabhavnani added 3 commits November 1, 2017 17:05

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

021caa6

Renamed importer script file

78fdb7b

Added arguments to importer script

d4e6dec

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 force-pushed the importer_script branch from 99b07bb to 40c8750 Compare November 1, 2017 22:06

theotherjimmy added needs: review and removed needs: work labels Nov 6, 2017

theotherjimmy suggested changes Nov 7, 2017

View reviewed changes

deepikabhavnani force-pushed the importer_script branch from c7a3b6e to 4672f8e Compare November 7, 2017 19:14

Updated the script to be generic for any repo

43251e1

deepikabhavnani force-pushed the importer_script branch from 4672f8e to 43251e1 Compare November 7, 2017 19:17

0xc0170 reviewed Nov 8, 2017

View reviewed changes

Nodraak suggested changes Nov 12, 2017

View reviewed changes

bulislaw approved these changes Nov 14, 2017

View reviewed changes

theotherjimmy suggested changes Nov 14, 2017

View reviewed changes

Deepika added 2 commits November 15, 2017 11:30

Updated code as per Python style guide

51430cd

https://www.python.org/dev/peps/pep-0008/

Corrected the input to remove command

d4f7291

deepikabhavnani force-pushed the importer_script branch from daa7689 to d4f7291 Compare November 15, 2017 17:31

theotherjimmy approved these changes Nov 16, 2017

View reviewed changes

theotherjimmy added needs: CI and removed needs: review labels Nov 16, 2017

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

theotherjimmy added the release-version: 5.7.0-rc1 label Nov 20, 2017

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

theotherjimmy removed the ready for merge label Nov 20, 2017

deepikabhavnani deleted the importer_script branch November 20, 2017 17:54

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

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

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

Uh oh!

Conversation

deepikabhavnani commented Oct 30, 2017

Uh oh!

bulislaw 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!

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!

theotherjimmy 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!

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!

sg- left a comment

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani commented Nov 7, 2017

Uh oh!

theotherjimmy 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

theotherjimmy left a comment •

edited

Loading

deepikabhavnani commented Nov 8, 2017 •

edited

Loading