simonowen / pyz80

pyz80 - a Z80 cross assembler
GNU General Public License v2.0
18 stars 8 forks source link

Shebang permits Python 2; code is not compatible with, at least, Python 2.7.16 #30

Closed TomHarte closed 1 week ago

TomHarte commented 2 weeks ago

I'm no Python expert but, on those version of macOS I tested — 11 and 14 — the existing shebang of #!/usr/bin/env python causes the included version of Python 2 to launch.

That then fails with:

Traceback (most recent call last):
  File "/Users/thomasharte/.vscode/extensions/simonowen.pyz80-1.4.2/lib/pyz80.py", line 1892, in <module>
    save_file_to_image(image,pathname)
  File "/Users/thomasharte/.vscode/extensions/simonowen.pyz80-1.4.2/lib/pyz80.py", line 308, in save_file_to_image
    add_file_to_disk_image(image,sam_filename, 1, 0, fromfile=pathname)
  File "/Users/thomasharte/.vscode/extensions/simonowen.pyz80-1.4.2/lib/pyz80.py", line 140, in add_file_to_disk_image
    image[dirpos+11] = nsectors // 256 # MSB number of sectors used
TypeError: integer argument expected, got float

The relevant segment of code is:

    nsectors = math.ceil(( filelength + 9 ) / 510)
    image[dirpos+11] = nsectors // 256 # MSB number of sectors used

Which makes sense because per w3schools on math.ceil:

> Python 3+ : Returns an int value
> Python 2.x : Returns a float value.

I'm guessing that the code is correct but that #!/usr/bin/env python3 is the appropriate shebang, though I thought maybe you'd prefer to add a type conversion there (and possibly elsewhere?) to retain Python 2 compatibility so I haven't provided a pull request.

Noting that this also causes your VS Code pyz80 plugin not to function out of the box on macOS.

simonowen commented 1 week ago

Thanks for reporting this! It looks like this was introduced in a55f492 with the switch to math.ceil last year. That's probably after I last tested the VSCode extension on macOS so I've not run into it myself.

In checking this I've also noticed it also adds a second Python 2.x bug, where the sector count is incorrectly rounded down due to other differences between Python 2 and 3:

Python 2.x:

>>> math.ceil(256 / 510)
0.0
>>> math.ceil(256 / 510.0)
1.0

Python 3.x:

>>> math.ceil(256 / 510)
1
>>> math.ceil(256 / 510.0)
1

Division of two integers in Python 2 gives an integer result, which rounds down before it's given to ceil. So the sector count is always rounded down instead of up, as well as being the wrong type!

I could fix the code with some extra casts but I think it's better to drop Python 2 support to avoid this kind of subtle issue. It hasn't been officially supported for years and everyone should have a Python 3 interpreter now -- even Windows 10 bundles it. I'll update the shebang line to python3 and release a new VSCode extension that includes it.

simonowen commented 1 week ago

I forgot to update the changelog in the extension before pushing it, but hopefully v1.4.3 is fixed for you now. :)