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

Additional fixes for running Python 3 in Windows #7092

Merged
merged 7 commits into from Jun 5, 2018

Conversation

Projects
None yet
4 participants
@cmonr
Contributor

cmonr commented Jun 3, 2018

Description

An additional set of small fixes for running Mbed OS and Mbed CLI functions within Windows.
Once #7078 is merged, this will be rebased to remove duplicate commits.

Pull request type

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

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Jun 4, 2018

cmonr added some commits May 31, 2018

Modified LazyDict to inherit from object instead of dict, and removed…
… iteration over values.

Py3 no longer supports dictionaries that self-modify their item lists during iteration.
Modified memap path separator parsing to support Py3.
Had to remove part of test that was incompatible with Py3 on Windows.
Modified IntelHex tofile parameter to use path.
Py3 open(...) returns a BufferedReader instead of a file.

@cmonr cmonr force-pushed the cmonr:py3-in-windows branch from 0ee7167 to cdbae99 Jun 4, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

/morph build

@@ -459,7 +459,7 @@ def merge_region_list(region_list, destination, notify, padding=b'\xFF'):
notify.info("Space used after regions merged: 0x%x" %
(merged.maxaddr() - merged.minaddr() + 1))
with open(destination, "wb+") as output:
merged.tofile(output, format=format.strip("."))
merged.tofile(destination, format=format.strip("."))

This comment has been minimized.

@bridadan

bridadan Jun 4, 2018

Contributor

This block doesn't make much sense. Did you mean to remove the open() call above? Currently, output isn't being used.

This comment has been minimized.

@cmonr

cmonr Jun 4, 2018

Contributor

Haha, you're right. I'm thinking that thewith open... could even be removed completely.

The tofile api happens to support passing the file path. The difficulty with this port was that In Py3, open returns a BufferedStream instead of a file, and I didn't have much luck finding something that worked in both versions.

API Ref: https://media.readthedocs.org/pdf/python-intelhex/latest/python-intelhex.pdf

This comment has been minimized.

@bridadan

bridadan Jun 4, 2018

Contributor

Ok no worries, sounds like removing the with open... statement is what you're after then.

@@ -14,6 +14,7 @@
# limitations under the License.
from __future__ import print_function, division, absolute_import
from past.builtins import basestring

This comment has been minimized.

@bridadan

bridadan Jun 4, 2018

Contributor

Doesn't look like this import is used anywhere

This comment has been minimized.

@cmonr

cmonr Jun 4, 2018

Contributor

It's not intuitive at all, but including basestring in this way affects how string concatenation occurs in line 77. Without it, Py3 defaults to treating strings as bytes, which breaks the concatenation.

This comment has been minimized.

@bridadan

bridadan Jun 4, 2018

Contributor

Ah I see, thanks for clarifying. I just checked the cheatsheet and I see what you're talking about: http://python-future.org/compatible_idioms.html#basestring

If you end up pushing more commits to this, do you think you could add a comment pointing that reference so others know why its there?

This comment has been minimized.

@cmonr

cmonr Jun 4, 2018

Contributor

Ah, yeah. Will do. There's been so much back and forth between testing across two separate OSs that I let commit quality suffer.

This comment has been minimized.

@bridadan

bridadan Jun 4, 2018

Contributor

Oh no its no big deal, its honestly fine how it is. Just trying to make it easier for future maintenance.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 4, 2018

Build : SUCCESS

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

Triggering tests

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

/morph build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

^^^ Ignore that. It's from the first build which was stopped due to the update.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 4, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr cmonr force-pushed the cmonr:py3-in-windows branch 2 times, most recently from 6b38f33 to f689ace Jun 4, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

Grumblegrumblegrumble......

Accidentally pushed a commit to this branch. Restarting CI after restoring the branch to it's approved commit history.

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 4, 2018

@bridadan @theotherjimmy Can I get one of y'all to approve?

@cmonr cmonr added the risk: G label Jun 4, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 4, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 80dde0b into ARMmbed:master Jun 5, 2018

14 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, 920 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9239 cycles (+289 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment