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

Formalize support for VCS-style requirements via PEP 440 #10728

Merged
merged 2 commits into from Sep 4, 2020

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Sep 3, 2020

Since 2016, users have been asking for a way to install wheels from a URL, e.g. from Git.

There are two approaches to VCS-style requirements:

  1. Pip's proprietary syntax, e.g. git+https://github.com/pypa/pip.git#egg=pip. This is what most people are familiar with. https://pip.pypa.io/en/stable/reference/pip_install/#vcs-support
  2. PEP 440's standardized "direct references", e.g. pip@ git+https://github.com/pypa/pip.git.

Turns out, ever since we upgraded to Pex 2.0, approach #2 has worked to achieve this goal. But few users knew about it (including us), and approach #1 has continued to not work.

We use setuptools to parse our requirements before passing to Pex. Why? We need to normalize it to, for example, strip comments from requirements.txt; and because we need the parsed information to do things like create a module mapping for dependency inference. However, pkg_resources.Requirement.parse() chokes on Pip's VCS style, and only understands PEP 440.

Both the Pip VCS and PEP 440 approaches achieve the same end goal. We could use some custom libraries like https://github.com/sarugaku/requirementslib and https://github.com/davidfischer/requirements-parser to allow for Pip's proprietary format, but that adds new complexity to our project.

Nevertheless, few users are familiar with PEP 440, so we eagerly detect if they're trying to use the Pip style and have a nice error message for how to instead use PEP 440:

▶ ./pants list 3rdparty/python: --no-print-exception-stacktrace
13:25:59.67 [ERROR] 1 Exception encountered:

  MappingError: Failed to parse 3rdparty/python/BUILD:
Invalid requirement 'git+https://github.com/django/django.git#egg=django' in 3rdparty/python/requirements.txt at line 32: Parse error at "'+https:/'": Expected stringEnd

It looks like you're trying to use a pip VCS-style requirement?
Instead, use a direct reference (PEP 440).

Instead of this style:

    git+https://github.com/django/django.git#egg=django
    git+https://github.com/django/django.git@stable/2.1.x#egg=django
    git+https://github.com/django/django.git@fd209f62f1d83233cc634443cfac5ee4328d98b8#egg=django

Use this style, where the first value is the name of the dependency:

    Django@ git+https://github.com/django/django.git
    Django@ git+https://github.com/django/django.git@stable/2.1.x
    Django@ git+https://github.com/django/django.git@fd209f62f1d83233cc634443cfac5ee4328d98b8

Closes #3063.

Additional benefit: doesn't allow --editable

With Pip VCS-style requirements, it's common to use -e or --editable, which changes how the distribution is downloaded so that the user can make edits to it. That is not sensible in Pants, and we are unlikely to ever support this feature.

PEP 440 style does not support -e, so, when users migrate to PEP 440, they won't have the opportunity to inadvertently use -e and for it to not work like they expect.

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano
Copy link
Contributor Author

FYI @darthveda-ai.

@coveralls
Copy link

coveralls commented Sep 3, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 681524f on Eric-Arellano:git-reqs into 5175b2b on pantsbuild:master.

@Eric-Arellano
Copy link
Contributor Author

Confirmed that this works after downloading the Django wheel locally :D

diff --git a/src/python/pants/util/BUILD b/src/python/pants/util/BUILD
index fcad8808d..c6609bbb9 100644
--- a/src/python/pants/util/BUILD
+++ b/src/python/pants/util/BUILD
@@ -3,4 +3,9 @@
 
 python_library()
 
-python_tests(name='tests')
+python_tests(name='tests', dependencies=[':req'])
+
+python_requirement_library(
+  name='req',
+  requirements=['Django @file:///Users/eric/Downloads/Django-3.1.1-py3-none-any.whl'],
+)
diff --git a/src/python/pants/util/strutil_test.py b/src/python/pants/util/strutil_test.py
index 791dde5d1..d34d69eb3 100644
--- a/src/python/pants/util/strutil_test.py
+++ b/src/python/pants/util/strutil_test.py
@@ -4,6 +4,7 @@
 import unittest
 from textwrap import dedent
 
+import django
 from pants.util.strutil import (
     ensure_binary,
     ensure_text,

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Sponsor Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Does pip itself support pep440-style?

@Eric-Arellano
Copy link
Contributor Author

Does pip itself support pep440-style?

Yep! That's how this all works. Pip supports both its proprietary thing and PEP 440. Behind the scenes, Pip is also doing special interpretation of the VCS constraints so that it understands references like @my_branch. The PEP doesn't say that that needs to be handled, only that installlers have the option to do it.

@Eric-Arellano Eric-Arellano merged commit 5886865 into pantsbuild:master Sep 4, 2020
@Eric-Arellano Eric-Arellano deleted the git-reqs branch September 4, 2020 00:12
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.

Support git+https / git+git style dependency in 3rdparty Python
3 participants