search

ckan / ckan-service-provider

A library for making web services that make functions available as synchronous or asynchronous jobs
http://ckan-service-provider.readthedocs.org
GNU Affero General Public License v3.0
21 stars 23 forks source link

pin dependency versions in setup.py #39

Closed gerbyzation closed 6 years ago

gerbyzation commented 6 years ago

fixes #38

s-chand commented 6 years ago

I think a better fix would be to also upgrade the implementation taking into consideration backward compatibility. For example:

try: import flask.ext.login as flogin except ImportError: import flask_login as flogin

gerbyzation commented 6 years ago

@s-chand Why do you prefer that? It solves this particular issue, but still leaves the risk of breaking the app in a similar fashion in the future. The purpose of version pinning is to freeze dependencies and give repeatable and reproducible deployments.

EDIT: sorry, it's not clear to me if you mean this in addition, or as a replacement of version pinning?

s-chand commented 6 years ago

@gerbyzation I actually meant in addition. Sorry I phrased it wrongly.

amercader commented 6 years ago

Thanks @gerbyzation

@s-chand I think it's safer to explicitly rely on the pinned versions

s-chand commented 6 years ago

@amercader of course. Thanks for looking into this.

davidread commented 6 years ago

I disagree with this. CKAN ecosystem has long used requirements.txt for pinning, not setup.py's install_requires. This will be biting people now, if not as soon as CKAN changes it's requests or flask version, say.

The reasoning is here: https://github.com/ga4gh/ga4gh-server/issues/1006#issuecomment-200474462 The official python version is here: https://packaging.python.org/discussions/install-requires-vs-requirements/

Sure, specify Flask<1.0 in setup.py since there is a specific incompatibility. (Or even better, fix the incompatibility). However by pinning every dep to a minor release in setup.py causes compatibility with different versions of CKAN.

davidread commented 6 years ago

Current CKAN master specifies sqlalchemy 1.1.11, so by this little library not installing unless it is <1.3.0,>=1.2.7, even though it works fine with a range of versions, makes no sense.

ckanserviceprovider 0.0.7 has requirement SQLAlchemy<1.3.0,>=1.2.7, but you'll have sqlalchemy 1.1.11 which is incompatible.
davidread commented 6 years ago

I've changed my mind on this - I'm happy with this PR, now that I've learnt datapusher (and this) should be installed in a separate virtualenv to CKAN (as discussed in #40).

Sorry for the disruption and for doubting you @gerbyzation & @amercader.

amercader commented 6 years ago

Please disrupt away @davidread, that's why we are here :)