This repository has been archived by the owner. It is now read-only.

PythonDependency: massive refactoring #24842

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
10 participants
Owner

MikeMcQuaid commented Dec 1, 2013

Kill PythonHelper and cut everything out of PythonDependency beyond
setting up PYTHONPATH and supporting the existing DSL without changes.

This isn't ready to be merged yet, it's up for review of the approach.
I next to test a lot more formulae, Python 3 support and modify some to
behave nicely. Basic tests seem to show this works, though.

@mistydemeo mistydemeo and 1 other commented on an outdated diff Dec 1, 2013

Library/Homebrew/extend/pathname.rb
@@ -387,6 +387,19 @@ def write_exec_script *targets
end
end
+ # Writes an exec script that sets an environment variable
@mistydemeo

mistydemeo Dec 1, 2013

Contributor

That sets multiple environment variables, right?

@MikeMcQuaid

MikeMcQuaid Dec 2, 2013

Owner

It can, yes. Good point.

@mistydemeo mistydemeo and 2 others commented on an outdated diff Dec 1, 2013

Library/Homebrew/formula.rb
@@ -181,6 +181,9 @@ def zsh_completion; share+'zsh/site-functions' end
# for storing etc, var files for later copying from bottles
def bottle_prefix; prefix+'.bottle' end
+ # for installing private Python libraries.
+ def private_python_packages; libexec+'lib/python2.7/site-packages' end
@mistydemeo

mistydemeo Dec 1, 2013

Contributor

How about folding the mkpath in here so formulae don't have to request it specifically?

@MikeMcQuaid

MikeMcQuaid Dec 2, 2013

Owner

Sounds reasonable.

@jacknagel

jacknagel Dec 2, 2013

Contributor

Eh, I would be wary of making a method that looks like an accessor "do stuff".

@mistydemeo

mistydemeo Dec 2, 2013

Contributor

On the other hand, then every formula has to mkpath this before using it the first time. Not a fan of the repetition.

@jacknagel

jacknagel Dec 2, 2013

Contributor

Making it create the directory means that it can never be used, at all, outside of the install method. There must be a better way.

@mistydemeo

mistydemeo Dec 2, 2013

Contributor

Good point.

@mistydemeo mistydemeo and 1 other commented on an outdated diff Dec 1, 2013

Library/Homebrew/formula.rb
- # Note that there are no quotation marks around python!
- # <https://github.com/mxcl/homebrew/wiki/Homebrew-and-Python>
- def python(options={:allowed_major_versions => [2, 3]}, &block)
- require 'python_helper'
- python_helper(options, &block)
- end
-
- # Explicitly only execute the block for 2.x (if a python 2.x is available)
- def python2 &block
- python(:allowed_major_versions => [2], &block)
- end
-
- # Explicitly only execute the block for 3.x (if a python 3.x is available)
- def python3 &block
- python(:allowed_major_versions => [3], &block)
+ # Deprecated
@mistydemeo

mistydemeo Dec 1, 2013

Contributor

I notice a few formulae calling python blocks still in this PR, are you planning to eliminate those?

@MikeMcQuaid

MikeMcQuaid Dec 2, 2013

Owner

Yeh. I just want to make sure they still work first (to avoid breaking the DSL).

@jacknagel jacknagel and 1 other commented on an outdated diff Dec 2, 2013

Library/Homebrew/caveats.rb
@@ -7,11 +7,12 @@ def initialize(f)
def caveats
caveats = []
- caveats << f.caveats
+ caveats << f.caveats if f.caveats.to_s.length > 0
@jacknagel

jacknagel Dec 2, 2013

Contributor

What is this guarding against?

@MikeMcQuaid

MikeMcQuaid Dec 3, 2013

Owner

Empty caveats string spitting out an unnecessary newline. Annoyingly need this until we can deprecate python_caveats returning an empty string.

@jacknagel

jacknagel Dec 3, 2013

Contributor

