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

Add helper class for Python virtualenvs #344

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@tdsmith
Copy link
Contributor

tdsmith commented Jun 12, 2016

  • 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 tests with your changes locally?

This PR proposes a new class for interacting with Python virtualenvs which enables a new way to write formulas for Python applications.

This approach resolves a few problems with the existing approach:

  • UX: Installing applications with this method yields clearer and much less code than the current method. If we trust pip to talk to pypi, we can pass a list of requirements instead of using resources, which means even less code.
  • Functional: In order that installing an application which uses Python packages does not disturb the global Python environment, we presently install packages and their dependencies into a Cellar-only prefix and write wrapper scripts that set PYTHONPATH to help Python find the installed tools. This generally works, with caveats. One is that namespace packages don't work very well, requiring formula authors to add touch foo/"__init__.py" manually. Another is PYTHONPATH propagates to child processes, which makes it difficult for a script installed with python2 to launch another script written in python3, since the Python 2 packages in PYTHONPATH may be incompatible. This has been a problem for e.g. Ansible. Virtualenvs do not have either problem.

This is functionally similar to pipsi. I expect this approach to work in general but I'll plan to ask some people in the Python packaging community for review.

Some highlights for review:

  • Where is the right place to store the virtualenv package's URL and hash? Should we just package a tarball in the Homebrew repository?
  • Is the hack I'm using to make sure that pip is invoked with Formula#system appropriate? I don't really like separating Virtualenv#create! from Virtualenv#initialize but we need an opportunity to monkeypatch the singleton class. :p I think I like this better than teaching the Virtualenv class about formula objects because they don't otherwise interact. Should this happen in Language::Python or is it more logical to write a Formula#python_virtualenv method that handles the hackery?
  • Can we usefully define tests for this class? (An example would be helpful!)
  • Does this belong in the evolution process?

Some work to do:

  • Make sure we do what we can to ensure that these don't break on revision or patch-level bumps for python3 virtualenvs -- python2 virtualenvs should be created against system Python and should be rock-solid

Thoughts welcome!

def create!
virtualenv_resource = Resource.new "virtualenv"
virtualenv_resource.url "https://files.pythonhosted.org/packages/5c/79/5dae7494b9f5ed061cff9a8ab8d6e1f02db352f3facf907d9eb614fb80e9/virtualenv-15.0.2.tar.gz"
virtualenv_resource.sha256 "fab40f32d9ad298fba04a260f3073505a16d52539a84843cf8c8369d4fd17167"

This comment has been minimized.

@xu-cheng

xu-cheng Jun 12, 2016

Contributor

Can we instead create a keg-only virtualenv formula, similar to our current sphinx-doc formula?

This comment has been minimized.

@tdsmith

tdsmith Jun 12, 2016

Contributor

We could; adding a virtualenv package that isn't intended to be used will probably be confusing to users, and that means interacting with the dependency system, which frightens me. It also means having to think harder about python/python3 interactions; this approach avoids letting python3 get out of sync with whatever version of python was used to install the virtualenv in the Cellar.

This comment has been minimized.

@xu-cheng

xu-cheng Jun 12, 2016

Contributor

I'm thinking we could handle Python/Python 3 in the similar approach as regular deps. For example, similar to Language::Hask::Cabal, we can have:

module Language
  module Python
    module Virtualenv
      def self.included(base)
        base.class_eval do
          if build.with? ("python3")
            depends_on "virtualenv" => [:build, "with-python3"]
          else
            depends_on "virtualenv" => :build
          end
        end
      end
    end
  end
end

And use it:

class Foo < Formula
  include Language::Python::Virtualenv
end 

This comment has been minimized.

@xu-cheng

xu-cheng Jun 12, 2016

Contributor

Also, personally, I think it's better to create a Virtualenv module for better readability.

This comment has been minimized.

@xu-cheng

xu-cheng Jun 12, 2016

Contributor

Or rethink a bit we could also do this if virtualenv formula is not of option:

module Language
  module Python
    module Virtualenv
      def self.included(base)
        base.class_eval do
          resource "virtualenv" do
            url ""
            sha256 ""
          end
        end
      end
    end
  end
