Patchwork D7258: makefile: use python3 by default (BC)

login
register
mail settings
Submitter phabricator
Date Nov. 6, 2019, 6:43 p.m.
Message ID <differential-rev-PHID-DREV-hmtj7vcgrm3gzy2motsg-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42809/
State New
Headers show

Comments

phabricator - Nov. 6, 2019, 6:43 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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. 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`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

AFFECTED FILES
  Makefile

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 6, 2019, 6:52 p.m.
This revision is now accepted and ready to land.
martinvonz added a comment.
martinvonz accepted this revision.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel
Raphaël Gomès - Nov. 6, 2019, 10:40 p.m.
I think it's a good idea to try to rip the band-aid sooner than later.

+1 for what Martin said about Linux tests, it would make sense to know 
where we're starting to not have the test suite break on our series for 
Python3 reasons.

On 11/6/19 7:43 PM, indygreg (Gregory Szorc) wrote:
> indygreg created this revision.
> Herald added a subscriber: mercurial-devel.
> Herald added a reviewer: hg-reviewers.
>
> REVISION 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. 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`.
>
> REPOSITORY
>    rHG Mercurial
>
> REVISION DETAIL
>    https://phab.mercurial-scm.org/D7258
>
> AFFECTED FILES
>    Makefile
>
> CHANGE DETAILS
>
> diff --git a/Makefile b/Makefile
> --- a/Makefile
> +++ b/Makefile
> @@ -5,7 +5,7 @@
>   # % make PREFIX=/opt/ install
>   
>   export PREFIX=/usr/local
> -PYTHON?=python
> +PYTHON?=python3
>   $(eval HGROOT := $(shell pwd))
>   HGPYTHONS ?= $(HGROOT)/build/pythons
>   PURE=
>
>
>
> To: indygreg, #hg-reviewers
> Cc: mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
phabricator - Nov. 7, 2019, 8:03 a.m.
indygreg added a comment.


  In D7258#106380 <https://phab.mercurial-scm.org/D7258#106380>, @martinvonz wrote:
  
  > 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/.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: dlax, martinvonz, mercurial-devel
phabricator - Nov. 7, 2019, 4:36 p.m.
mharbison72 added a comment.
mharbison72 added subscribers: durin42, mharbison72.


  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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 7, 2019, 8:29 p.m.
durin42 added a comment.


  In D7258#106949 <https://phab.mercurial-scm.org/D7258#106949>, @mharbison72 wrote:
  
  > 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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 8, 2019, 1:21 a.m.
mharbison72 added a comment.


  In D7258#106995 <https://phab.mercurial-scm.org/D7258#106995>, @durin42 wrote:
  
  > In D7258#106949 <https://phab.mercurial-scm.org/D7258#106949>, @mharbison72 wrote:
  >
  >> 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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: mharbison72, durin42, dlax, martinvonz, mercurial-devel
Yuya Nishihara - Nov. 8, 2019, 2:53 p.m.
>     ifeq ($(shell uname -o), Msys)
>     PYTHON?=py -2
>     else
>     PYTHON?=python3
>     endif

Perhaps, `ifeq($(OS),Windows_NT)` would work. My .profile had one.
phabricator - Nov. 8, 2019, 2:54 p.m.
yuja added a comment.


  >   ifeq ($(shell uname -o), Msys)
  >   PYTHON?=py -2
  >   else
  >   PYTHON?=python3
  >   endif
  
  Perhaps, `ifeq($(OS),Windows_NT)` would work. My .profile had one.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 8, 2019, 4:35 p.m.
indygreg added a comment.
indygreg planned changes to this revision.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 8, 2019, 4:59 p.m.
mharbison72 added a comment.


  In D7258#107444 <https://phab.mercurial-scm.org/D7258#107444>, @indygreg wrote:
  
  > 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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 22, 2019, 3:03 a.m.
indygreg added a comment.


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

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 22, 2019, 3:49 a.m.
mharbison72 added a comment.
mharbison72 accepted this revision.


  > 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?

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz, mharbison72
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 22, 2019, 4:06 a.m.
indygreg added a comment.


  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.

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz, mharbison72
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 22, 2019, 4:10 a.m.
indygreg added a comment.


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

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz, mharbison72
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel
phabricator - Nov. 22, 2019, 4:22 a.m.
mharbison72 added a comment.


  In D7258#109923 <https://phab.mercurial-scm.org/D7258#109923>, @indygreg wrote:
  
  > 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.
  
  In D7258#109927 <https://phab.mercurial-scm.org/D7258#109927>, @indygreg wrote:
  
  > I just uploaded a compromise patch that ignores Windows for now.
  
  Thanks!

REPOSITORY
  rHG Mercurial

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7258/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7258

To: indygreg, #hg-reviewers, martinvonz, mharbison72
Cc: yuja, mharbison72, durin42, dlax, martinvonz, mercurial-devel

Patch

diff --git a/Makefile b/Makefile
--- a/Makefile
+++ b/Makefile
@@ -5,7 +5,7 @@ 
 # % make PREFIX=/opt/ install
 
 export PREFIX=/usr/local
-PYTHON?=python
+PYTHON?=python3
 $(eval HGROOT := $(shell pwd))
 HGPYTHONS ?= $(HGROOT)/build/pythons
 PURE=