I figured it was something like that :(

Contributor

adamv commented Dec 15, 2013

I'm still trying to think of a better pattern or name than Pathname.mv_and_do_a_thing.

Owner

MikeMcQuaid commented Dec 15, 2013

Indeed. I'm open to suggestions. Perhaps should build it into something that does it for everything in bin.

Owner

MikeMcQuaid commented Dec 20, 2013

Updated with some more tweaks to helpers and the ansible.rb-like formulae. Looks pretty good now. I'm going to focus now on getting the rest of the stuff infested with python.whatever working without.

Owner

MikeMcQuaid commented Jan 4, 2014

@Homebrew/owners I've pushed all the changes to formulae but not the PythonDependency changes. This means that it should be easy to revert individual formulae (and ping me) if there are problems (which I'm sure there will be). When this has calmed down for a week or so I'll push the PythonDependency stuff. Apologies for the imminent breakage.

Owner

MikeMcQuaid commented Jan 4, 2014

@samueljohn Some formulae have had functionality removed that you added. Please feel free to re-add functionality but make PRs per-formula and any requested core changes with small diffs, thanks.

@dakcarto dakcarto referenced this pull request in OSGeo/homebrew-osgeo4mac Jan 7, 2014

Closed

qgis install fails #10

@dakcarto dakcarto added a commit to OSGeo/homebrew-osgeo4mac that referenced this pull request Jan 7, 2014

@dakcarto dakcarto qgis-20: large python dependency refactor; forces homebrew python 2.7…
… for OS < 10.7

- Note: this is in preparation for upcoming major homebrew python dependency changes
  see: Homebrew/legacy-homebrew#24842
731eae2
Owner

MikeMcQuaid commented Jan 9, 2014

Probably going to merge this this weekend unless hordes of people beg for mercy.

Contributor

samueljohn commented Jan 9, 2014

Okay. Question: when you now pip install stuff (without a sitecustomize.py) where do the modules get installed to?

Owner

MikeMcQuaid commented Jan 9, 2014

Happy to have that moved to the python formula.

Owner

MikeMcQuaid commented Jan 9, 2014

Feel free to make a PR against this PR.

Contributor

samueljohn commented Jan 9, 2014

Alright. That might work. Will make a PR.

Owner

MikeMcQuaid commented Jan 11, 2014

@samueljohn What's your ETA?

Contributor

samueljohn commented Jan 19, 2014

@MikeMcQuaid I did some test installs and played around with formulae but this needs more time. Brewed python mostly works (though some things will break if a new minor release of python 2.x.y will come as some paths are hardcoded into the Cellar). The depends_on 'nose' => :python does not seem to work for me. Not sure why.

I only startet testing python 3.x support ...
but, sorry, I don't have a reliable ETA. Feel free to fix before I come up with a PR, if you have a good idea.

Owner

MikeMcQuaid commented Jan 19, 2014

You don't need to test anything. What you agreed to is sorting out the sitecustomise stuff. I'll probably just remove it completely instead.

but this needs more time

I'm sorry but we've waited 5 months at this point for you to clean this up. You said you'd get this done this weekend. There is no more time. I'll do it myself.

Contributor

samueljohn commented Jan 19, 2014

@MikeMcQuaid sure, go ahead. I don't want to hold you back.

Owner

MikeMcQuaid commented Jan 20, 2014

@BrewTestBot test this please

Owner

MikeMcQuaid commented Jan 20, 2014

@BrewTestBot test this please

@MikeMcQuaid MikeMcQuaid deleted the unknown repository branch Jan 20, 2014

Owner

MikeMcQuaid commented Jan 20, 2014

Merged. Moved the sitecustomise.py stuff to the Python formulae. Here goes nothing; sorry if this breaks everything.

@wjwwood wjwwood referenced this pull request in Homebrew/homebrew-science Jan 21, 2014

Closed

vtk and homebrew/versions/vtk5 fail to build on mavericks #630

Contributor

wjwwood commented Jan 21, 2014

@MikeMcQuaid I think this new error in the vtk formula may be related to this. Currently the vtk formula does this:

# For Xcode-only systems, the headers of system's python are inside of Xcode:
args << "-DPYTHON_INCLUDE_DIR='#{python.incdir}'"
# Cmake picks up the system's python dylib, even if we have a brewed one:
args << "-DPYTHON_LIBRARY='#{python.libdir}/lib#{python.xy}.dylib'"

Which results in:

Warning: Formula#python is deprecated and will go away shortly.
Warning: Formula#python is deprecated and will go away shortly.
Error: undefined method `incdir' for #<PythonDependency: "python" []>

How should the vtk formulae (also vtk5 in homebrew-versions) be changed, or am I wrong and this is this unrelated to this pull request?

Owner

MikeMcQuaid commented Jan 21, 2014

It is, yes. The vtk formula needs fixed up; I'll try and take a look myself but basically it needs to hardcode the Python path rather than using this python. stuff any more.

@dpo dpo referenced this pull request in Homebrew/homebrew-science Jan 21, 2014

Closed

opencv 2.4.7.1 fails to install on OS 10.8.5 #628

Contributor

wjwwood commented Jan 21, 2014

It appears that removing the offending lines resolves the issue, which makes sense. I would think that the System Python's headers should always be on the include path by default. The note about them being somewhere inside Xcode seem like an antiquated problem.

Contributor

wjwwood commented Jan 21, 2014

The line about that linking might be relevant though, how are formula supposed to distinguish between the system Python and the brew Python. I suppose that it should link the brew Python if it is installed otherwise link to the system one, this may be another case where it now "just works" and this is not needed at all.

Not to be a downer @MikeMcQuaid but I would consider reverting these changes until you're sure that stuff won't break. Earlier today I decided to update my OpenCV install, and have proceeded to spend the past hour or so screwing with homebrew stuff because:
-the opencv formula doesn't work with your changes
-the suggested patch results in a build that segfaults on import

I manually worked around this by reverting my homebrew install to a point before your changes and I could update OpenCV successfully, but to be honest I only knew to do this because I'd already spent time in the past figuring out how homebrew works and I knew what things would be useful to try to debug this issue. From an "end-user" point of view, anybody who tries to install opencv today will be met with pain and frustration, which sucks.

At the least, since this is an API-breaking change would it be possible to output a console error message pointing end users to this PR or some debugging tips or something if a formula they are installing references python.incdir, python.standard_caveats, etc?

@wilsaj wilsaj referenced this pull request in Homebrew/homebrew-science Jan 21, 2014

Closed

qgis fails to build on Mavericks #631

Owner

MikeMcQuaid commented Jan 21, 2014

@peterbrook I fixed all the formulae in core. I don't think I should have to fix every formulae in every tap. I'm sorry this commit causes issues with some formulae but it needs to be made and solves issues with a bunch of other formulae. It's been on the cards for ~6 months.

Contributor

wjwwood commented Jan 22, 2014

@MikeMcQuaid I agree with you, it's just that we need some minimal amount of information on how these formula should change. It isn't clear to me what the migration path is for these formula. If you could give some examples of problems you've run into and how to properly handle them, I would gladly fix up formula I use.

For instance, what should we do instead of python.xy (still works right now, I think, but it does invoke the warning)? I know you said hardcode it, but should we then have py26-foo, py27-foo, and py33-foo (like Macports) or should we do some different conditional logic in the formula.

Another example, what about python.standard_caveats, that seemed pretty useful, should we replicate this everywhere too?

Owner

MikeMcQuaid commented Jan 22, 2014

@wjwwood Yeh, that's fair. Basically you need to go back to the approach used before we had the python do stuff; hardcode HOMEBREW_PREFIX/lib/python2.7/site-packages for example.

with-python and with-python3 were previously possible to both support at once. Now you need to pick one or the other (and/or create foo-py3 packages).

python.standard_caveats now happens automatically without needing added to the formula.

The commits around here:
https://github.com/Homebrew/homebrew/commits/master?page=12
or by running
git diff 86a9beda9f09094bd0f6289d106e7e0abb78ae7a..2f58395673ee20aec8c1454a6a3ae323b38dee7a

Should give you an idea of the type of changes I made and how they need to be made similarly. Feel free to ping me for any individual issues on individual formulae.

Again, I apologise for breaking some of this stuff. I wish I didn't need to and there's a certain amount of stuff that's gone on behind the scenes here that I wish I could elaborate on but unfortunately can't really for now. I know it sucks and I'll do whatever I can to help.

Owner

MikeMcQuaid commented Jan 24, 2014

I've updated all the stuff in all of the official taps and tested those I can. Should get you most of the way there.

Contributor

dakcarto commented Jan 24, 2014

@MikeMcQuaid For those of us maintaining third-party taps and formulae, I think it would be good if there were a running changelog of proposed or significant changes to the Homebrew core and tools, along with information on when API changes are going to take place and how to migrate. (Does this already exist somewhere?)

The more Homebrew is relied upon as the underlying framework to build software off of, the more useful such a one-stop news feed would be for maintainers, users and IT folk.

I'm thinking something relatively permanent like a Homebrew-owners blog, not temporal social media, like a Twitter. Following issues and pull requests is OK, but if I miss something like this issue, I would be in the dark about the changes and playing catch-up.

Owner

MikeMcQuaid commented Jan 24, 2014

We don't break formulae compatibility normally. This is a very unusual case, apologies for it.

Contributor

dakcarto commented Jan 26, 2014

@MikeMcQuaid I noticed your comment here, describing that the Python used is the first found on PATH. But now any system calls to python invariably return the system Python, even when Homebrew's Python is installed, and even if HOMEBREW_PREFIX/bin is prepended in the parent shell environ's PATH. This is the case for brew sh as well.

$ echo ${PATH}

/usr/local/bin:/usr/local/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/X11/bin

Install formula, with Homebrew's Python already installed:

  def install
    puts %x(which python) # returns /usr/bin/python
    puts %x(which python-config) # returns /usr/bin/python-config
  end

$ brew sh
...
$ echo ${PATH}

/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin:/usr/X11R6/bin

Is this the default now? PATH seems to be rewritten so that Homebrew's Python executable will never be found first. Then, what is the recommended method for always defaulting to Homebrew's Python, if installed, for a formula (as before)?

Edit: prepending PATH with /usr/local/opt/python/bin: also had no effect, as reported by @cstrahan here.

Owner

MikeMcQuaid commented Jan 26, 2014

My mistake; it seems this is blocked on Homebrew#26140 being merged.

@robotvert robotvert referenced this pull request Jan 27, 2014

Closed

itstool 2.0.2 #25678

Contributor

FlorianFranzen commented Jan 28, 2014

It is merged now, all issues are gone again.

@MikeMcQuaid MikeMcQuaid added a commit that referenced this pull request Jan 29, 2014

@MikeMcQuaid MikeMcQuaid python_dependency: cleanup and fix build env.
* Only set PYTHONPATH for Python 2.
* Set the Python binary for superenv.

References #24842.
Closes #26197.
Closes #26216.
Closes #26218.
Closes #26228.
9ead3a1

mmontag commented Feb 9, 2014

Hi, I still had some issues with the latest homebrew-science OpenCV formula; see Homebrew/homebrew-science#664
Thanks!

@ehershey ehershey added a commit to ehershey/homebrew that referenced this pull request Apr 4, 2014

@MikeMcQuaid @ehershey MikeMcQuaid + ehershey PythonDependency: massive refactoring.
Closes #24842.
d5b2d65

@ehershey ehershey added a commit to ehershey/homebrew that referenced this pull request Apr 4, 2014

@MikeMcQuaid @ehershey MikeMcQuaid + ehershey python_dependency: cleanup and fix build env.
* Only set PYTHONPATH for Python 2.
* Set the Python binary for superenv.

References #24842.
Closes #26197.
Closes #26216.
Closes #26218.
Closes #26228.
9f6d856

@marcoesposito1988 marcoesposito1988 referenced this pull request in ros/homebrew-hydro Aug 25, 2014

Closed

pyassimp formula broken by Homebrew update #24

@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 17, 2016

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