end

But I think a standalone formula is still preferable.

This comment has been minimized.

@tdsmith

tdsmith Jun 18, 2016

Contributor

Thanks for the review, btw. Can you explain how the module technique would be different from the current implementation? I don't understand the distinction, sorry.

Why do you prefer making virtualenv a resource on the Formula class instead of instantiating a Resource directly?

This comment has been minimized.

@xu-cheng

xu-cheng Jun 20, 2016

Contributor

Can you explain how the module technique would be different from the current implementation?

The biggest advantage is it can help you solve the scope issue. By include Language::Python::Virtualenv inside Formula, you can use system normally which will be fall into the scope of that formula class. Beside, I think it helps with code readability and is more consistency with existing codebase.

Why do you prefer making virtualenv a resource on the Formula class instead of instantiating a Resource directly?

Because, the way you initialize resource directly involves private API in codebase, especially the owner hack.

This comment has been minimized.

@tdsmith

tdsmith Jun 20, 2016

Contributor

I see, re: scope; thanks. Ruby!

Because, the way you initialize resource directly involves private API in codebase, especially the owner hack.

The owner hack isn't great, right. If I change the Resource class to work correctly with a nil owner, would you still feel that instantiating a Resource is using API I shouldn't touch?

This comment has been minimized.

@xu-cheng

xu-cheng Jun 20, 2016

Contributor

If I change the Resource class to work correctly with a nil owner, would you still feel that instantiating a Resource is using API I shouldn't touch?

I would still prefer you not touch owner at all. It's very likely the internal owner logic will be changed in the future to support deps prioritize.

@tdsmith tdsmith force-pushed the tdsmith:virtualenv-support branch from fd917ff to d51b788 Jun 18, 2016

venv = Virtualenv.new venv_root, python
# Do some cute metaprogramming so that when `system` is invoked by the
# Virtualenv instance, `Formula#system` is called, so that we get the
# output capture and logging behavior we expect from formulae.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 19, 2016

Member

I think that's probably a good reason to just pull Formula#system into a utility function; there's been a few other places where we've wanted something similar.

This comment has been minimized.

@tdsmith

tdsmith Jun 19, 2016

Contributor

Are you envisioning something like system_for formula, "my_command"?

This comment has been minimized.

@tdsmith

tdsmith Jun 19, 2016

Contributor

— though if we do that it seems like we could just pass a Formula instance into the Virtualenv constructor and call formula.system.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 20, 2016

Member

Yeh, passing in the formula makes sense 👍

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 19, 2016

Installing applications with this method yields clearer and much less code than the current method. If we trust pip to talk to pypi, we can pass a list of requirements instead of using resources, which means even less code.

My concern is the code becomes pretty opaque as to what's actually being done. There's a reason we e.g. deprecated Formula subclasses previously and it was because Formula are designed to be as human readable as machine readable and this feels like a bit of a step back to me.

Virtualenvs do not have either problem.

I do agree that virtualenvs do feel like The Right Solution for this, though.

Where is the right place to store the virtualenv package's URL and hash? Should we just package a tarball in the Homebrew repository?

I'm not sure I understand what the virtualenv package is and what it would contain.

Should this happen in Language::Python or is it more logical to write a Formula#python_virtualenv method that handles the hackery?

I think splitting out Formula#system into a utility function would be best.

Can we usefully define tests for this class? (An example would be helpful!)

At least integration tests.

Does this belong in the evolution process?

Probably but given there's a sample implementation here I'm happy to discuss code over abstract concepts 😉

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 19, 2016

Where is the right place to store the virtualenv package's URL and hash? Should we just package a tarball in the Homebrew repository?

I'm not sure I understand what the virtualenv package is and what it would contain.

By "the virtualenv package" I mean that the virtualenv tool isn't part of the Python stdlib so we need to download it from Pypi at some point. The current implementation instantiates a Resource using a URL and checksum defined as constants on the Python module to fetch (and cache) it as needed.

ETA: @xu-cheng favors adding a keg-only virtualenv formula but I think this is an internal implementation detail and would prefer that it did not contaminate brew search, especially since it's hard to deliver a useful virtualenv formula since people expect it to be bound to their favorite Python.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 20, 2016

