Skip to content

/ arcanist

Permalink
Browse files
Add a configuration for python path 
Test Plan: not a clue

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5717
  • Loading branch information
Phil Dibowitz committed Apr 16, 2013
1 parent b310866 commit fb88bb9da69e721f3e91f4dfc1e42e8b7b7f9df3
Unified Split
Showing with 1 addition and 8 deletions.
  1. +1 −8 src/lint/linter/ArcanistPEP8Linter.php
9 src/lint/linter/ArcanistPEP8Linter.php
View file
@@ -41,7 +41,7 @@ public function getPEP8Path() {
$bin = $working_copy->getConfig('lint.pep8.bin');

if ($bin === null && $prefix === null) {
$bin = csprintf('/usr/bin/env python %s',
$bin = csprintf('/usr/bin/env python2.6 %s',
phutil_get_library_root('arcanist').
'/../externals/pep8/pep8.py');
} else {
@@ -74,13 +74,6 @@ public function getPEP8Path() {
}
}

list(, $stderr) = execx('/usr/bin/env python -V');
if ($stderr < 'Python 2.5') {
throw new ArcanistUsageException(
"Python 2.5 or greater is required to run the PEP8 Python linter, but ".
rtrim($stderr)." was found instead.");
}

return $bin;
}

11 comments on commit fb88bb9

@bolinfest

This comment has been minimized.

Sign in to view
Copy link

@bolinfest bolinfest replied May 8, 2013

I'm using Ubuntu 12.04, so I have Python 2.7 not 2.6 and now things aren't working for me. Why are you requiring 2.6?
@relrod

This comment has been minimized.

Sign in to view
Copy link
Contributor

@relrod relrod replied May 8, 2013

fwiw, Fedora doesn't have a python2.6 alias either.
@epriestley

This comment has been minimized.

Sign in to view
Copy link
Member

@epriestley epriestley replied May 8, 2013

@jaymzh ^
@jaymzh

This comment has been minimized.

Sign in to view
Copy link
Contributor

@jaymzh jaymzh replied May 9, 2013

I'm not sure the best solution here. @epriestley you didn't like the idea of making the python path configurable, the original version of this patch, which is why we went down this road.

FYI, @bolinfest it's so you don't pick up the default 2.4 version of py on CentOS5 :)

I see two options:

  1. Try python2.7, python2.6, python, in that order, use whichever is found
  2. Make it configurable.
@relrod

This comment has been minimized.

Sign in to view
Copy link
Contributor

@relrod relrod replied May 9, 2013

I would like to think that Cent/RHEL 5 is a fairly rare case. Is it unreasonable to just fix your $PATH so that /usr/bin/env python works? The entire point of /usr/bin/env is to be universal, and by hardcoding a version, we're going against that.

Even if we go down from python2.7 - what happens when 2.8 comes out, or when we inevitably one day target 3.0 and Cent 5 is still on 2.4 - this reeks of not being maintainable, in that we'd have to update it each time.

I'd be more in favor of making it a config thing - but I really think this is a pretty rare case, and correcting your $PATH yourself is the proper fix.

IMO, the best solution is to go back to /usr/bin/env python and possibly make the error hint to the user "If you have another Python that we should use, make sure you prioritize your $PATH accordingly." /cc @epriestley
@jaymzh

This comment has been minimized.

Sign in to view
Copy link
Contributor

@jaymzh jaymzh replied May 9, 2013

RHEL5 is actually quite common. Sadly.

The reason PATH doesn't work is because most system things want the system python, changing the path to shove /usr/local/bin in front breaks things. Python isn't terribly backwards compatible.

Further, arc expects py2.6 or later, and so it makes sense to specify such.

Also, we tried setting the path in PHP land before calling the py linter, but it didn't work, sadly.

I'm fine making it configurable...
@relrod

This comment has been minimized.

Sign in to view
Copy link
Contributor

@relrod relrod replied May 9, 2013

What about something like:

PATH=/usr/local/bin:$PATH arc [..] (and alias arc to that)

That is a good point that system things (namely yum, I suspect) will break with newer Python.

Just tossing out some ideas if @epriestley still doesn't want to clutter the config with this.
@jaymzh

This comment has been minimized.

Sign in to view
Copy link
Contributor

@jaymzh jaymzh replied May 9, 2013

that's a lot of .bashrcs to update :)
@epriestley

This comment has been minimized.

Sign in to view
Copy link
Member

@epriestley epriestley replied May 9, 2013

In general, the problems here are:

  • With python earlier than 2.6, PEP8 dies with a syntax error which is confusing to users and which they can not resolve unless they are extremely sophisticated, because it gives them no hints that it's a python version error. We should not emit this error, so changing it to python alone isn't acceptable.
  • Previously, we used python and then did a bunch of hacky stuff to check the Python version and try to raise a better error. This didn't work for @jaymzh because the first python in his path was python 2.5 (or some other earlier version).
  • I don't think I objected to making the path configurable (in D5717, I only had issues with the configuration being untested and undocumented, it seems?) but I don't especially like this. One issue is that it's dead in the water if two users want to run on two different systems which locate the right python version in two different locations.

A workaround is:

  • To run pep8, you must have some python binary capable of running pep8 somewhere on your system.
  • Create a symlink to that python binary called python2.6 anywhere your $PATH.

Generally, I'm going to fix this in several ways:

  • Implement support/bin/ for Arcanist, like the similar mechanism in Phabricator. Specifically, arc will ship with an empty support/bin/ directory and add that directory to its $PATH first.
  • Genericize all the "run an external program and do stuff with its output" linters.
  • Provide a generic binary-selection mechanism to lint engines.
  • Select python2.6 preferentially, then python, and validate the version of python.
  • Implement https://secure.phabricator.com/T2039 and allow per-engine overrides.
  • Allow per-machine overrides with config on top of named engines.

This will probably take a while to get through (like, possibly months). You can pursue the workaround above until then. After the first step lands, you can create the python2.6 symlink in arcanist/support/bin instead of elsewhere on your system.
@bolinfest

This comment has been minimized.

Sign in to view
Copy link

@bolinfest bolinfest replied May 9, 2013

It sounds like it's the Python 2.5 (<2.6) users that are ruining this for everyone. Since they're likely the dying breed, why not punish them instead of incurring extra steps for more up-to-date and future users?
@epriestley

This comment has been minimized.

Sign in to view
Copy link
Member

@epriestley epriestley replied May 9, 2013

All python users are ruining this for me. The action plan above is carefully crafted to reduce support requests as close to zero as I can. :)
Please sign in to comment.