Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Py3 support #8181

Merged
merged 4 commits into from
Sep 20, 2018
Merged

Py3 support #8181

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
55336b0
Py3 support back. Reverted accidentially in icetea changes rebase
Sep 19, 2018
b8b11f0
Move exception in two lines that traceback looks better
Sep 19, 2018
f5ad414
Check that TEST_APPS folder exists
Sep 19, 2018
822fd37
Move exception earlier
Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tools/build_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1357,4 +1357,4 @@ def merge_build_data(filename, toolchain_report, app_type):
if 'type' not in build[0]:
build[0]['type'] = app_type
build_data['builds'].insert(0, build[0])
dump(build_data, open(filename, "wb"), indent=4, separators=(',', ': '))
dump(build_data, open(filename, "w"), indent=4, separators=(',', ': '))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this close file handle after dump ?

Copy link
Contributor Author

@OPpuolitaival OPpuolitaival Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how python close files but normally there is no problem even when not closing the files in code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more pythonic way would be like:

with open('data.json', 'w') as outfile:
    json.dump(data, outfile)

I little doubt that file handle stay open after dump call - normally if you open file with open() you need to take care to close it as well..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing @jupe's comment about using with open()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry. Typed too quickly. @jupe this is ok because the file handle is only opened within the context of the function call. Once the call is completed, the reference is garbage collected.

I agree that your suggestion is more pythonic and makes sure to close the file, but considering this is a fix and this is at the end of the file, I'm ok with this going in asis.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is true that when there is no references to file handle it will be closed automatically . But that is not the case when exceptions happens because then reference to file handle is stored to traceback object - part of stack trace and file will not be closed.. That could happen in some corner case - even unlikely. Anyway, it would be more safe to use with statement just in case..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Hadn't thought about what happens in the case of an exception.

I'm fine with seeing the change be made if @OPpuolitaival wants to make it. I was also under the impression that it would be good to get this in sooner rather than later, and since this seemed like a small change/improvement, I didn't want to hold up the PR.

9 changes: 6 additions & 3 deletions tools/run_icetea.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,15 @@ def icetea_tests(target, tcdir, verbose):
command = ['icetea', '--tcdir', tcdir, '--list', '--json', '--platform_filter', target] \
+ (['-v'] if verbose else [])

if not os.path.exists(tcdir):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be before command variable definition? it would be more readable..

raise Exception("Icetea run error: No TEST_APPS folder in {}".format(os.path.curdir))

stdout, stderr, returncode = run_cmd(command)

if returncode != 0:
raise Exception(
"Error when running icetea. \ncwd:{} \nCommand:'{}' \noutput:{}".format(os.getcwd(), ' '.join(command),
stderr.decode()))
additional_information = "\ncwd:{} \nCommand:'{}' \noutput:{}".format(os.getcwd(), ' '.join(command),
stderr.decode())
raise Exception("Error when running icetea. {}".format(additional_information))

return json.loads(stdout)

Expand Down