My concern is the code becomes pretty opaque as to what's actually being done.

I've tried to clean up the API a little bit; the only public methods now are pip_install!(target) and pip_install_and_link_scripts!(target, destination). target is passed through to pip unless target is a Resource, in which case it is staged and installed. This way, it's at least explicit that we're invoking pip. Does that help?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 20, 2016

If we trust pip to talk to pypi, we can pass a list of requirements instead of using resources, which means even less code.

Can you elaborate a bit more on what this might look like, even just with sample code that doesn't work yet?

By "the virtualenv package" I mean that the virtualenv tool isn't part of the Python stdlib so we need to download it from Pypi at some point. The current implementation instantiates a Resource using a URL and checksum defined as constants on the Python module to fetch (and cache) it as needed.

Feels like this should probably be a dedicated formula and/or depends_on :virtualenv. It'll otherwise produce pretty confusing errors when the download fails.

ETA: @xu-cheng favors adding a keg-only virtualenv formula but I think this is an internal implementation detail and would prefer that it did not contaminate brew search, especially since it's hard to deliver a useful virtualenv formula since people expect it to be bound to their favorite Python.

To me that at least points towards a depends_on :virtualenv.

pip_install!(target) and pip_install_and_link_scripts!(target, destination). target is passed through to pip unless target is a Resource, in which case it is staged and installed. This way, it's at least explicit that we're invoking pip. Does that help?

