munificent / vigil

Vigil, the eternal morally vigilant programming language
2.9k stars 61 forks source link

Fails if python is Python3 #36

Closed Keith-S-Thompson closed 2 years ago

Keith-S-Thompson commented 2 years ago

The first line of vigil is

#!/usr/bin/env python

On my system (Ubuntu 22.04.1 LTS), /usr/bin/python is a symlink to python3, and since vigil uses Python2-specific constructs, it fails.

I have a patch that adds parentheses to all the print statements, making the code compatible with both Python2 and Python3. I'll create a pull request in a moment.

Another solution would be to change the first line to

#!/usr/bin/env python2

but that would fail on systems that don't have Python2 installed, or that don't call it python2.

I see there's another pending PR that converts the code to Python3. I note that that could break on older systems where python is Python2.

I use Vigil for one of the entries in my fizzbuzz-polyglot project. My current workaround is to manually change the first line to #!/usr/bin/env python2.

This is, on multiple levels, not a critical issue for me.

Keith-S-Thompson commented 2 years ago

Pull request: https://github.com/munificent/vigil/pull/37

strugee commented 2 years ago

I note that that could break on older systems where python is Python2.

Nice catch! I'm getting soft... there was a time in my life where I totally would've thought of that :joy:

strugee commented 2 years ago

Actually @Keith-S-Thompson I believe that your PR isn't sufficient to run on Python 3, only to not throw syntax errors? Just offhand I see that xrange is still in use, which doesn't exist in Python 3.

Keith-S-Thompson commented 2 years ago

@strugee You're quite correct.

It worked with Python 3 in my testing because punish() isn't called. (I hadn't realized that the error isn't caught unless the code is executed.)

I think that replacing xrange by range would fix that problem, but more testing would be required to make sure all code paths are ok (and my Python-fu is insufficient to be sure enough to submit a patch).

strugee commented 2 years ago

IIRC I did all that testing for #35. So that's probably the solution. I guess I should push up a fix to the shebang?