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

Relax dependency backports.functools-lru-cache to be >= 1.2.1 #499

Closed
wants to merge 1 commit into from

Conversation

rtdean
Copy link

@rtdean rtdean commented Nov 30, 2017

backports.functools-lru-cache is up to version 1.4. Because of how setuptools ends up calculating transitive dependencies, if you depend on something that depends on backports.functools-lru-cache (no version specified), and depend on something that depends on arrow that depends on backports.functools-lru-cache==1.2.1, you can end up with setuptools pulling in backports.functools-lru-cache at 1.4, and skipping/ignoring the ==1.2.1 requirement.

This then breaks execution, as package requirements aren't satisfied.

I've run the complete test suite on both linux and macos using 1.4 (latest), with no issue. And, of course, the travis builds for this pr should pull in 1.4 as well, because of the relaxed requirements.

fixes #497

@codecov-io
Copy link

codecov-io commented Nov 30, 2017

Codecov Report

Merging #499 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #499   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        3189   3189           
  Branches      229    229           
=====================================
  Hits         3189   3189

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66ac193...1c7228f. Read the comment docs.

setup.py Outdated
@@ -28,7 +28,7 @@ def grep(attrname):

install_requires = ['python-dateutil']
if sys.version_info[0] < 3:
install_requires.append('backports.functools_lru_cache==1.2.1')
install_requires.append('backports.functools_lru_cache==1.4')
Copy link

Choose a reason for hiding this comment

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

Would it be possible to have here backports.functools_lru_cache>=1.2.1?

Copy link
Author

Choose a reason for hiding this comment

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

I personally don't have any issue with >= (in fact, I think less stringent bounding in general is a good thing, until a conflict is found), however all the other documented requirements are a strict ==.

I will make the change and see what the maintainer says.

The default version of backports.functools-lru-cache is now 1.4.  Given
the... interesting... behaviour of setuptools when calculating
transitive dependencies, another package may pull in
backports.functools-lru-cache 1.4, prior to evaluating arrow's need for
backports.functools-lru.cache 1.2.1.

I've run all the test suites in macos and linux, and
backports.functools-lru-cache 1.4 works.

Accordingly, I'm relaxing the requirement for
backports.functools-lru-cache to be >= 1.2.1.
@rtdean rtdean force-pushed the fix-backports-lru-dependency branch from 3e069f4 to 1c7228f Compare November 30, 2017 21:00
@rtdean rtdean changed the title Switch to backports.functools-lru-cache 1.4 Relax dependency backports.functools-lru-cache to be >= 1.2.1 Nov 30, 2017
@MinchinWeb
Copy link
Contributor

Note that declaring dependencies like this via runnable code means you can run into issue when generating wheels, specifically that the code to determine your dependencies will be run when the when is generated rather than when the wheel is installed.

Two solutions:

  • don't provide wheels
  • use the extra_requires() method as outlined here

@papachoco
Copy link

Any ETA for a release w/ this patch?

@pganssle
Copy link
Contributor

pganssle commented Jan 2, 2018

@papachoco Per @MinchinWeb, I would say this patch should not be required at all, and that #504 should be accepted instead. This is the wrong way to handle conditional dependencies.

@ramonsaraiva
Copy link
Contributor

Yes let's continue with #504

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.

backports.functools-lru-cache
7 participants