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

tools: Refactor notification API #6781

Merged
merged 20 commits into from May 7, 2018

Conversation

Projects
None yet
6 participants
@theotherjimmy
Contributor

theotherjimmy commented Apr 30, 2018

Description

Historically, the notification API is coupled to an mbedToolchain instance.
This is problematic for things that could use notification without a toolchain
object, such a export zipping, or post build merge scripts, and this is a poor
coupling of the API that requires passing many, many parameters through to the
toolchain, such as verbose and silent.

This PR decouples the notification API from the toolchain object, and delegates
implementaion of this Notifier API to a single implementation:
TerminalNotifier. In testing this PR I prototyped another implementation as
part of the Online Compiler's build back end.

This new API allowed me to decouple the verbosity parameter handling, -v and
--silent, from the build API entirely. They are now completely oblivious to
these parameters, instead calling methods on the Notifier object that they
are passed.

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 30, 2018

TODO for tomorrow theotherjimmy: Fix those dang tools tests.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 1, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 2, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 2, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 2, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 3, 2018

Aborted again, but not by us, tests are failing and then results in timeout? Please review

@0xc0170 0xc0170 added the needs: work label May 3, 2018

@0xc0170 0xc0170 removed the needs: CI label May 3, 2018

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 3, 2018

@0xc0170 Yeah, it looks like IAR everything is failing for some targets.

Use mocked notifier for individual tests
That way we separate the collection of notifications from everything else
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 3, 2018

@0xc0170 I got the bug, I think. The test builds were sharing a notifier, which basically squared the amount of output we got.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 3, 2018

/morph build

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels May 3, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented May 3, 2018

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 3, 2018

/morph build

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 3, 2018

So the prior abort was caused by a combination of a few things leading up to the test builder consuming an unbounded (and very, very large) amount of RAM. That commit seems to keep it capped at a little less than 400MB on my machine. At particularly bad spots, the prior commit could consume >2GB RAM on my machine.

Reduce memory consumption and lock contension
29% speedup (old: 45sec, new: 35sec) on my machine

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:refactor-notify branch from 79b7e7e to 81f969e May 3, 2018

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 3, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 4, 2018

@theotherjimmy Can you review the jenkins CI failure, there's a build error

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 4, 2018

@0xc0170 Jenkins passed.

@mbed-ci

This comment has been minimized.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 4, 2018

/morph test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 requested a review from screamerbg May 7, 2018

@screamerbg

Quite a large change set, but for all the important things. Thanks, @theotherjimmy!

@0xc0170

0xc0170 approved these changes May 7, 2018

@cmonr cmonr merged commit 53d3c43 into ARMmbed:master May 7, 2018

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build 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
travis-ci/astyle Passed, 582 warnings (+0 warnings)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9183 cycles (+276 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the needs: CI label May 7, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented May 15, 2018

Hi @theotherjimmy

Since this PR merge, build python script is not working any more :-(

$ python tools/build.py -m NUCLEO_F030R8 -t ARM
name 'TerminalNotifer' is not defined

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 15, 2018

@jeromecoutant Thanks for reporting. we will look at this

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