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

unbundle colorama package #110

Merged
merged 1 commit into from Jul 8, 2015

Conversation

Projects
None yet
2 participants
@marbu
Contributor

marbu commented Jul 5, 2015

I propose to:

  • remove bundled colorama package
  • add it into requirements.pip instead
unbundle colorama package
 * remove bundled colorama package
 * add it into requirements.pip instead
@tony

This comment has been minimized.

Member

tony commented Jul 7, 2015

Nice @marbu !

Colorama is in the realm where we'd be ok keeping it vendorized.

When vendorizing - several factors come into play. What comes to mind when / when not to vendorize an external lib:

  • How big is the package?
  • What's it's purpose?
  • How often does it change?
  • Is the maintainer open with development? (for instance, are they just uploading the package to Pypi with no VCS? Yeah, sometimes it happens!)
  • Does this save or create a headache for package maintainers (ports, deb, rpm, etc.) who wants to create packages?
  • Is the API interfaces and behavior stable?

To put it into context, pip vendorizes colorama as well https://github.com/pypa/pip/tree/develop/pip/_vendor/colorama.

Colorama is small. It doesn't change much. It's nice to have it inside the package because it saves a download.

Frankly I'd be more inclined to invite upgrading the vendorized version of colorama to 0.3.3. Would you be willing to do that?

On that note, if good reason can be given - I'm open to hearing the benefits in favor of de-vendorizing colorama.

@marbu

This comment has been minimized.

Contributor

marbu commented Jul 7, 2015

Thank you for your explanation! I will try to explain my reasoning below as well.

I'm actually working on packaging of tmuxp with hope to have it included into future fedora release, and so I need to follow Packaging:No Bundled Libraries rule. And from this point of view, not vendorizing colorama would make it easier for me, as:

  • Either: I don't have to ask Fedora Packaging Committee for an exception which would be likely rejected
  • or don't have to maintain this patch downstream while other distro's packagers would need to do the same

Moreover there already is fedora package or debian package for colorama.

While I'm not familiar with details of Debian packaging policies, I guess they have similar policy in place, so de-vendorizing colorama would make packaging easier for Debian and other distros as well.

I understand that sometimes there are legitimate reasons for vendorizing packages, but here I feel that the cost of the change is quite small: the colorama is already available on pypi so it can be easily installed with pip, which is the default installation method of this project anyway (so from the user point of view, there is no change).

When you say that colorama doesn't change much, it implies that the API is stable. This is another reason for de-vendorising as there is little risk of breaking something over time.

And last but not least, de-vendorizing colorama would make it easier for you to maintain the project, as you wouldn't need to update vendorized code now and then.

Speaking of pip, I think that this project is in quite unique position. Since it's a python package manager, it can't easily install it's own dependencies by itself so bundling some dependencies actually makes sense as it tries to overcome the chicken - egg problem.

@tony

This comment has been minimized.

Member

tony commented Jul 8, 2015

Thanks for articulating that. I thought vendorizing was ultimately doing a favor for packagers, apparently my belief was mistaken.

What does fedora do for pip? Pip has vendorized colorama.

I think I'm more inclined to merge this then. It's late here, I will come by and check on this tomorrow.

@tony

This comment has been minimized.

Member

tony commented Jul 8, 2015

Makes sense to me. Way to go @marbu!

tony added a commit that referenced this pull request Jul 8, 2015

Merge pull request #110 from marbu/unbundle_colorama
unbundle colorama package

@tony tony merged commit 28e0d3b into tmux-python:master Jul 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marbu

This comment has been minimized.

Contributor

marbu commented Jul 8, 2015

Thanks!

@tony

This comment has been minimized.

Member

tony commented Jul 8, 2015

@marbu Thank you! This is live in v0.9.0. 👍

@marbu marbu deleted the marbu:unbundle_colorama branch Jul 8, 2015

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