Skip to content

Make Python scripts in build_defs compatible with Python 3.#452

Closed
philwo wants to merge 3 commits intobazelbuild:masterfrom
philwo:python3
Closed

Make Python scripts in build_defs compatible with Python 3.#452
philwo wants to merge 3 commits intobazelbuild:masterfrom
philwo:python3

Conversation

@philwo
Copy link
Copy Markdown
Member

@philwo philwo commented Nov 21, 2018

Some distributions like Arch Linux no longer ship Python 2 by default
and /usr/bin/python on these systems is a Python 3.x version, which
causes Bazel to try to run them with Python 3.x as part of the build.

With these changes, I was able to successfully build the plug-in on my
Arch Linux machine. I made sure that the changes are backwards
compatible with Python 2.x.

Some distributions like Arch Linux no longer ship Python 2 by default
and /usr/bin/python on these systems is a Python 3.x version, which
causes Bazel to try to run them with Python 3.x as part of the build.

With these changes, I was able to successfully build the plug-in on my
Arch Linux machine. I made sure that the changes are backwards
compatible with Python 2.x.
idea_plugin.appendChild(new_element)

print dom.toxml(encoding="utf-8")
print(dom.toxml())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI the encoding was there for a reason, please don't remove it. It caused errors when we didn't have it, I believe because we have non-ASCII characters in our changelogs.

The docs for 2.7 specifically say you should ALWAYS include it: https://docs.python.org/2.7/library/xml.dom.minidom.html#xml.dom.minidom.Node.toxml

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No problem. I had to remove this, because dom.toxml(encoding="utf-8") returns a byte-string, but print only accepts text-strings.

I'll add a commit that should restore the previous behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FYI, in merge_xml.py you already had a plain print(dom.toxml()) without a specified encoding - shall we keep this as is, or should I add a drive-by fix for that file as well? :)

@drichelson
Copy link
Copy Markdown

It looks like this should resolve #453

@philwo
Copy link
Copy Markdown
Member Author

philwo commented Nov 21, 2018

@drichelson Yes, I can confirm that was one of the errors I also saw on my machine. :)

@brendandouglas @plumpy I've imported the change internally in our code review system for review and merging.

@philwo
Copy link
Copy Markdown
Member Author

philwo commented Dec 8, 2018

Closing, because this has been merged and pushed as part of f054292 :)

@philwo philwo closed this Dec 8, 2018
@philwo philwo deleted the python3 branch December 8, 2018 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants