Page MenuHomePhabricator

makefile: use Python 3 by default outside of Windows (BC)
AcceptedPublic

Authored by indygreg on Nov 6 2019, 1:43 PM.

Details

Reviewers
martinvonz
mharbison72
Group Reviewers
hg-reviewers
Summary

I think the time has come. My main reason for wanting to do this is
to force Mercurial developers to use Python 3 in their day-to-day work.
This should help flush out any remaining Python 3 bugs.

If this change is too controversial, we can revert it before the next
release.

.. bc::

Makefile now uses `python3` instead of `python` by default on
non-Windows platforms. This means Mercurial will be built and
run with Python 3 instead of Python 2.7 by default.

To continue using Python 2, set the PYTHON variable. e.g.
`make install PYTHON=python2.7`.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

indygreg created this revision.Nov 6 2019, 1:43 PM
martinvonz accepted this revision.Nov 6 2019, 1:52 PM
martinvonz added a subscriber: martinvonz.

All tests except for test-check-py3-compat.t pass on py3 (on Linux), right?

This revision is now accepted and ready to land.Nov 6 2019, 1:52 PM
dlax added a subscriber: dlax.Nov 6 2019, 3:04 PM

All tests except for test-check-py3-compat.t pass on py3 (on Linux), right?

Yes, tests are clean on Python 3.6 and 3.7. See https://ci.hg.gregoryszorc.com/.

This will break Windows by default, because py3 is still named python.exe. Alternately, I've been using using PYTHON="py -3" to invoke make.

The command issued to the buildbot will need to explicitly use PYTHON="py -2" to keep that happy. Probably also need to invoke run-tests.py with the right python too, so that $PYTHON is correct. It looks like the shebang lines are already $PYTHON, through some aren't quoted.

CC @durin42

This will break Windows by default, because py3 is still named python.exe. Alternately, I've been using using PYTHON="py -3" to invoke make.

I'm not sure what we should do here. We probably can't change the default only for Windows somehow? (My make-fu is not awesome)

The command issued to the buildbot will need to explicitly use PYTHON="py -2" to keep that happy. Probably also need to invoke run-tests.py with the right python too, so that $PYTHON is correct. It looks like the shebang lines are already $PYTHON, through some aren't quoted.

Okay, I'll see about wiring that up tonight or tomorrow then. I believe that's the only (other) thing blocking this?

CC @durin42

This will break Windows by default, because py3 is still named python.exe. Alternately, I've been using using PYTHON="py -3" to invoke make.

I'm not sure what we should do here. We probably can't change the default only for Windows somehow? (My make-fu is not awesome)

ifeq ($(shell uname -o), Msys)
PYTHON?=py -2
else
PYTHON?=python3
endif

Generally I just test uname, but that prints "MINGW32_NT-6.2" on Windows 10 and "MINGW32_NT-6.1" on Win7.

The command issued to the buildbot will need to explicitly use PYTHON="py -2" to keep that happy. Probably also need to invoke run-tests.py with the right python too, so that $PYTHON is correct. It looks like the shebang lines are already $PYTHON, through some aren't quoted.

Okay, I'll see about wiring that up tonight or tomorrow then. I believe that's the only (other) thing blocking this?

I think that's correct. I vaguely recall that some test helpers were run like ./foo.py at one point, which works on Windows because the registered file handler for *.py is python2. But that's more of an issue with py3 porting, and shouldn't block this. And maybe it was fixed with all of the $PYTHON work you did.

yuja added a subscriber: yuja.Nov 8 2019, 9:54 AM
ifeq ($(shell uname -o), Msys)
PYTHON?=py -2
else
PYTHON?=python3
endif

Perhaps, ifeq($(OS),Windows_NT) would work. My .profile had one.

indygreg planned changes to this revision.Nov 8 2019, 11:34 AM

I've seen ifeq($(OS),Windows_NT) used a lot. Although OS may be something else under msys. I'm away from my Windows machine until Sunday. I'll send a revision once I've had time to test on Windows.

I've seen ifeq($(OS),Windows_NT) used a lot. Although OS may be something else under msys. I'm away from my Windows machine until Sunday. I'll send a revision once I've had time to test on Windows.

I can confirm that this is the correct environment variable under msys on Windows 7 and 10, and it works in the makefile as suggested.

indygreg retitled this revision from makefile: use python3 by default (BC) to makefile: use Python 3 by default (BC).Nov 21 2019, 10:01 PM
indygreg edited the summary of this revision. (Show Details)
indygreg updated this revision to Diff 18276.
This revision is now accepted and ready to land.Nov 21 2019, 10:01 PM
indygreg updated this revision to Diff 18277.Nov 21 2019, 10:02 PM

This is ready for re-review. Please take a look @durin42.

mharbison72 accepted this revision.Nov 21 2019, 10:49 PM

To continue using Python 2, set the PYTHON variable. e.g.
make install PYTHON=python2.7 or
make install PYTHON=py.exe -2.

The "py -2" part needs to be quoted.

I'm not super excited to deal with this on Windows just yet, but I can't dispute the rationale. There are two traps for anybody dealing with this on Windows to be aware of:

  1. Running ./hg debuginstall will still run py2, even if it was built with py3. I assume this is the shebang line, and also an issue on other platforms.
  2. Running ./hg.exe debuginstall will run py3, but since that python isn't in $PATH, it complains that it can't find python37.dll and exits.

I know @durin42 took a whack at #1; I'm not sure if there's something we can do about #2. (I realize I could just edit $PATH, but it seems that the point of py.exe is to keep python.exe off of $PATH so that multiple installs can be managed. IIRC, it's not added to $PATH by default on installation.) Maybe we can just copy the dll to the local directory as part of setup.py?

I seriously question whether we need a Makefile at all [on Windows]: our Makefile is just a glorified shell script and isn't doing much in terms of dependency management. I'm tempted to move the packaging targets to contrib/packaging and replace the remaining build targets with a build.py script.

indygreg retitled this revision from makefile: use Python 3 by default (BC) to makefile: use Python 3 by default outside of Windows (BC).Nov 21 2019, 11:09 PM
indygreg edited the summary of this revision. (Show Details)
indygreg updated this revision to Diff 18278.

I just uploaded a compromise patch that ignores Windows for now.

I seriously question whether we need a Makefile at all [on Windows]: our Makefile is just a glorified shell script and isn't doing much in terms of dependency management. I'm tempted to move the packaging targets to contrib/packaging and replace the remaining build targets with a build.py script.

That seems OK to me. I like being able to mindlessly run the same commands on any platform. But in practice I only ever run make clean and make local everywhere, and make install-bin on unix systems. Maybe a python script could handle that everywhere. And I'd be fine with the packaging targets elsewhere in any case.

I just uploaded a compromise patch that ignores Windows for now.

Thanks!