New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

python, python@2: make PEP 394 compliant #25060

Closed
wants to merge 30 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@ilovezfs
Contributor

ilovezfs commented Mar 9, 2018

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

https://www.python.org/dev/peps/pep-0394/

This means python (Python 3) installs as python3 and that python@2
(Python 2) installs as python and python2.

Also,

python:

  • make gdbm, readline, sqlite and xz required instead of recommended
  • depend on sphinx-doc at build-time non-optionally to build the docs
  • remove quicktest option

python@2:

  • make non-keg_only
  • make gdbm, readline and sqlite required instead of recommended
  • depend on sphinx-doc at build-time non-optionally to build the docs
  • remove berkeley-db@4 optional dependency
  • remove quicktest option
  • remove poll option and leave poll enabled instead of forcing it off
# https://bugs.python.org/issue5154
if build.without? "poll"
inreplace "pyconfig.h", /.*?(HAVE_POLL[_A-Z]*).*/, '#undef \1'
end

This comment has been minimized.

@chdiza

chdiza Mar 9, 2018

Contributor

If the poll option is vanishing, then this will be built as if --without-poll were passed, which means we still need the inreplace, right?

This comment has been minimized.

@fxcoudert

fxcoudert Mar 9, 2018

Member

After reviewing the history of this hack, we want to get rid of it (i.e. in effect make --with-poll the new behavior).

Some background: the poll() system call on macOS is limited in some cases (devices, rather than regular files). Someone reported it to python, and suggested turning it off completely, i.e. mimicking a system that has no poll support at all. Upstream considered it, and decided they’d rather ship with poll support “as close to the OS”, rather than disable it altogether. We should follow that decision.

ilovezfs added some commits Mar 9, 2018

python: make PEP 394 compliant
This means python (Python 3) installs as `python3` and that python@2
(Python 2) installs as `python` and `python2`.

Also,
- make gdbm, readline, sqlite and xz required instead of recommended
- depend on sphinx-doc at build-time non-optionally to build the docs
- remove quicktest option
python@2: make PEP 394 compliant
This means python (Python 3) installs as `python3` and that python@2
(Python 2) installs as `python` and `python2`.

Also,
- make gdbm, readline and sqlite required instead of recommended
- depend on sphinx-doc at build-time non-optionally to build the docs
- remove berkeley-db@4 optional dependency
- remove quicktest option
- remove poll option and leave poll enabled instead of forcing it off
@adah1972

This comment has been minimized.

Contributor

adah1972 commented Mar 10, 2018

MacVim seems not yet included in this batch of changes.

@fxcoudert

This comment has been minimized.

Member

fxcoudert commented Mar 10, 2018

@adah1972 test passed… is macvim failing since this change?

@astiob

This comment has been minimized.

Contributor

astiob commented Mar 10, 2018

I’m late to the party, but I’m curious as to why you reversed the decision to avoid shadowing the system python at all that was made in #14408. Mind, I do shadow python myself, but I’ve been unable to find any discussion of this matter on GitHub. In other issues I’ve seen the reasoning that brew install python should install the latest Python, and that’s fine, but then what was the point of making python refer to /usr/bin/python?

@adah1972

This comment has been minimized.

Contributor

adah1972 commented Mar 11, 2018

@fxcoudert I misunderstood the change.

Let me be clear. Making python point to python3 does not bother me as much as making Vim linking with Python3. Dropping the Vim-Python2 support is a show-stopper to me: it breaks Vim scripts.

Making brew install python mean installing python3 does not sound good to me either. I would counter-propose that there should be separate python2 and python3 packages (not @2 as they can coexist very long together instead of one replacing another), and python should be a pseudo-package that installs the PEP-394 version of Python, and warns that its meaning may change in the future. The packages that depend on python should specify the specific version they depend on. Those that used to depend on Python should then depend on Python2 instead of Python3!

The current way like that brew upgrade python can destroy my Python2 installation and replace it with Python3 overnight is just too dangerous.

@adah1972

This comment has been minimized.

Contributor

adah1972 commented Mar 11, 2018

Reading more about this change, I think I am too late to the party. Maintainers really planned to do this half a year ago. I do not think my ‘counter-proposal’ can be accepted today, but I want to insist my point about dependent packages: packages that used to depend on Python should depend on Python2 instead of Python3 after the change of meaning of Python. Whether those packages should upgrade from Python2 to Python3 should be a case-by-case decision.

Please notice that python always mean python2 in the context of Vim/MacVim. Python3 is called python3. When I build Vim myself on Windows, I always build in support for both Python2 and Python3. (Since I do not develop Mac apps, I have only command-line tools—instead of the full Xcode—installed on my Mac.) The stand-alone download of MacVim currently has also both python2 and python3 support (as dyn versions, so they are not hard dependencies).

@markstachowski

This comment has been minimized.

markstachowski commented Mar 11, 2018

has anyone else encountered issues with their virtualenv and virtualenvwrapper now?

@ilovezfs

This comment has been minimized.

Contributor

ilovezfs commented Mar 11, 2018

@markstachowski virtualenvs use an absolute path, so whenever brew python is upgraded your virtualenvs are invalidated unless you keep the old versions lying around. Upstream does not consider that a bug. See pypa/pip#5048

@markstachowski

This comment has been minimized.

markstachowski commented Mar 11, 2018

sweet thx, ilovezfs! Not all heroes wear capes!

@acecilia acecilia referenced this pull request Mar 11, 2018

Closed

brew migrate python fails because of python 2.7.14_3 #24926

3 of 6 tasks complete
@saagarjha

This comment has been minimized.

Contributor

saagarjha commented Mar 12, 2018

@ilovezfs I think this change "breaks" swift:

$ swift
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/usr/local/Cellar/python@2/2.7.14_3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/copy.py", line 52, in <module>
    import weakref
  File "/usr/local/Cellar/python@2/2.7.14_3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/weakref.py", line 14, in <module>
    from _weakref import (
ImportError: cannot import name _remove_dead_weakref
Traceback (most recent call last):
  File "<input>", line 1, in <module>
NameError: name 'sys' is not defined

[EDIT: there's a bunch of these, so I removed them out of mercy to those who need to scroll through this]

Welcome to Apple Swift version 4.1 (swiftlang-902.0.43 clang-902.0.37.1). Type :help for assistance.
  1>

Would you mind taking a quick look and seeing if it's caused by this?

@ilovezfs

This comment has been minimized.

Contributor

ilovezfs commented Mar 12, 2018

@saagarjha is anything observably functionally broken other than the unwanted messages about imports?

@saagarjha

This comment has been minimized.

Contributor

saagarjha commented Mar 12, 2018

I couldn't find anything that didn't work, but I'd assume the error is coming from an LLDB script that swift loads. I'm not sure what it does, but whatever it is it's likely broken.

@ilovezfs

This comment has been minimized.

Contributor

ilovezfs commented Mar 12, 2018

@saagarjha OK. At this point, I don't think it's worth making python@2 keg_only over this, but you can

alias swift="PATH=/System/Library/Frameworks/Python.framework/Versions/Current/bin:$PATH swift"

as a workaround.

coreygo referenced this pull request in Homebrew/brew Mar 13, 2018

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.