Skip to content

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

Closed
ilovezfs wants to merge 30 commits into
Homebrew:masterfrom
ilovezfs:PEP-394
Closed

python, python@2: make PEP 394 compliant#25060
ilovezfs wants to merge 30 commits into
Homebrew:masterfrom
ilovezfs:PEP-394

Conversation

@ilovezfs

@ilovezfs ilovezfs commented Mar 9, 2018

Copy link
Copy Markdown
Contributor
  • 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

Comment thread Formula/python@2.rb Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 20 commits March 9, 2018 15:09
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
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

Copy link
Copy Markdown
Contributor

MacVim seems not yet included in this batch of changes.

@fxcoudert

Copy link
Copy Markdown
Member

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

@astiob

astiob commented Mar 10, 2018

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

@ilovezfs

ilovezfs commented Mar 11, 2018

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown

sweet thx, ilovezfs! Not all heroes wear capes!

@saagarjha

saagarjha commented Mar 12, 2018

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown
Contributor Author

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

@saagarjha

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

@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.

Labels

python Python use is a significant feature of the PR or issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants