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

Updated piptools to only lock compatible packages #2430

Merged
merged 13 commits into from
Jun 29, 2018
Merged

Conversation

techalchemy
Copy link
Member

@techalchemy techalchemy commented Jun 26, 2018

@techalchemy techalchemy added Type: Bug 🐛 This issue is a bug. Category: Dependency Resolution Issue relates to dependency resolution. Type: Regression This issue is a regression of a previous behavior. labels Jun 26, 2018
@techalchemy techalchemy added this to the 2018.6.26 milestone Jun 26, 2018
@@ -21,16 +22,20 @@
def clean_requires_python(candidates):
"""Get a cleaned list of all the candidates with valid specifiers in the `requires_python` attributes."""
all_candidates = []
py_version = parse_version(os.environ.get('PIP_PYTHON_VERSION', str(sys.version_info[:3])))
Copy link
Contributor

@frostming frostming Jun 27, 2018

Choose a reason for hiding this comment

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

parse_version(str(sys.version_info[:3]) is not going to work(at least from my testing). Should use '.'.join(map(str, sys.version_info[:3]))

Besides, os.environ['PIP_PYTHON_VERSION'] is a string value 'False', it needs more handling.

@frostming
Copy link
Contributor

I will send a PR to this branch to include a test case 😄

@techalchemy
Copy link
Member Author

Lol. It never fails since it’s just a fallback but it’s good to have a working fallback so thanks!

- Fixes #1901

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
except InvalidSpecifier:
continue
else:
if not specifierset.contains(py_version):
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always be true, because os.environ.get('PIP_PYTHON_VERSION') = 'False'

Please merge #2437 and the CI will pass

Copy link
Member Author

Choose a reason for hiding this comment

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

Still failing :(

@frostming
Copy link
Contributor

frostming commented Jun 27, 2018

I didn't see the testing output until just now. It's my fault not noticing the mocked pypi doesn't have requires_python meta at all. 😞

Need to find a better way to do the testing. We could consult the real PyPI through internet or just remove the test case.



@pytest.mark.lock
def test_lock_respecting_python_version(PipenvInstance):
Copy link
Contributor

Choose a reason for hiding this comment

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

@pytest.mark.needs_internet

django_version = '==2.0.6' if six.PY3 else '==1.11.10'
c = p.pipenv('lock')
assert c.return_code == 0
assert p.lockfile['default']['django']['version'] == django_version
Copy link
Contributor

@frostming frostming Jun 27, 2018

Choose a reason for hiding this comment

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

For internet PyPI, should update this to test if django_version[:3]=='==2'(PY3) and django_version[:3]=='==1'(PY2)

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Dependency Resolution Issue relates to dependency resolution. Type: Bug 🐛 This issue is a bug. Type: Regression This issue is a regression of a previous behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants