Skip to content
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

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

Merged
merged 14 commits into from
Mar 20, 2017

Conversation

KrzysztofMadejski
Copy link
Contributor

@KrzysztofMadejski KrzysztofMadejski commented Mar 2, 2017

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

@KrzysztofMadejski
Copy link
Contributor Author

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

@KrzysztofMadejski
Copy link
Contributor Author

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

@KrzysztofMadejski KrzysztofMadejski changed the title Requirements file shouldn't use >= Requirements update to match reality Mar 14, 2017
@davidread
Copy link
Contributor

The policy/strategy we have in ckan extensions is:

  • setup.py specifies the widest possible versions that are likely to work
  • requirements.txt pins the recommended versions

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
Copy link
Contributor Author

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
Copy link
Contributor

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:

  • setup.py is a basic safety check for obvious incompatibilities, required for every user
  • requirements.txt is for the newbie installing only one or two extensions and don't have conflict problems. The key thing is it is optional.
  • If requirements.txt is not working for you, write your own one.

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
Copy link
Contributor Author

KrzysztofMadejski commented Mar 15, 2017

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?

@KrzysztofMadejski KrzysztofMadejski changed the title Requirements update to match reality Setup.py should list dependencies and limit future major version updates Mar 15, 2017
@davidread
Copy link
Contributor

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
Copy link
Contributor Author

"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
Copy link
Contributor

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
Copy link
Contributor Author

KrzysztofMadejski commented Mar 17, 2017

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

What about #9 then?

@davidread
Copy link
Contributor

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

@KrzysztofMadejski
Copy link
Contributor Author

KrzysztofMadejski commented Mar 20, 2017 via email

@davidread davidread merged commit 900949f into ckan:master Mar 20, 2017
@davidread
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants