braintree / braintree_python

Braintree Python library
https://developer.paypal.com/braintree/docs/start/overview
MIT License
242 stars 115 forks source link

setup.py improvements and universal wheels #76

Closed adamchainz closed 7 years ago

adamchainz commented 7 years ago

Re-re-hash of the rejected #74 and #75.

Includes these changes:

danakatz commented 7 years ago

We're currently not looking to use setuptools or release as a wheel, but we'll be happy to accept the long description and spacing fix. Please update the commit and we can get this merged!

graingert commented 7 years ago

@danakatz This only uses setuptools if it's available

danpalmer commented 7 years ago

@danakatz why aren't you looking to release a wheel?

It's reasonable for a build process now to only support the use of wheels, and that would preclude a project from using Braintree. Plus this is a progressive enhancement on top of the current system that doesn't break backwards compatibility.

danakatz commented 7 years ago

Thanks for the input, @graingert and @danpalmer. We're not aware of this issue blocking anyone from integrating with Braintree, and it also adds dependencies to our internal release process.

However, we see the value of this feature and appreciate your focus on including it in the library. We'll consider revisiting and merging when we can!

graingert commented 7 years ago

I've released briantree for those that want a faster-cacheyer install experience.

Just pip uninstall braintree && pip install briantree

On 31 Oct 2016 19:46, "Dana Katz" notifications@github.com wrote:

Thanks for the input, @graingert https://github.com/graingert and @danpalmer https://github.com/danpalmer. We're not aware of this issue blocking anyone from integrating with Braintree, and it also adds dependencies to our internal release process.

However, we see the value of this feature and appreciate your focus on including it in the library. We'll consider revisiting and merging when we can!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/braintree/braintree_python/pull/76#issuecomment-257399428, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZQTKmNIVCUtVeE_6x0f-9pRRkuKvrJks5q5kWCgaJpZM4KgjrV .

danpalmer commented 7 years ago

@danakatz thanks for the update. I think it's a shame that a few packages are still not making wheels available, and I hope Braintree reconsiders soon so that the Python ecosystem can continue to move forward at a good pace.

I understand that this may complicate internal processes, but as the product here is so developer focused I think being a good citizen in the modern Python ecosystem is important. Avoiding this sends mixed messages to that developer community, and those of us considering integrating with Braintree.

adamchainz commented 7 years ago

In case you're wondering the difference that wheels can make:

  1. Install braintree in a fresh virtualenv, excluding its dependencies, and not using pip's cache:
$ mkvirtualenv test
...
$ time pip install --no-cache-dir --no-deps braintree==3.30.0
Collecting braintree==3.30.0
  Downloading braintree-3.30.0.tar.gz (57kB)
    100% |████████████████████████████████| 61kB 2.0MB/s
Installing collected packages: braintree
  Running setup.py install for braintree ... done
Successfully installed braintree-3.30.0
pip install --no-cache-dir --no-deps braintree==3.30.0  0.89s user 0.30s system 63% cpu 1.877 total
$ deactivate
  1. Installing @graingert's briantree wheel in the same environment:
$ rmvirtualenv test
Removing test...
$ mkvirtualenv test
...
$ time pip install --no-cache-dir --no-deps briantree==3.30.0
Collecting briantree==3.30.0
  Downloading briantree-3.30.0-py2.py3-none-any.whl (97kB)
    100% |████████████████████████████████| 102kB 1.6MB/s
Installing collected packages: briantree
Successfully installed briantree-3.30.0
pip install --no-cache-dir --no-deps briantree==3.30.0  0.50s user 0.26s system 63% cpu 1.206 total

So from around 0.9s to 0.5s, or nearly 50%! Multiplying this by the ~100 packages our build process installs, it's a big difference to our build process that gets run 100's of times a day. That's why I've opened this pull request, because I'm on a project to speed up our build process. Most of our key dependencies already had wheels but I've had pull requests merged on 10's of repos so far.

As Dan said, this is about being a good citizen in the ecosystem, and helping the developers that work with your product.

adamchainz commented 7 years ago

I've updated the build process in the Rakefile now too.

EvanHahn commented 7 years ago

We're going to close this PR due to inactivity, but let us know if you want us to reopen it and take another look after you make the requested changes.

graingert commented 7 years ago

@EvanHahn please reopen

demerino commented 7 years ago

@graingert Thanks for the quick feedback. Please take a look at the requested changes so we can try to merge this in.

Specifically

We're adding long_description with some custom information in another commit, revert this change.

Reopening 👐

graingert commented 7 years ago

It would be much better to switch to reStructuredText wholesale

adamchainz commented 7 years ago

@demerino ♻️ rebased

EvanHahn commented 7 years ago

Thanks for your changes! Glad we finally got them in.

We'll release a new version soon with these changes.

adamchainz commented 7 years ago

Thanks, look forward to wheel