ckan / ckanext-archiver

Archive CKAN resources
MIT License
21 stars 46 forks source link

Setup.py should list dependencies and limit future major version updates #33

Closed KrzysztofMadejski closed 7 years ago

KrzysztofMadejski commented 7 years ago

Requirements file shouldn't use >=, it can lead to automatically updating libs with backward incompatible changes

KrzysztofMadejski commented 7 years ago

Now I've read https://caremad.io/posts/2013/07/setup-vs-requirement/ and I think requirements should be put in setup.py.

KrzysztofMadejski commented 7 years ago

Also incorporating #34 (dependency on ckanext-report) https://caremad.io/posts/2013/07/setup-vs-requirement/

davidread commented 7 years ago

The policy/strategy we have in ckan extensions is:

The key difference between the two mechanisms is that setup.py is forced upon you when you install the module, so should only force you when there is a genuine version incompatibility.

So yes, I'm up for changing the >= in requirements.txt to ==. However I disagree with you being overspecific in the setup.py. For example you'd break every install that uses requests 2.7.0, which is pretty common, and yet is perfectly compatible with ckanext-archiver.

I guess you've discovered that one of these requirements has a new version with backward incompatibilities, and it would be good to add that into setup.py with a < clause.

KrzysztofMadejski commented 7 years ago

The problem with requirements file is that one plugin can specify one version, other another version and you would have to inspect pip install -r logs to see if one version overwrites the other and then guess if it's ok for this or that plugin to run with that version.

If you put versions in setup.py, then you can check if many plugins don't have conflicting requirements with pip check. Otherwise it's hard to maintain in my opinion. Or maybe you can recommend some tools that help with that?

I'm all for setup.py specifies the widest possible versions that are likely to work, but I believe in practice it should be done with caution: 1) Specify package~=1.0.1 or package~= to just install "safe updates". 2) If one needs features from higher version setup.py can be changed for example to package>=1.0.1, package<3.0 (after testing it).

In this approach developer extending functionality is required to test and bump versions. In the other you get issues saying "this plugin doesn't work".

davidread commented 7 years ago

The problem with requirements file is that one plugin can specify one version, other another version

Correct, and in this case the advice is for you to ignore the requirements file if you want. Create your own one and pip install that instead.

If you change the setup.py to exclude any perfectly working versions then you're going to annoy every developer who uses that version and who is now required to fork the code and change the setup.py before even installing it.

Put it another way:

You're focussed on the safety of people writing a custom requirements file, whereas I think they should just be allowed to try whatever they like, without resorting to writing a PR for setup.py.

KrzysztofMadejski commented 7 years ago

If you change the setup.py to exclude any perfectly working versions then you're going to annoy every developer who uses that version and who is now required to fork the code and change the setup.py before even installing it.

So if I use current available pip package versions to limit setup.py that won't be a problem. Ie. current requests is 2.13.0, so putting <3.0 will be safe regarding existing installations.

setup.py is a basic safety check for obvious incompatibilities, Bumping a major version is in my opinion marking an obvious incompatibility.

setup.py specifies the widest possible versions that are likely to work Coming back to requests example: requests 3.0 is imho unlikely to work

I've modified code limiting in setup.py only future major versions upgrades. Also modified title ;) Does it make sense now?

davidread commented 7 years ago

So if I use current available pip package versions to limit setup.py that won't be a problem. Ie. current requests is 2.13.0, so putting <3.0 will be safe regarding existing installations.

Only if you can demonstrate that requests 3.0 will definitely break ckanext-archiver. Otherwise, I think it would be preferable to not put false restrictions on users.

KrzysztofMadejski commented 7 years ago

"A major release will include breaking changes" -> http://docs.python-requests.org/en/master/community/release-process/.

Or is your argumentation that it will only possibly break archiver and as such shouldn't be limited?

davidread commented 7 years ago

Yeah, it's about whether something that we use gets broken. When we upgraded requests from 1.1 to 2.3 on ckanext-archiver we suffered no breakage.

It's simply less hassle to maintain if we are liberal, rather than conservative with setup.py.

KrzysztofMadejski commented 7 years ago

When we upgraded requests from 1.1 to 2.3 on ckanext-archiver we suffered no breakage.

What about https://github.com/ckan/ckanext-archiver/issues/9 then?

davidread commented 7 years ago

Ok good point, but better to fix little things in the code like that, than force people to fork to use newer versions.

KrzysztofMadejski commented 7 years ago

Ok, thanks for the discussion. I will correct this PR. I'm also planning next one with i18n for archiver and report.

Krzysztof

On 20 March 2017 at 10:45, David Read notifications@github.com wrote:

Ok good point, but better to fix little things in the code like that, than force people to fork to use newer versions.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ckan/ckanext-archiver/pull/33#issuecomment-287713339, or mute the thread https://github.com/notifications/unsubscribe-auth/AAz4N2Lz6LAEJjdUOL0iyJ4QWV80ihzAks5rnkqpgaJpZM4MRMeW .

-- Krzysztof Madejski Product Manager at TransparenCEE.org http://transparencee.org/ Code for Poland http://codeforpoland.org Program Manager / Koordynator Koduj Dla Polski http://kodujdlapolski.pl krzysztof.madejski@epf.org.pl / +48 664 082 823

http://pdfcee17.pl/ http://pdfcee17.pl

davidread commented 7 years ago

@KrzysztofMadejski Yes good to discuss. I've merged the parts of changes that follow our general strategy, which leaves out a few things: https://github.com/ckan/ckanext-archiver/commits/master But it's still a useful improvement to what there was before, so many thanks for that.