I like pip_install (but don't think we need the !) but pip_install_and_link_scripts feels like it's doing a bit too much.

As a general concern with this: we've done Clever Stuff with Python before and I just want us to be extra sure that this code is readable, maintainable and works well with system Python, Homebrew's Python and a random external Pythons (or at least just picks one and works well with that).

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 20, 2016

If we trust pip to talk to pypi, we can pass a list of requirements instead of using resources, which means even less code.

Can you elaborate a bit more on what this might look like, even just with sample code that doesn't work yet?

I've updated the example gist to reflect the current API: https://gist.github.com/tdsmith/f9e83e9ce66fa19349ac711b1c170462

One note is that pip normally maintains a per-user download cache but I expect that our $HOME munging will render that ineffective. We can manually set a cache directory to some persistent location, e.g. a child of HOMEBREW_CACHE or whatever it is, if the sandbox allows. NB the pip cache doesn't provide integrity guarantees, so allowing writes to the cache would permit malicious code to poison the cache on CI, so we'd want to explicitly clean that up in test-bot.

pip_install_and_link_scripts feels like it's doing a bit too much

The problem this tries to solve is that we don't want to link scripts for everything that's installed into the virtualenv: some dependencies of the application we're installing will also have scripts. These will all end up in the same place in the virtualenv. That means (I think) we want a method which can keep track of which scripts exist before and after we install our "real" application, and take care of linking the scripts we want into bin. The gist above shows the difference in action.

Feels like this should probably be a dedicated formula and/or depends_on :virtualenv. It'll otherwise produce pretty confusing errors when the download fails.

I think it's not so bad: https://gist.github.com/b18ee6238915542d9df2ae0816f94ff0 -- I'll work on a branch for :virtualenv but I think it's also nice that in the current implementation virtualenv is cached exactly once for however many formulas need it.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 20, 2016

NB the pip cache doesn't provide integrity guarantees, so allowing writes to the cache would permit malicious code to poison the cache on CI, so we'd want to explicitly clean that up in test-bot.

Does that mean it doesn't use checksums? If so, I think we'd need to pass on that.

I've updated the example gist to reflect the current API: https://gist.github.com/tdsmith/f9e83e9ce66fa19349ac711b1c170462

I think it would be nicer to have that take a hash of packages and their versions but it's otherwise pretty appealing in the simplicity.

That means (I think) we want a method which can keep track of which scripts exist before and after we install our "real" application, and take care of linking the scripts we want into bin. The gist above shows the difference in action.

Is there a way to have the virtualenv use different directories for this, by any chance?

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 20, 2016

NB the pip cache doesn't provide integrity guarantees, so allowing writes to the cache would permit malicious code to poison the cache on CI, so we'd want to explicitly clean that up in test-bot.

Does that mean it doesn't use checksums? If so, I think we'd need to pass on that.

pip doesn't re-verify the checksums against PyPI before using the cache; any local list of checksums would have the same trust properties as the cache.

I think it would be nicer to have that take a hash of packages and their versions but it's otherwise pretty appealing in the simplicity.

I like that, too.

ETA: The multiline text block is nice, though, because it's literally the output of pip freeze and you wouldn't need any additional helper tools like homebrew-pypi-poet to generate Homebrew-parseable code.

Is there a way to have the virtualenv use different directories for this, by any chance?

Welllllll I think so; pip doesn't support this directly (yet) but setuptools does and pip lets you pass through options to the underlying setup.py like --install-option="--install-scripts=/foo/bar". I'm not confident that actually works with virtualenvs but I can test. This is potentially not future-proof since there's work afoot to allow pip to work with build systems other than distutils/setuptools and this breaks the encapsulation around the underlying build systems. It probably works fine for everything that exists today.

@dstufft

This comment has been minimized.

Copy link

dstufft commented Jun 20, 2016

Couple thoughts here:

  • You can use venv on Python 3 which involves far less black magic and hackery than virtualenv does, and is less likely to break on patch upgrades because while virtualenv involves copying parts of the stdlib into the virtual environment, venv does not.
  • It's a little hacky, but you can bake hashes for the entire dependency tree into a requirements.txt file (possibly dynamically written?) so that you still get the same "verify sha256 hashes" that Homebrew itself gets and protect yourself from both a malicious cache and a malicious PyPI.
  • You probably don't want to diff the installed scripts because you're going to then link scripts from dependencies too, not just the top level. With a little bit of Python code you should be able to extract the names of all of the scripts for a particular package using the pkg_resources library.
  • Also, specific to virtualenv and not venv, what version of Python you have virtualenv installed with only really matters for what the default is for creating virtual environments. Beyond that you can easily create a virtual environment for different Pythons by using the -p flag. I think Debian has switched their virtualenv over to using Python3 but patched it so the default remains 2.x.
@dstufft

This comment has been minimized.

Copy link

dstufft commented Jun 20, 2016

pip doesn't re-verify the checksums against PyPI before using the cache; any local list of checksums would have the same trust properties as the cache.

Small bit of clarification, pip does verify hashes from PyPI, but that's also a HTTP request and thus can be cached in the same HTTP cache that package downloads themselves are in. So if someone is in a position to insert malicious packages into that cache they are also in a position to insert malicious checksums into that cache.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 20, 2016

Thanks for taking a look, Donald! One note; diffing installed scripts should be reliable since we're sending --no-deps to all of the pip invocations to force the dependency tree to be fixed in the formula.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 20, 2016

Added a note about pip_install! accepting version specs as text strings vs Ruby hashes to my comment above.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 20, 2016

pip doesn't re-verify the checksums against PyPI before using the cache; any local list of checksums would have the same trust properties as the cache.

When are the checksums ever verified; initial download? If that's the case it seems probably fine. After all, most people have the same write permissions on their cache and Library/Taps where the resource checksums are.

Welllllll I think so; pip doesn't support this directly (yet) but setuptools does and pip lets you pass through options to the underlying setup.py like --install-option="--install-scripts=/foo/bar". I'm not confident that actually works with virtualenvs but I can test. This is potentially not future-proof since there's work afoot to allow pip to work with build systems other than distutils/setuptools and this breaks the encapsulation around the underlying build systems. It probably works fine for everything that exists today.

I guess I'm curious how e.g. Debian and friends handle this situation as presumably they also don't want to dump everything in /usr/bin? Regardless, your current solution seems fine.

@dstufft

This comment has been minimized.

Copy link

dstufft commented Jun 20, 2016

diffing installed scripts should be reliable since we're sending --no-deps to all of the pip invocations to force the dependency tree to be fixed in the formula.

Oh, are you doing a multi-stage install then to install the "real" thing separately from it's dependencies? Diffing scripts will work in that case, I just assumed you were doing a single shot and done.

@dstufft

This comment has been minimized.

Copy link

dstufft commented Jun 20, 2016

When are the checksums ever verified; initial download? If that's the case it seems probably fine. After all, most people have the same write permissions on their cache and Library/Taps where the resource checksums are.

pip always verifies hashes if given, but those come from PyPI via HTTPS the same as package downloads. Our download cache is a transparent HTTP cache that doesn't speak anything pip or packaging specific. There's been an idea in my head about refusing to cache things that fail the initial hash check but that's mostly about preventing getting into a broken cache situation rather than anything security related, since (unless you're using the explicit --hash feature new in pip 8) the only hashes we have are the ones that come via PyPI that are also routed via this same caching mechanism.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 20, 2016

I guess I'm curious how e.g. Debian and friends handle this situation as presumably they also don't want to dump everything in /usr/bin? Regardless, your current solution seems fine.

I think they mostly don't. For system packages, Debian et al have scores of python-foo packages to handle dependencies and dpkg doesn't run pip at install time; distros can use their control files to curate whatever they want in /usr/bin. The distributions don't affect what happens when a user runs pip on their own.

@tdsmith tdsmith force-pushed the tdsmith:virtualenv-support branch 2 times, most recently from c9f6dca to 007b28e Jun 29, 2016

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 29, 2016

Pushed changes that reflect @xu-cheng's wish that I use Formula#resource instead of instantiating a Resource directly, and removed the exclamation marks on the Virtualenv methods.

Sample formula is here: https://gist.github.com/89f443db881a0cb0e8f3cc9fd0a57f82

I am not confident that I did a reasonable thing by creating the Virtualenv module. Does it make sense to have a Virtualenv class inside a Virtualenv module? I need to retain some state and I don't think I want to add instance variables to the Formula the module is being mixed in to so I think this is necessary. @xu-cheng, what do you think?

I thought about it for a few minutes and I don't know how to implement depends_on :virtualenv — i.e., a Requirement that sets up a(n ephemeral or permanent) virtualenv installation without a Formula on disk. I do regret hard-coding a URL and hash into core code. Since at that point we're effectively vendoring a remote resource, my favorite alternative is just vendoring the virtualenv tarball directly into the Homebrew/brew repository.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Jun 29, 2016

Does it make sense to have a Virtualenv class inside a Virtualenv module?

I think it's fine.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 29, 2016

Want to do more detailed review on this but won't be able to probably until next week. Just a request if this can hold off until then, thanks.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jun 29, 2016

Sounds good.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 3, 2016

@tdsmith Could you make a PR (which obviously will fail for now) for an example homebrew-core formula(e) refactoring using this so we can see what it looks like? Thanks 🙇

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 3, 2016

It's still here: https://gist.github.com/89f443db881a0cb0e8f3cc9fd0a57f82 -- do you need a PR?

include_dirs = [HOMEBREW_PREFIX/"include", Formula["openssl"].opt_include]
library_dirs = [HOMEBREW_PREFIX/"lib", Formula["openssl"].opt_lib]
include_dirs << Formula["sqlite"].opt_include
library_dirs << Formula["sqlite"].opt_lib

This comment has been minimized.

@tdsmith

tdsmith Jul 3, 2016

Contributor

This is an arbitrary set of formulas, cribbed from the python formula; should we expose all keg-only formulas?

This comment has been minimized.

@tdsmith

tdsmith Jul 3, 2016

Contributor

Nevermind; this is unnecessary under superenv! I forgot to declare a dependency when I was testing.

@tdsmith tdsmith force-pushed the tdsmith:virtualenv-support branch from 167526b to 5f10079 Jul 3, 2016

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 4, 2016

It occurs to me that venv.pip_install_and_link_scripts(".", bin) could also work like venv.link_scripts(bin) { venv.pip_install "." } which might be nicer.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 23, 2016

We'll just end up doing this as a workaround:

virtualenv_create(libexec) do |venv|
  virtualenv_install_with_resources
end

which is fine but it would be better to do

virtualenv_install_with_resources do
end

except when the first version is actually required in some particular circumstance.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 23, 2016

It looks like we also had two different opinions about what a method with that signature should do, which makes me a little warier. :)

Extending an API later is cheap; let's get some experience with this and see if we'll really use it.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 23, 2016

We already have the ability to take a block in install_cabal_package, and it is indeed used. I'm not sure why you'd deliberately cripple virtualenv_install_with_resources with less functionality before we even start seeing what works well and where.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 23, 2016

In the most complex case, this should be permissible:

virtualenv_create(libexec) do |venv|
  # blah
  virtualenv_install_with_resources do
    # blah blah
  end
  # blah blah blah
end
@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 24, 2016

Can you explain why you think it's axiomatic that everything should accept a block and what you think each method that accepts a block should do with that block? I don't think I understand the value of complicating the interface and the implementation.

Taking another look, actually, I don't remember why virtualenv_create yields to a block instead of just returning a venv instance.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 24, 2016

There's no plausible finalization action after we're done installing things into the virtualenv and no need to drag it out of scope so let's just return a Virtualenv instance instead of yielding a block with access to one. Fewer blocks, hurrah!

@tdsmith tdsmith force-pushed the tdsmith:virtualenv-support branch from f06bb88 to a8d0238 Jul 24, 2016

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 25, 2016

Can we clarify whether we're going to use pip freeze output style heredocs at all or not? Also, if we are, how/where should the pip freeze spec be placed in the formulae for the virtualenv_install_with_resources case?

Personally, I'm eager to see this PR 🚢 ASAP as it currently stands.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 25, 2016

Can we clarify whether we're going to use pip freeze output style heredocs at all or not?

Not without an evolution proposal, I'd say.

Thanks for all your time and for caring about the outcome. I'll do a final squash now and leave it open for just another couple of days in case there are any final comments.

@tdsmith tdsmith force-pushed the tdsmith:virtualenv-support branch from 94c61d2 to 3807579 Jul 25, 2016

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jul 27, 2016

Would be great if you'd be willing to follow-up in /core changing a handful of the most popular Python-using formulae to the new system, particularly jrnl since we point to it as the example.

On these set of changes alone though, I think it's a significant improvement on the status quo & would be happy to see it shipped. Appreciate your work here Tim.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 27, 2016

Thanks all!

@tdsmith tdsmith closed this in 9f6cb8e Jul 27, 2016

@tdsmith tdsmith removed the in progress label Jul 27, 2016

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jul 27, 2016

Dang, I missed the deadline. Would you mind granting me a grace period of 1-2 days before this gets deployed to formulae? This would allow me to make some comments and suggest some minor tweaks (that nonetheless could affect the public interface, hence this request).

If not, that's also fair … after all there was an advance warning that it will be merged today-ish.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 27, 2016

@tdsmith tdsmith deleted the tdsmith:virtualenv-support branch Jul 27, 2016

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Jul 27, 2016

I did make sure it was Wednesday in most relevant time zones. :) If you want changes can you make a PR by a firm date in the near future?

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jul 27, 2016

I did make sure it was Wednesday in most relevant time zones. :)

You are correct, sir! And the only one I'm blaming is me.

If you want changes can you make a PR by a firm date in the near future?

Yes, this was how I was planning to follow up, now that I missed the comment deadline.

@ilovezfs ilovezfs referenced this pull request Jul 30, 2016

Closed

Problems with virtualenv system & dependency formulae. #603

2 of 3 tasks complete

@UniqMartin UniqMartin referenced this pull request Jul 31, 2016

Merged

python: tweak script linking in virtualenv #613

8 of 10 tasks complete
@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jul 31, 2016

If you want changes can you make a PR by a firm date in the near future?

Yes, this was how I was planning to follow up, now that I missed the comment deadline.

Sorry for the unexpectedly long delay: #613

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Aug 25, 2016

wrt the wisdom of fixing dependencies, the popular lxml package recently broke OS X builds on a patch release: https://bugs.launchpad.net/lxml/+bug/1614693

@ilovezfs ilovezfs referenced this pull request Dec 6, 2017

Closed

ddgr 1.1 (new formula) #21370

4 of 4 tasks complete

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

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