clalancette / pycdlib

Python library to read and write ISOs
GNU Lesser General Public License v2.1
147 stars 38 forks source link

1.13.0 claims to be compatible with Python 2.7, while it is not #95

Closed felixfontein closed 2 years ago

felixfontein commented 2 years ago

https://github.com/clalancette/pycdlib/blob/master/setup.py#L52 says that this library is compatible with Python 2.7.

You probably want to put something like python_requires='>=3.4.0', (or whatever the exact required minimum Python version is) into setup.py.

clalancette commented 2 years ago

I did not intentionally drop Python 2.7 support, and it seems to work here in my testing. Can you please let me know what is not working for you?

felixfontein commented 2 years ago

The code uses f-strings, which are Python 3.6+ only, see for example https://github.com/clalancette/pycdlib/blob/master/pycdlib/utils.py#L483.

I saw the following stacktrace in CI with Python 2.7:

  File "/usr/lib/python2.7/site-packages/pycdlib/__init__.py", line 5, in <module>
    from .pycdlib import PyCdlib  # NOQA
  File "/usr/lib/python2.7/site-packages/pycdlib/pycdlib.py", line 38, in <module>
    from pycdlib import dr
  File "/usr/lib/python2.7/site-packages/pycdlib/dr.py", line 26, in <module>
    from pycdlib import dates
  File "/usr/lib/python2.7/site-packages/pycdlib/dates.py", line 31, in <module>
    from pycdlib import utils
  File "/usr/lib/python2.7/site-packages/pycdlib/utils.py", line 483
    target = rf"\\.\{target}"
                            ^
SyntaxError: invalid syntax
clalancette commented 2 years ago

Oh, I see. I only test on Linux, so I never saw that.

Well, the library is meant to be Python 2.7 compatible. I don't think we need to use f-strings. I can propose a patch to fix it, but I can't really test it out. Would you be willing to test it if I open a PR to change that?

clalancette commented 2 years ago

As an FYI, I just pushed 9006ad8bfc5f88c277e67d52e237a207275ddb2d , which switches the format strings out for more traditional string substitution. I think that should fix the issue on Python 2.7. If someone can confirm that on Windows, then I can do a patch release of pycdlib that has this fix in it.

felixfontein commented 2 years ago

I cannot test on Windows, but I could do a cursory import test on Python 2 in a venv, and it still fails:

>>> import pycdlib
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "pycdlib/__init__.py", line 5, in <module>
    from .pycdlib import PyCdlib  # NOQA
  File "pycdlib/pycdlib.py", line 38, in <module>
    from pycdlib import dr
  File "pycdlib/dr.py", line 26, in <module>
    from pycdlib import dates
  File "pycdlib/dates.py", line 31, in <module>
    from pycdlib import utils
  File "pycdlib/utils.py", line 528
    return (*geometry, disk_size)
            ^
SyntaxError: invalid syntax
>>> 

You probably want to extend your CI to also do some basic testing (installing and running flake8 or something like that) on Python 2.7 and 3.4.

clalancette commented 2 years ago

You probably want to extend your CI to also do some basic testing (installing and running flake8 or something like that) on Python 2.7 and 3.4.

The thing is, I do this testing locally, and it has all been passing for me. That's why I'm so confused.

Ah, actually, I see what is happening now. It turns out that my scripts for testing this out aren't actually invoking Python 2 properly, but were still invoking Python 3. That's why it was always working for me locally. Let me fix that up and then see what happens.

clalancette commented 2 years ago

All right, I believe that this is really fixed now. I was able to get my local tests actually using Python 2.7, and the latest commit in 7e96f95624b97cfa4e59cc61b2b86ed411f9e3aa fixes the last problem that I know about. I also added in a new test in #96 for Python 2.7 . Thanks for the report, I'm going to close this out for now.