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/utils: Fix issue with loading json files as ascii on python3 linux #8177

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
5 participants
@andrewleech
Contributor

andrewleech commented Sep 19, 2018

Description

This resolves #7026, a failure to run mbed-cli tool on linux python 3.5 due to the error:

Traceback (most recent call last):
  File "/project/mbed-os/tools/make.py", line 41, in <module>
    from tools.tests import TESTS, Test, TEST_MAP
  File "/project/mbed-os/tools/tests.py", line 18, in <module>
    from tools.data.support import DEFAULT_SUPPORT, CORTEX_ARM_SUPPORT
  File "/project/mbed-os/tools/data/support.py", line 17, in <module>
    from tools.targets import TARGETS
  File "/project/mbed-os/tools/targets/__init__.py", line 569, in <module>
    update_target_data()
  File "/project/mbed-os/tools/targets/__init__.py", line 558, in update_target_data
    in Target.get_json_target_data().items()
  File "/project/mbed-os/tools/targets/__init__.py", line 87, in wrapper
    CACHES[(func.__name__, args)] = func(*args, **kwargs)
  File "/project/mbed-os/tools/targets/__init__.py", line 159, in get_json_target_data
    Target.__targets_json_location_default)
  File "/project/mbed-os/tools/utils.py", line 372, in json_file_to_dict
    object_pairs_hook=OrderedDict)
  File "/usr/lib/python3.5/json/__init__.py", line 312, in loads
    s.__class__.__name__))
TypeError: the JSON object must be str, not 'bytes'

This problem was accidentally introduced in 7b49edc when the recursive function which called str.encode('ascii').decode() was essentially replaced with a one liner which calls str.encode('ascii', 'ignore') without any conversion back to string. It appears some versions / platforms of python handle bytes being passed into json.loads while others don't (it's supposed to take str)

This method is also somewhat inefficient however (though still better than the recursive function). Reading in the file and converting a couple of times before sending into json in unneccesary, the builtin codecs module gives us a much cleaner way of achieving the same goal

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-tools Sep 19, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 20, 2018

Ooh, nice!

@andrewleech Isn't the use of the codec module deprecated?
https://bugs.python.org/issue8796

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Sep 21, 2018

@cmonr good call, looks like I should be using the matching method from io

@andrewleech andrewleech force-pushed the andrewleech:tools_json_python3 branch from ec7e176 to b663c5d Sep 21, 2018

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Sep 21, 2018

Well that was simple. Turns out io.open is an alias for builtin open in current python3, however it is different in python2 so worth explicitly declaring as such.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 26, 2018

Gonna test this against all other Python versions we support tomorrow, but assuming nothing bad is found, will start CI on it.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Oct 3, 2018

Any updates?

@andrewleech andrewleech force-pushed the andrewleech:tools_json_python3 branch from b663c5d to 41ff7b9 Oct 3, 2018

@SenRamakri SenRamakri requested a review from cmonr Oct 5, 2018

@cmonr

cmonr approved these changes Oct 8, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 8, 2018

@andrewleech My bad. Started looking into this, and found more issues that needed to be fixed, right before I left for a week.

Let's get this in to unblock you, and I'll continue working on those other issues.

@cmonr cmonr added needs: CI and removed needs: review labels Oct 8, 2018

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Oct 8, 2018

Sounds good!

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 8, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 9, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 9, 2018

/morph mbed2-build

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 9, 2018

@cmonr cmonr merged commit be4bb9f into ARMmbed:master Oct 10, 2018

14 checks passed

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
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 626 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9870 cycles (-342 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 8372B (+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