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

add conditional dependicies for older versions of Python on tox #180

Merged
merged 1 commit into from Dec 21, 2017

Conversation

MinchinWeb
Copy link
Contributor

Requirements vary slightly between different version of Python. This allows tox to deal with that.

@MinchinWeb MinchinWeb mentioned this pull request Dec 9, 2017
@MinchinWeb
Copy link
Contributor Author

This fixes two immediate issues:

  • pytest dropped support for Python 3.3
  • arrow isn't installing a required dependency on Python 2.7

An alternate way to deal with this would be move the dependencies to the setup.py file, and use the conditional markers provided there.

@SpotlightKid
Copy link
Contributor

Alas, fixing the requirements for the tests to run is not enough. arrow is a run-time dependency, so we need to make sure that a Python 2.7 compatible version is installed alongside Watson. We could conditionally add a max version qualifier to the install_requires line in setup.py, but that would need a restructuring of the way we declare dependencies at the moment (they are parsed form requirements.txt). IMHO it's easier to just put the max version restriction for every Python version by having it in requirements.txt. As I see it, arrow releases 0.11.0 and 012.0 are simply broken.

The change to tox.ini setting the pytest version could be merged if the other two changes are removed.

@k4nar
Copy link
Collaborator

k4nar commented Dec 11, 2017

For Python 3.3, its support was ended a few month ago, so maybe we could remove it from tox?

I agree with @SpotlightKid for Python 2.7 & Arrow. This is very unfortunate though. We should consider removing Arrow for something more stable at some point (https://github.com/sdispater/pendulum for instance).

@MinchinWeb
Copy link
Contributor Author

I've submitted a PR to arrow to fix the issue.

Version 0.11 (of arrow) does appear to be broken, but version 0.12 does work, if you install from source (and not from a cached wheel generated by Python 3; pip shares a cache between all version installed on a machine). So this issue with arrow will never appear if you only have Python 2.7 on your system. To test this yourself, try install arrow on your system by ignoring the cache (pip2 install arrow==0.12 --no-cache). This is also way the issue never came up on Travis (see here where the arrow dependency is installed as expected).

So the issue is a weird combination of pip, wheel, arrow, and multiple Python versions installed on the same machine.


It may actually be possible to maintain the requirements.txt fill as currently used, and use conditional dependencies, if you're willing to require setuptools >= 36.2.1. (See the second example in the wheel documentation on conditional dependencies.)


I'm just leary of pinning a dependency to a maximum version like this because for me, watson is installed at a system level, and with the dependency pinned I can't upgrade it for anything else. I know that is discouraged on Linux systems, but I work on Windows and this works well, barring dependency hell, as there is to "system Python" to break.

@SpotlightKid
Copy link
Contributor

It may actually be possible to maintain the requirements.txt fill as currently used, and use conditional dependencies, if you're willing to require setuptools >= 36.2.1.

But the documentation also says:

Specifying extras via install_requires does not yet work with pip (v9.0.1 as of this writing).

Copy link
Contributor

@SpotlightKid SpotlightKid left a comment

Choose a reason for hiding this comment

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

I suggest merging only the last four added lines to tox.ini and removing the max version restriction for arrow again when there's a new arrow release.

@MinchinWeb
Copy link
Contributor Author

What if I just blacklist the "broken" versions of arrow?

@SpotlightKid
Copy link
Contributor

Yes, that would work too. But until there's a fixed arrow release it makes now difference to the way requirements.txt is atm, so I don't see a need to change it now.

@SpotlightKid
Copy link
Contributor

Ok, you've convinced me, I'll merge this as it is now. Let's hope that the next arrow version is really fixed! ;)

@SpotlightKid SpotlightKid merged commit 8806c91 into TailorDev:master Dec 21, 2017
@SpotlightKid SpotlightKid mentioned this pull request Dec 21, 2017
@MinchinWeb MinchinWeb deleted the conditional-tox branch December 23, 2017 16:34
@MinchinWeb
Copy link
Contributor Author

Awesome!

Here's the arrow PR to watch...

@MinchinWeb MinchinWeb mentioned this pull request Jun 5, 2018
@MinchinWeb
Copy link
Contributor Author

The update to arrow has been merged and released as version 0.12.1. PR #210 has been created to provide that change.

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.

None yet

3 participants