Skip to content

req: Add warning of not recording VCS info with legacy 'setup.py install' #9208

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

req: Add warning of not recording VCS info with legacy 'setup.py install' #9208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
786b0df
req: Add warning of not recording VCS info with legacy 'setup.py inst…
Sep 16, 2021
d841b46
tests: Add test of warning of not recording VCS info with legacy 'set…
Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/9178.trivial.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add warning of not recording VCS info with legacy 'setup.py install'
32 changes: 20 additions & 12 deletions src/pip/_internal/req/req_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,18 +802,26 @@ def install(

self.install_succeeded = success

if success and self.legacy_install_reason == 8368:
deprecated(
reason=(
"{} was installed using the legacy 'setup.py install' "
"method, because a wheel could not be built for it.".format(
self.name
)
),
replacement="to fix the wheel build issue reported above",
gone_in=None,
issue=8368,
)
if success:
if self.legacy_install_reason == 8368:
deprecated(
reason=(
"{} was installed using the legacy 'setup.py install' "
"method, because a wheel could not be built for it.".format(
self.name
)
),
replacement="to fix the wheel build issue reported above",
gone_in=None,
issue=8368,
)
if self.link.is_vcs:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if self.link.is_vcs:
if self.original_link:

To better match the condition used to create direct_url.json ?

logger.warning(
"Direct URL of package '%s' will not be recorded when "
"using legacy 'setup.py install'.\n"
"Consider installing the 'wheel' package.",
Copy link
Member

Choose a reason for hiding this comment

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

Installing the wheel package may not be sufficient, as there are other reason for falling back to setup.py install (such as using --no-binary or --{install,global}-options. So we may want to drop this recommendation line.

self.name,
)


def check_invalid_constraint_type(req: InstallRequirement) -> str:
Expand Down
24 changes: 24 additions & 0 deletions tests/unit/test_req_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from pip._vendor.packaging.requirements import Requirement

from pip._internal.exceptions import InstallationError
from pip._internal.models.link import Link
from pip._internal.req.constructors import (
install_req_from_line,
install_req_from_req_string,
Expand Down Expand Up @@ -106,3 +107,26 @@ def test_install_req_from_string_with_comes_from_without_link(self):
assert install_req.link.url == wheel_url
assert install_req.req.url == wheel_url
assert install_req.is_wheel

def test_install_req_vcs_without_wheel_warning(self, caplog):
"""
Test to make sure that installing from VCS without wheel generates
a warning.
"""
req = InstallRequirement(
Requirement("gputil==1.4.0"),
comes_from=[],
link=Link("git+https://github.com/llamafilm/gputil")
)

req.source_dir = os.curdir
req.install([])
Copy link
Member

Choose a reason for hiding this comment

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

Uhhh, don't do this in a unit test... This has side effects that affect the interpreter it's running in. Specifically, it'll install the package when you run the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the correct way to do it? This is how it was done in test_req_file.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lemme know what you think @pradyunsg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @pradyunsg

Copy link
Member

@pradyunsg pradyunsg Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the ping! This got buried in my notifications. :(

Write an integration test that exercises this code path. Here's an example, that you'll likely need to change the name of the package for. Feel free to add the source code for a minimal reproducer package in tests/data/packages or use a script.pip("install", "...") for getting something minimal from the internet.

def test_install_vcs_something_something_triggers_a_warning(
    script,
    data,
):
    to_install = data.packages.joinpath("a-thing-that-triggers-this")
    result = script.pip_install_local(
        to_install,
        allow_stderr_warning=True,
    )
    msg = "Direct URL of package 'a-thing-that-triggers-this' will not be recorded when using"
    assert msg in result.stderr

Copy link
Member

Choose a reason for hiding this comment

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

See test_basic_install_editable_from_svn for an example of a local-only VCS test.


actual = [(r.levelname, r.message) for r in caplog.records]
expected = (
'WARNING',
"Direct URL of package 'gputil' will not be recorded when using"
" legacy 'setup.py install'.\n"
"Consider installing the 'wheel' package."
)
assert expected in actual