-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
language/python: support pypy #4833
Conversation
@@ -66,6 +73,7 @@ def self.setup_install_args(prefix) | |||
--no-user-cfg | |||
install | |||
--prefix=#{prefix} | |||
--install-scripts=#{prefix}/bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#{bin}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That variable doesn't exist in this context. prefix
is passed from the caller (usually the formula for a more advanced build) and typically goes in the formula equivalent of libexec
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed it would exist when this variable would actually be called? We use buildpath
in this file, for example, which obviously isn't defined at this point. Could be wrong, it's 5am and I definitely should be asleep 😅.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does exist while called, but this function doesn't have access to it. prefix
is passed in by the formula, and it can be anything path-like. Check out e.g. sphinx-doc.rb for what I mean. libexec
(in the formula's context) turns into prefix
here.
buildpath
works because it's part of the Virtualenv
mixin class, which I believe would have access to its cohabitants variables like bin
etc.
I assume to prevent things installed using
Outside of Homebrew, or? For me |
Aren't
With |
Only the
Setting |
@jonchang As a more general question: what benefits does this DSL provide compared to doing it manually in the formula? Does it matter if you have FYI when I read the PR description I was 👎 on this but the code change is minimal enough I'm a very tentative 👍. |
This provides feature-parity for Homebrew/brew support for python@2 installs using
It does not detect formulae doing the same thing manually, since I think that would be better handled by an audit. Many formulae are already doing the "manual approach" (which here I take to mean "not using If that's something that should be fixed as well or instead of this patch, I think a new audit would be a better approach.
The caveat is that if you are doing custom stuff (anything but Arguably, if it makes more sense for formula authors to do this manually in all cases, this language-specific support should be removed from Homebrew/brew. I'm agnostic with respect to that option.
There should be no issue with both pypy and python@2 in the same dependency tree. I don't see how it would be possible for pypy and python@2 to be in the same process; any scripts in
👍 |
All sounds good to me @jonchang! Could you try to write some more tests in Library/Homebrew/test/language/python_spec.rb to increase the % of the patch that's hit? After that I'm 👍 on merging this. Thanks! |
One thing to note about PyPy 3 is that it's 3.5, not 3.7, though they are working on 3.6 and 3.7 was released just a couple of months ago. |
@MikeMcQuaid ok, I added tests for some of the functions in |
Thanks @jonchang! |
brew style
with your changes locally?brew tests
with your changes locally?Now that PyPy is maturing (it builds numpy and scipy now!) I think it would be good for Homebrew to support this, if not in core, but at least permit taps to use PyPy as opposed to Python. The Python dev team do not have speed as a huge priority, if I recall they prioritize readability and ease of understanding of the CPython source code.
This is an opt-in change. Formulae using the recommended
virtualenv_install_with_resources
method of installing a python formula and its dependencies merely need to changedepends_on "python@"
todepends_on "pypy"
.There is an API change here, but I've checked
homebrew/core
and the first ten pages of GitHub search and there are no formulae passing an argument toLanguage::Python.homebrew_site_packages
. A backwards compatible change could be made but I think it's cleaner this way.Formulae doing something more advanced need a bit more work, e.g., this diff for
numpy
successfully compiles a PyPy-enabled NumPy installation. I couldn't get a side-by-side install for Python 3, Python 3, and PyPy to work though. There's some compiler confusion going on. But PyPy by itself is a-ok.Currently, only pypy for python2 is supported. pypy3 has some issues that I'm investigating. But once that's figured out it should be simple enough to just add
pypy3
to the changed code.Setting the
--install-scripts
path is necessary for PyPy but not for regular Python. Not sure why this is. I dug up this commit Homebrew/homebrew-core@7d376d5fdf5 but there's not a whole lot of context why scripts were installed toshare
instead ofbin
.bin
and I think their shebang is modified to point to theopt_prefix
.opt_prefix
thing was introduced before or after 2012. So if that makes sense I'd like to make a follow up pull request in core to get that removed from the pypy formula.