Skip to content
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

Merged
merged 2 commits into from
Sep 25, 2018
Merged

language/python: support pypy #4833

merged 2 commits into from
Sep 25, 2018

Conversation

jonchang
Copy link
Contributor

@jonchang jonchang commented Sep 6, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run 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 change depends_on "python@" to depends_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 to Language::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.

diff --git a/Formula/numpy.rb b/Formula/numpy.rb
index 2b08e8a790..4d4948de23 100644
--- a/Formula/numpy.rb
+++ b/Formula/numpy.rb
@@ -23,8 +23,7 @@ class Numpy < Formula
   option "without-python@2", "Build without python2 support"
 
   depends_on "gcc" => :build # for gfortran
-  depends_on "python@2" => :recommended
-  depends_on "python" => :recommended
+  depends_on "pypy" => :recommended
 
   resource "nose" do
     url "https://files.pythonhosted.org/packages/58/a5/0dc93c3ec33f4e281849523a5a913fa1eea9a3068acfa754d44d88107a44/nose-1.3.7.tar.gz"
@@ -33,17 +32,18 @@ class Numpy < Formula
 
   def install
     Language::Python.each_python(build) do |python, version|
-      dest_path = lib/"python#{version}/site-packages"
+      site_packages = Language::Python.site_packages(python)
+      dest_path = prefix/site_packages
       dest_path.mkpath
 
-      nose_path = libexec/"nose/lib/python#{version}/site-packages"
+      nose_path = libexec/"nose"/site_packages
       resource("nose").stage do
         system python, *Language::Python.setup_install_args(libexec/"nose")
         (dest_path/"homebrew-numpy-nose.pth").write "#{nose_path}\n"
       end
 
       if build.head?
-        ENV.prepend_create_path "PYTHONPATH", buildpath/"tools/lib/python#{version}/site-packages"
+        ENV.prepend_create_path "PYTHONPATH", buildpath/"tools"/site_packages
         resource("Cython").stage do
           system python, *Language::Python.setup_install_args(buildpath/"tools")
         end
@@ -51,7 +51,7 @@ class Numpy < Formula
 
       system python, "setup.py",
         "build", "--fcompiler=gnu95", "--parallel=#{ENV.make_jobs}",
-        "install", "--prefix=#{prefix}",
+        "install", "--prefix=#{prefix}", "--install-scripts=#{bin}",
         "--single-version-externally-managed", "--record=installed.txt"
     end
   end
  • 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 to share instead of bin.

    • From what I can tell, regular scripts installed via pip are put in bin and I think their shebang is modified to point to the opt_prefix.
    • This means they survive version upgrades anyway as long as python still is installed. I don't recall if the 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.

@ghost ghost assigned jonchang Sep 6, 2018
@ghost ghost added the in progress Maintainers are working on this label Sep 6, 2018
@@ -66,6 +73,7 @@ def self.setup_install_args(prefix)
--no-user-cfg
install
--prefix=#{prefix}
--install-scripts=#{prefix}/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

#{bin}?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

@DomT4
Copy link
Contributor

DomT4 commented Sep 6, 2018

there's not a whole lot of context why scripts were installed to share instead of bin.

I assume to prevent things installed using pypy's version of pip smashing into executables installed using the more widely used python and python3 versions of pip.

From what I can tell, regular scripts installed via pip

Outside of Homebrew, or? For me pip_pypy install virtualenv results in virtualenv living at /usr/local/share/pypy/virtualenv, with the shebang #!/usr/local/Cellar/pypy/6.0.0/bin/pypy.

@jonchang
Copy link
Contributor Author

jonchang commented Sep 6, 2018

I assume to prevent things installed using pypy's version of pip smashing into executables installed using the more widely used python and python3 versions of pip.

Aren't pip_pypy and friends are already renamed?

Outside of Homebrew, or? For me pip_pypy install virtualenv results in virtualenv living at /usr/local/share/pypy/virtualenv, with the shebang #!/usr/local/Cellar/pypy/6.0.0/bin/pypy.

With pip, not pip_pypy. With regular pip for python 2 the shebang is at #!/usr/local/opt/python@2/bin/python2.7. Not sure what the python@2 formula is doing differently.

@DomT4
Copy link
Contributor

DomT4 commented Sep 6, 2018

Aren't pip_pypy and friends are already renamed?

Only the pip and setuptools we install. Anything the user runs pip_pypy install on isn't appended with _pypy.

Not sure what the python@2 formula is doing differently.

Setting sys.executable.

@MikeMcQuaid
Copy link
Member

@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 pypy and python@2 in the same dependency tree (or process)? If so, does this detect or handle that in any way? What are the caveats if any you see from this approach.

FYI when I read the PR description I was 👎 on this but the code change is minimal enough I'm a very tentative 👍.

@jonchang
Copy link
Contributor Author

jonchang commented Sep 7, 2018

As a more general question: what benefits does this DSL provide compared to doing it manually in the formula?

This provides feature-parity for Homebrew/brew support for python@2 installs using virtualenv_install_with_resources. The idea is that an existing formula using that DSL can replace e.g., depends_on "python@2" with depends_on "pypy" and it should Just Work.

If so, does this detect or handle that in any way?

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 virtualenv_install_with_resources"). This is in theory extremely fragile as any changes to the python@2 or python formulae could break the install for these formulae. Grepping for site-packages or site_packages show that many formulae hardcode the location of site-packages, assuming that it will always live at e.g., $HOMEBREW_PREFIX/lib/python2.7/site-packages rather than reading out the value of Language::Python.homebrew_site_packages.

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.

What are the caveats if any you see from this approach.

The caveat is that if you are doing custom stuff (anything but virtualenv_install_with_resources) you'll have to look at the definitions in language/python.rb to see how PyPy does things differently. Namely, it installs site-packages directly into the prefix, rather than into lib/python2.7/site-packages. Formula authors who wish to continue using python@2 should see no change.

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.

Does it matter if you have pypy and python@2 in the same dependency tree (or process)?

There should be no issue with both pypy and python@2 in the same dependency tree. virtualenv_install_with_resources already detects any potential ambiguity and raises an exception asking the formula writer to clarify with :using => "python@2" or similar.

I don't see how it would be possible for pypy and python@2 to be in the same process; any scripts in bin are sandboxed into their respective virtualenvs and wouldn't have access to modules from another version of python installed. Side-by-side installs of libraries will need special handling, but formulae like numpy already do this. Applications should pick the right version of Python for them and stick with it.

FYI when I read the PR description I was 👎 on this but the code change is minimal enough I'm a very tentative 👍.

👍

@MikeMcQuaid
Copy link
Member

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!

@maxnordlund
Copy link
Contributor

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.

@jonchang
Copy link
Contributor Author

@MikeMcQuaid ok, I added tests for some of the functions in Language::Python. Will merge when tests are 📗

@MikeMcQuaid MikeMcQuaid merged commit 2ea3aee into Homebrew:master Sep 25, 2018
@MikeMcQuaid
Copy link
Member

Thanks @jonchang!

@ghost ghost removed the in progress Maintainers are working on this label Sep 25, 2018
@jonchang jonchang deleted the pypy-support branch September 26, 2018 17:36
@lock lock bot added the outdated PR was locked due to age label Oct 26, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants