In the original review, I suggested that we don't add Python 2
compatibility for new features because Python 2.7 is retiring and some
Linux distributions are even deprecating/removing Python 2 support. My
feeling is:
If some utilities do not support Python 2, we should probably not bother
making them Python 2 compatible. Maintaining Python 2/3 compatibility
may not worth the efforts. "utilities" include some command line tools
under llvm/utils, which are not part of instructure like lit. What do
people think?
BTW, what's the Python 3 support status of build bots? Are there any
running Python 3?
I personally only use Python 3 reluctantly. I’ve yet to encounter a situation where I actually preferred Python 3. That being said, given the decision to retire Python 2.7 (grumblegrumble), I’ll probably be forced sometime in the new year to uninstall it by somebody in charge of security somewhere. I certainly don’t see a personal need to have all scripts support Python 2, unless they are used in the build/test pipeline somewhere (i.e. get touched by a fresh check-all).
At the beginning of the year, I’ve landed a large set of patches to support both Python 2 and Python3 in most Python scripts. Looks like I missed some of them
At that time, backward portability with Python2 was still relevant, and I suspect it will still be the case for a few distributions that ship Python2 by default. That being said, Even RHEL8 uses Python3 by default, so at some point we may be able to drop the compatibility stuff.
Until then, I’d argue for maintaining compatibility as it’s not a tremendous task.
At the beginning of the year, I’ve landed a large set of patches to support both Python 2 and Python3 in most Python scripts. Looks like I missed some of them
At that time, backward portability with Python2 was still relevant, and I suspect it will still be the case for a few distributions that ship Python2 by default. That being said, Even RHEL8 uses Python3 by default, so at some point we may be able to drop the compatibility stuff.
Until then, I’d argue for maintaining compatibility as it’s not a tremendous task.
This is my feeling as well. In yesterday’s instance, I lost more time to fixing bugs in the py3 detection logic on systems that don’t have it than it took me to make the script just run fine with both Python 2 and 3.
On macOS, I think 10.15 is the first version that ships with python3, and that was released just 2 months ago.
IMO, having non-critical utility scripts require python 3 should be allowed now. But, not yet for any scripts which are critical to build or test the distributed components.
If we need to spend some time to fix the test runner to allow properly skipping tests of python3-only components when python3 isn’t available, that seems entirely worthwhile, since we only need to do that once.
How do you define “non-critical”? That seems like a rule that’s hard to apply consistently.
In this case, they’re covered by tests, so they’re considered somewhat critical I suppose.
I personally have no problem if we make the decision to drop Py 2 support across the board, but allowing a mix seems confusing to me.
If we do want to drop Py 2 support, we should probably use the same process we use for bumping C++ or CMake versions: List advantages and costs, and evaluate based on that. Since Py 2 is still the only installed Python on fairly recent OS versions, I weakly feel that dropping support for it is premature, but I don’t care all that much. I do care that the community has a “yes” or “no” answer to the question “do we support Python 2?”.
I define “critical” as: anything which is required to build or test any components which are part of a release. The intent being that we DO continue to support python 2 for building llvm, and for end-users of llvm, for now.
However, developers of LLVM can be assumed to be able to install python3 if they want to be able to run these various optional, auxiliary, scripts. Having a unit test for a script should not make that script “critical”, when the purpose of the test is only to test that script – the test should simply be skipped when python3 is unavailable.
That sounds nice in theory, but in practice it means that people who run tests and don’t happen to have Python 3 installed on their fleet get to debug random test failures. Which, anecdotally, is more work than just supporting Python 2 everywhere. It also makes it easier to start shipping a utility script in a release (promoting it to “critical” per your definition), and it’s a rule that’s much easier to remember.
It sounds like you ran into a bug in the test infrastructure’s code to determine if python3 is supported. Fixing that might be harder, but it only needs to be fixed once no matter how much more python3 development there will be.
Right now, most of our scripts were originally written for python 2, so certainly it’s easy for them to support python 2. But, it was a lot of work by various people to port them all to additionally work in python 3. And, in the future (or maybe even now), people will be generally be writing python3 scripts by default rather than python2. Certainly they ought to.
I just don’t think it’s worthwhile to require all new such scripts to continue to be written bilingually, unless doing that extra work helps to serve users.
I’m not at all worried about a hypothetical case where we want to ship a script that was written for python3 only. Firstly, because that usually doesn’t happen. But if it does, we can port it then, or else we might just decide it’s fine for it to be python3 only.
It sounds like you ran into a bug in the test infrastructure’s code to determine if python3 is supported. Fixing that might be harder, but it only needs to be fixed once no matter how much more python3 development there will be.
No, it was in some local.lit.cfg.
Right now, most of our scripts were originally written for python 2, so certainly it’s easy for them to support python 2. But, it was a lot of work by various people to port them all to additionally work in python 3. And, in the future (or maybe even now), people will be generally be writing python3 scripts by default rather than python2. Certainly they ought to.
I just don’t think it’s worthwhile to require all new such scripts to continue to be written bilingually, unless doing that extra work helps to serve users.
I’m not at all worried about a hypothetical case where we want to ship a script that was written for python3 only. Firstly, because that usually doesn’t happen. But if it does, we can port it then, or else we might just decide it’s fine for it to be python3 only.
You don’t see any advantage to having a consistent language level across the project? (See also the flang vs c++17 discussion.)
It sounds like you ran into a bug in the test infrastructure’s code to determine if python3 is supported. Fixing that might be harder, but it only needs to be fixed once no matter how much more python3 development there will be.
No, it was in some local.lit.cfg.
I see that now. Sure, in that case I suggest to fix whatever the issue is and move it to common code, so that the “python3” feature is correctly detected and available to any test.
Right now, most of our scripts were originally written for python 2, so certainly it’s easy for them to support python 2. But, it was a lot of work by various people to port them all to additionally work in python 3. And, in the future (or maybe even now), people will be generally be writing python3 scripts by default rather than python2. Certainly they ought to.
I just don’t think it’s worthwhile to require all new such scripts to continue to be written bilingually, unless doing that extra work helps to serve users.
I’m not at all worried about a hypothetical case where we want to ship a script that was written for python3 only. Firstly, because that usually doesn’t happen. But if it does, we can port it then, or else we might just decide it’s fine for it to be python3 only.
You don’t see any advantage to having a consistent language level across the project? (See also the flang vs c++17 discussion.)
In this particular situatoin, correct. For these auxilliary scripts which are not released or used to build/test released components, I see no advantage to requiring these to support python2, anymore.
Don't really care, but I have a mild preference for accepting patches to keep python2 working. I wouldn't *require* scripts to work with python2, but I see no reason not to land patches if someone wants to put in the work.
If some library-like Python scripts are changed to support without
introducing complexity, I certainly agree.
I think that particular patch actually achieves this goal. What I am
concerned of is that the patch changes the shebang from python3 to
python (may be a symlink to either python2 or python3), which means the
auxilliary script can more likely have different behaviors on different
people's machines.
challenging. A lot of complexity comes from str/unicode vs bytes/str
differences. The code can be made quite a bit cluttered.
