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

Improve Windows compatibility #134

Closed
wants to merge 3 commits into from
Closed

Improve Windows compatibility #134

wants to merge 3 commits into from

Conversation

ydylla
Copy link

@ydylla ydylla commented Dec 3, 2022

Hi,
this improves the Windows compatibility of PeachPy.
In particular, it fixes #121. The problem was under Windows the default encoding for open() is cp1252 instead of utf-8. So by always using utf-8 for the TextWriter it is now possible to generate Go asm under Windows.

The second commit deals with the problems described in PR #43. Under Windows it is not possible to unlink an open file. With this the writers close the file before attempting to unlink it. Just like @bawr described in his last comment but apparently never did.

Closes #121, closes #43

@ydylla
always use utf-8 when writing files
ca368ae 
Required for windows since there the default encoding is cp1252, which causes #121
@ydylla
close files before unlinking
cb5d697 
improves compatibility with windows, which can not delete open files
Maratyszcza
Maratyszcza approved these changes Dec 3, 2022
View reviewed changes
Copy link
Owner

@Maratyszcza Maratyszcza left a comment

Choose a reason for hiding this comment

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

Thank you!

Maratyszcza
Maratyszcza requested changes Dec 3, 2022
View reviewed changes
Copy link
Owner

@Maratyszcza Maratyszcza left a comment

Choose a reason for hiding this comment

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

Replace open with codecs.open for Python 2.7 compatibility

@ydylla
Copy link
Author

ydylla commented Dec 3, 2022

Did not know python 2 does not have the encoding parameter 😄
I also found io.open which I think is nicer than codecs.open because in Python 3 it's just an alias.

@ydylla
Copy link
Author

ydylla commented Dec 3, 2022 

File "codegen\x86_64.py", line 1152, in main
        print(str(code), file=out)
    TypeError: write() argument 1 must be unicode, not str

Well looks like I have to actually install Python 2.7 again and really test this 🙃

@ydylla
Copy link
Author

ydylla commented Dec 4, 2022

Hmm I am out of ideas (and time) how to solve this correctly.
Just switching to codecs.open makes the tests work for all python versions. But go asm would still be broken for python 2.7 (apparently there is no test that covers this).

When executing the go example with Python 2.7 and codecs.open I get this:

(venv) G:\Code\PeachPy\examples\go-generate>python -m peachpy.x86_64 dot_product.py -S -o dot_product_amd64.s -mabi=goasm
Traceback (most recent call last):
  File "C:\Python27\lib\runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "C:\Python27\lib\runpy.py", line 72, in _run_code
    exec code in run_globals
  File "g:\code\peachpy\peachpy\x86_64\__main__.py", line 283, in <module>
    main()
  File "g:\code\peachpy\peachpy\x86_64\__main__.py", line 270, in main
    execute_script(writers, options.input[0])
  File "g:\code\peachpy\peachpy\x86_64\__main__.py", line 201, in execute_script
    execute_script(writers, source_filename)
  File "g:\code\peachpy\peachpy\writer.py", line 27, in __exit__
    self.output_file.write(self.serialize())
  File "C:\Python27\lib\codecs.py", line 708, in write
    return self.writer.write(data)
  File "C:\Python27\lib\codecs.py", line 369, in write
    data, consumed = self.encode(object, self.errors)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 143: ordinal not in range(128)

and with io.open this (same as in the pipeline):

(venv) G:\Code\PeachPy\examples\go-generate>python -m peachpy.x86_64 dot_product.py -S -o dot_product_amd64.s -mabi=goasm
Traceback (most recent call last):
  File "C:\Python27\lib\runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "C:\Python27\lib\runpy.py", line 72, in _run_code
    exec code in run_globals
  File "g:\code\peachpy\peachpy\x86_64\__main__.py", line 283, in <module>
    main()
  File "g:\code\peachpy\peachpy\x86_64\__main__.py", line 270, in main
    execute_script(writers, options.input[0])
  File "g:\code\peachpy\peachpy\x86_64\__main__.py", line 201, in execute_script
    execute_script(writers, source_filename)
  File "g:\code\peachpy\peachpy\writer.py", line 27, in __exit__
    self.output_file.write(self.serialize())
TypeError: write() argument 1 must be unicode, not str

The io.open one can be fixed by adding decode before writing. But this is python 2.7 specific and gets rather ugly since it has to happen before each write (probably in various places):

content = self.serialize()
if sys.version_info.major == 2:
    content = content.decode("utf-8")
self.output_file.write(content)

So which version do you prefer? Using codecs.open and wait until someone actually complains that go asm with python 2.7 does not work. Or using io.open with sys.version_info.major == 2 in various places?
Or maybe you can think of a better solution.

@ydylla
Copy link
Author

ydylla commented Jan 29, 2023

I will close this because I do not intend to spend time on fixing python2 support.
Especially since I also switched to https://github.com/mmcloughlin/avo for generating go assembly, which just works under windows.

@ydylla ydylla closed this Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Maratyszcza Maratyszcza Maratyszcza requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

go-generate: invalid UTF-8 encoding
2 participants
@ydylla @Maratyszcza