Add new script to automate compilation of all mbed-os 2 targets in online IDE #3818

adbridge · 2017-02-21T15:44:50Z

No description provided.

Usage: python tools/check_release.py Run from the mbed-os directory. For details on how the script works and how to set up the configuration json file, please see the python file header.

adbridge · 2017-02-21T15:45:46Z

@bridadan @theotherjimmy please review.

theotherjimmy

Looks fine. Some stuff seems a bit silly though.

theotherjimmy · 2017-02-21T18:06:48Z

tools/check_release.json

@@ -0,0 +1,16 @@
+{
+  "config" : {
+    "mbed_repo_path" : "C:/Users/annbri01/Work/Mercurial"


Can we not check it stuff related to only your computer?

The point of the config is it allows any path / user to use the script. So by modifying the version of this it could run in CI or in a general account.

theotherjimmy · 2017-02-21T18:09:23Z

tools/check_release.py

+
+OFFICIAL_MBED_LIBRARY_BUILD = get_mbed_official_release('2')
+
+def log_message(str):


Can't we just import logging?

Will look at that, now that I know we have a logging module...

It's built into python.

theotherjimmy · 2017-02-21T18:10:02Z

tools/check_release.py

+    for m in messages:
+        if 'severity' in m and 'message' in m:
+            if m['severity'] == 'error':
+                return m['message']


There may be more than one error message, this just gets the first one.

That should be fine for our purposes. We only really want to know that the compilation failed and what the first compilation error was.

theotherjimmy · 2017-02-21T18:11:37Z

tools/check_release.py

+    fail_type = None
+
+    # poll for output
+    for check in range(0, polls):


You can safely drop the 0

theotherjimmy · 2017-02-21T18:12:21Z

tools/check_release.py

+            # 3) Internal failure of the online compiler            
+            result = bool(response['result']['data']['compilation_success'])
+            if result:
+                sys.stdout.write("SUCCESSFUL \n")


Why is this not flushed?

Should be, will add.

theotherjimmy · 2017-02-21T18:17:39Z

tools/check_release.py

+
+    # get password
+    cmd = ['hg', 'config', 'auth.x.password']
+    ret, output = run_cmd_with_output(cmd, exit_on_failure=True)


theotherjimmy · 2017-02-21T18:18:18Z

tools/check_release.py

+        exit(1)
+
+    print("Latest mbed lib version = %s" % mbed)    
+    print("Latest mbed-dev lib version = %s" % mbed_dev)    


More logging here.

theotherjimmy · 2017-02-21T18:18:34Z

tools/check_release.py

+
+    # Compile each test for each supported target
+    for test in tests:
+        print("Test compiling program: %s\n" % test)


Logging here too please.

theotherjimmy · 2017-02-21T18:18:54Z

tools/check_release.py

+                if not result:
+                    if mesg == 'Internal':
+                        # Internal compiler error thus retry
+                        sys.stdout.write("FAILURE \n")


theotherjimmy · 2017-02-21T18:19:01Z

tools/check_release.py

+                        continue
+                    else:
+                        # Genuine compilation error, thus print it out
+                        print("\t\tError: %s\n" % mesg)


bridadan

Mostly just agree with @theotherjimmy, with one thing that should be changed when getting the list of mbed 2 targets. Everything else looks good though!

bridadan · 2017-02-21T21:21:12Z

tools/check_release.py

+
+    # send task to api
+    log_message(url + begin + "| data: " + str(payload))
+    r = requests.post(url + begin, data=payload, auth=auth)


This shouldn't hold up this PR, but you may find the urljoin function useful in this case: https://docs.python.org/2/library/urlparse.html#urlparse.urljoin

@bridadan that looks like a useful module but I'm wondering if it is overkill for this one use case....

I saw you use similar logic around the script so I thought I'd mention it if you get tired of doing URL concatenation yourself, but yeah no worries if you just want to stick with this.

It's actually the remnants of Sarah's original script and I didn't look too closely at it until you mentioned it !

bridadan · 2017-02-21T21:49:45Z

tools/check_release.py

+
+    # Get a list of the officially supported mbed-os 2 targets
+    for tgt in OFFICIAL_MBED_LIBRARY_BUILD:
+        if 'ARM' in tgt[1]:


I don't think is a necessary check as the tools already do this for you when you call get_mbed_official_release('2'). And in fact, it's incorrect because it excludes the one mbed 2 target that only supports uARM: https://github.com/ARMmbed/mbed-os/blob/master/targets/targets.json#L989

@bridadan I thought the online IDE only supported the ARM compiler ?

uARM is the ARM compiler, it just changes the standard library to a size optimized version (micro lib)

ah ok I just wasn't sure if the online compiler supported that. I will remove the offending IF .

Replace use of print and write with logging (with different levels). Add the ability to choose the logging level on the command line. Fix the exclusion of uARM from compilation. Remove print_on_fail from run_cmd() functions, now use logging.

adbridge · 2017-02-23T17:54:20Z

@theotherjimmy @bridadan please check review comments

theotherjimmy

I like the use of logging. We should look into using the same logging module in the rest of the tools.

theotherjimmy · 2017-02-23T17:57:45Z

tools/check_release.py

+    logging.debug(r.request.body)
+
+    if r.status_code != 200:
+        raise Exception("Error while talking to the mbed API")


Can this exception be more specific? "Error while talking to the mbed API" is a bit vague

That's from Sarah's original script and as I haven't actually seen that hit, I'm not sure what the actual exception is. Http code 200 means " the action requested by the client was received, understood, accepted, and processed successfully" , so not 200 means the request failed to be one of those options , ie a general communication with the API error.

Well, the exception could include the error code it got back.

I think including the status code would be helpful in this case.

theotherjimmy · 2017-02-23T17:59:59Z

tools/check_release.py

+
+        os.rename(lib_file, bak_file)
+    else:
+        logging.error("!! Error trying to backup lib file prior to updating.")


Nit: Can we get rid of the "!! Error"? I think marking it as an error should be good enough.

We could but it's not worth blocking the PR for at this stage

That's why it's approved.

theotherjimmy · 2017-02-23T18:01:50Z

tools/check_release.py

+            cmd = ['hg', 'push', '-f', repo]
+            run_cmd(cmd, exit_on_failure=True)
+
+        except:


Bare exception. Could we specialize this to use a particular Exception type?

Tricky as the documentation for client.rawcommand() is sparse to say the least and certainly doesn't specify what exceptions are raised...

That's a good point.

theotherjimmy · 2017-02-23T18:04:04Z

tools/check_release.py

+if __name__ == '__main__':
+
+    parser = argparse.ArgumentParser(description=__doc__,
+                                     formatter_class=argparse.RawDescriptionHelpFormatter)


This line is really long. I don't know how we could make it shorter other than to put a newline immediatly after the (. This is a style thing, so feel free to ignore it.

theotherjimmy · 2017-02-23T18:04:33Z

tools/check_release.py

+
+    parser = argparse.ArgumentParser(description=__doc__,
+                                     formatter_class=argparse.RawDescriptionHelpFormatter)
+    parser.add_argument('-l', '--log-level', help="Level for providing logging output", default='INFO')


This line is also really long, but making it shorter should be pretty simple.

I think the line length things again aren't worth blocking the PR for. If I need to do some more work on this script in the future I'll tidy these minor stylistic things up.

👍 That's why I approved the changes. style things should not block it at this stage.

theotherjimmy · 2017-02-23T18:05:18Z

tools/check_release.py

+    logging.basicConfig(level=level)
+
+    # Read configuration data
+    json_data = json.load(open(os.path.join(os.path.dirname(__file__), "check_release.json")))


Another long line. maybe a newline after that comma?

bridadan

LGTM

bridadan

Looks good!

theotherjimmy

Nice!

adbridge added 2 commits February 21, 2017 15:33

New script for automating the compilation of mbed-os 2 targets.

42f9861

Usage: python tools/check_release.py Run from the mbed-os directory. For details on how the script works and how to set up the configuration json file, please see the python file header.

Fix indentation level to be consistent.

251ef81

adbridge requested review from theotherjimmy and bridadan February 21, 2017 15:45

adbridge added the needs: review label Feb 21, 2017

theotherjimmy reviewed Feb 21, 2017

View reviewed changes

theotherjimmy approved these changes Feb 21, 2017

View reviewed changes

bridadan suggested changes Feb 21, 2017

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Feb 23, 2017

sg- added the release-version: 5.4.0 label Feb 23, 2017

Address review comments

e95092c

Replace use of print and write with logging (with different levels). Add the ability to choose the logging level on the command line. Fix the exclusion of uARM from compilation. Remove print_on_fail from run_cmd() functions, now use logging.

theotherjimmy approved these changes Feb 23, 2017

View reviewed changes

bridadan approved these changes Feb 23, 2017

View reviewed changes

Removed unecessary exception and replaced with a logging error.

415486e

bridadan approved these changes Feb 23, 2017

View reviewed changes

theotherjimmy approved these changes Feb 23, 2017

View reviewed changes

sg- added needs: CI ready for merge and removed needs: work needs: CI labels Feb 23, 2017

sg- merged commit 2e5f2e9 into ARMmbed:master Feb 23, 2017

sg- removed the ready for merge label Feb 23, 2017


		OFFICIAL_MBED_LIBRARY_BUILD = get_mbed_official_release('2')

		def log_message(str):

Add new script to automate compilation of all mbed-os 2 targets in online IDE #3818

Add new script to automate compilation of all mbed-os 2 targets in online IDE #3818

Uh oh!

Conversation

adbridge commented Feb 21, 2017

Uh oh!

adbridge commented Feb 21, 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

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!

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

adbridge commented Feb 23, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment