Skip to content

Conversation

razius
Copy link

@razius razius commented Aug 5, 2018

Because the packages in setup.py are really old and they use exact
version, when installing the package into a virtualenv it will end-up
downgrading already existing packages.

In the Python documentation it is also mentioned that it is not a best
practice to use install_requires to pin dependencies to specific versions.

See also:

Because the packages in `setup.py` are really old and they use exact
version, when installing the package into a virtualenv it will end-up
downgrading already existing  packages.

In the Python documentation it is also mentioned that it is not a best
practice to use `install_requires` to pin dependencies to specific versions.

See also:

- snowplow#195
- snowplow#198
- https://packaging.python.org/discussions/install-requires-vs-requirements/#install-requires
@coveralls
Copy link

coveralls commented Aug 5, 2018

Coverage Status

Coverage remained the same at 80.738% when pulling 8e522bf on unisport:master into 91da00a on snowplow:master.

@razius
Copy link
Author

razius commented Aug 5, 2018

Looks like Python 3.3 has reached EOL and is no longer supported, should I drop the test for it and update docs?

https://www.python.org/download/releases/3.3.0/

@snowplowcla
Copy link

Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://github.com/snowplow/snowplow/wiki/CLA to learn more and sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

@razius
Copy link
Author

razius commented Aug 6, 2018

I signed it.

@snowplowcla
Copy link

@razius has signed the Software Grant and Corporate Contributor License Agreement

Copy link

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'll let @chuwy have a look

@BenFradet BenFradet requested a review from chuwy August 6, 2018 10:13
"greenlet==0.4.10",
"requests==2.2.1",
"greenlet>=0.4.10",
"requests>=2.2.1",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would better to put upper bounds to be safe, but feel free to ignore this suggestion. For example Requests uses semver: http://docs.python-requests.org/en/master/dev/philosophy/#semantic-versioning

There is also ~= operator if you prefer it: https://www.python.org/dev/peps/pep-0440/#compatible-release

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the ~= will still downgrade if the minor version doesn't match so I'm not sure but an upper bound sounds like a very good idea.

What should the upper bound be? I can bump on our end to latest, try it out and put that as the upper bound.

Copy link

@asoltysik asoltysik Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For requests I think it's safe to put < 3.0.0 as they are using SemVer - for others I guess it depends what versioning scheme they are using?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also all for adding upper bounds. Of course we need to update the tracker more often, but last time unbounded dependencies caused a lot of pain as we simply didn't know what deps were used last time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for upper bounds

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@razius If we can get upper bounds on all these, this should be ready for merging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhadam , I'll try to find some time this weekend.

@mhadam mhadam self-requested a review September 22, 2018 03:52
@razius
Copy link
Author

razius commented Sep 22, 2018

I see this was done in 91da00a.

@razius razius closed this Sep 22, 2018
@mhadam
Copy link
Contributor

mhadam commented Sep 24, 2018

I see this was done in 91da00a.

I don't see any changes in setup.py, but maybe I'm missing something.

@mhadam mhadam reopened this Sep 24, 2018
@razius
Copy link
Author

razius commented Sep 24, 2018

@mhadam , wouldn't it be this ? 91da00a#diff-2eeaed663bd0d25b7e608891384b7298R77

@mhadam
Copy link
Contributor

mhadam commented Sep 24, 2018

I think what we're aiming for is that install_requires has upper bounds like "greenlet>=0.4.10, <1.0.0" (this follows from the thread with @asoltysik above)

So all of these need >= for the versions and proper upper bounds, I think most if not all follow semver.

@mhadam
Copy link
Contributor

mhadam commented Oct 1, 2018

closing this in favor of #209

@mhadam mhadam closed this Oct 1, 2018
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.

8 participants