Open cjerdonek opened 6 years ago
Should this distinguish between -e [remote/VCS URL]
and -e [local path]
? I agree it would be a very good idea to warn the user in the former case (I actively discourage people from doing it for roughly the same reason). In the latter one, however, it would be quite obvious (to me at least) the local path wouldn’t be removed by inspecting the file list in the prompt.
Yes, I agree. The former case is the more important, motivating case. For the latter case, I'd be open to suppressing (or perhaps changing) the message, assuming we can reliably distinguish those cases, etc.
I actively discourage people from doing it for roughly the same reason
Out of curiosity, what is it that you discourage -- installing VCS dependencies in editable mode, or just uninstalling them, or something else? If the second, what do you recommend for someone that wants to uninstall?
Oh, sorry I didn’t make it clearer. I discourage running install -e [VCS]
directly, and usually suggest doing a checkout manually, and install from it instead, i.e.
$ git clone git@github.com/pypa/pip.git ~/whatever/pip
$ pip install -e ~/whatever/pip
It just feels more straightforward to me, especially when the package is installed into a virtual environment (in which case the clone would go to $VIRTUAL_ENV/src
by default).
Oh, interesting. That's what I thought you might have meant. One reason I asked for clarification is that when I was perusing the pipenv docs, this sentence jumped out at me, which seems to be the opposite of what you're saying:
Note that it is strongly recommended that you install any version-controlled dependencies in editable mode, using pipenv install -e, in order to ensure that dependency resolution can be performed with an up to date copy of the repository each time it is performed, and that it includes all known dependencies.
from: https://pipenv.readthedocs.io/en/latest/basics/#a-note-about-vcs-dependencies
(This is a side-track from this issue, but I think that's okay.)
Ah, I see. I believe that section is actually talking about a different topic (I didn’t write it), comparing editable to non-editable VCS installs. Pipenv locks non-editable VCS checkouts to a specific commit when you install it, and some people are confused why the environment is not updated when they add new commits (because it is not editable, the environment is using a clone, not the repo directly), hence the suggestion.
IIUC, detecting if the package which is being uninstalled was installed in editable mode requires a check if dist
is editable at https://github.com/pypa/pip/blob/master/src/pip/_internal/req/req_uninstall.py#L453
I assume something like this would be a good starting point towards it. Please take a look at the same, and if it looks good, I can create a PR for it.
diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py
index 559061a6..72c0c401 100644
--- a/src/pip/_internal/req/req_uninstall.py
+++ b/src/pip/_internal/req/req_uninstall.py
@@ -18,6 +18,7 @@ from pip._internal.utils.misc import (
ask,
dist_in_usersite,
dist_is_local,
+ dist_is_editable,
egg_link_path,
is_local,
normalize_path,
@@ -462,6 +463,14 @@ class UninstallPathSet(object):
)
return cls(dist)
+ if dist_is_editable(dist):
+ logger.info(
+ "Uninstalling package %s installed in editable mode. "
+ "The source will not be removed, and will be located at %s",
+ dist.key,
+ dist.location
+ )
+
if dist_path in {p for p in {sysconfig.get_path("stdlib"),
Also the test will be to install a repo in editable mode, uninstall it, and check if the debug message contains the added info message.
The motivation for this issue is for -e [remote/VCS URL]
(which installs source in the target src
directory). With PEP 610 support in pip freeze, this use of editable may progressively fade away.
I also note that dist.location
points to the parent of the .egg-info
directory, which is not necessarily the root of what has been checked out.
The motivation for this issue is for -e [remote/VCS URL] (which installs source in the target src directory). With PEP 610 support in pip freeze, this use of editable may progressively fade away.
So considering this fact and if there is still benefit adding this warning in the longer run, I can still implement this change. Otherwise we can safely close this as well 😊
I also note that dist.location points to the parent of the .egg-info directory, which is not necessarily the root of what has been checked out.
When I implemented the mentioned changes, and tried out installing via a VCS URL in editable mode, and then uninstalling, I got the following (I tried my changes inside a virtualenv).
Also dist.location
does indeed point to /Users/devesh/pip/.env/src/master
, which has the .egg-info
directory, but it also contains the source code. Where would the location of the checked out source be then, if not dist.location
?
$ pip install -e git+https://github.com/pypa/twine#egg=master
Obtaining master from git+https://github.com/pypa/twine#egg=master
Cloning https://github.com/pypa/twine to ./.env/src/master
Running command git clone -q https://github.com/pypa/twine /Users/devesh/pip/.env/src/master
Installing build dependencies ... done
Getting requirements to build wheel ... done
Preparing wheel metadata ... done
WARNING: Generating metadata for package master produced metadata for project name twine. Fix your #egg=master fragments.
Requirement already satisfied: tqdm>=4.14 in ./.env/lib/python3.8/site-packages (from twine) (4.45.0)
Requirement already satisfied: setuptools>=0.7.0 in ./.env/lib/python3.8/site-packages (from twine) (41.2.0)
Requirement already satisfied: requests-toolbelt!=0.9.0,>=0.8.0 in ./.env/lib/python3.8/site-packages (from twine) (0.9.1)
Requirement already satisfied: pkginfo>=1.4.2 in ./.env/lib/python3.8/site-packages (from twine) (1.5.0.1)
Requirement already satisfied: requests>=2.20 in ./.env/lib/python3.8/site-packages (from twine) (2.23.0)
Requirement already satisfied: readme-renderer>=21.0 in ./.env/lib/python3.8/site-packages (from twine) (26.0)
Requirement already satisfied: keyring>=15.1 in ./.env/lib/python3.8/site-packages (from twine) (21.2.0)
Requirement already satisfied: chardet<4,>=3.0.2 in ./.env/lib/python3.8/site-packages (from requests>=2.20->twine) (3.0.4)
Requirement already satisfied: certifi>=2017.4.17 in ./.env/lib/python3.8/site-packages (from requests>=2.20->twine) (2020.4.5.1)
Requirement already satisfied: urllib3!=1.25.0,!=1.25.1,<1.26,>=1.21.1 in ./.env/lib/python3.8/site-packages (from requests>=2.20->twine) (1.25.9)
Requirement already satisfied: idna<3,>=2.5 in ./.env/lib/python3.8/site-packages (from requests>=2.20->twine) (2.9)
Requirement already satisfied: bleach>=2.1.0 in ./.env/lib/python3.8/site-packages (from readme-renderer>=21.0->twine) (3.1.4)
Requirement already satisfied: six in ./.env/lib/python3.8/site-packages (from readme-renderer>=21.0->twine) (1.14.0)
Requirement already satisfied: docutils>=0.13.1 in ./.env/lib/python3.8/site-packages (from readme-renderer>=21.0->twine) (0.16)
Requirement already satisfied: Pygments>=2.5.1 in ./.env/lib/python3.8/site-packages (from readme-renderer>=21.0->twine) (2.6.1)
Requirement already satisfied: webencodings in ./.env/lib/python3.8/site-packages (from bleach>=2.1.0->readme-renderer>=21.0->twine) (0.5.1)
Installing collected packages: twine
Running setup.py develop for twine
Successfully installed twine
$ pip uninstall twine -y
Found existing installation: twine 3.1.2.dev52+g2cec216
Uninstalling package twine installed in editable mode. The source will not be removed, and will be located at /Users/devesh/pip/.env/src/master
Uninstalling twine-3.1.2.dev52+g2cec216:
Successfully uninstalled twine-3.1.2.dev52+g2cec216
$ ls -l /Users/devesh/pip/.env/src/master
total 112
-rw-r--r-- 1 devesh staff 1212 Apr 26 14:42 AUTHORS
-rw-r--r-- 1 devesh staff 9695 Apr 26 14:42 LICENSE
-rw-r--r-- 1 devesh staff 14582 Apr 26 14:42 README.rst
drwxr-xr-x 10 devesh staff 320 Apr 26 14:42 docs
-rw-r--r-- 1 devesh staff 1099 Apr 26 14:42 mypy.ini
-rw-r--r-- 1 devesh staff 131 Apr 26 14:42 pyproject.toml
-rw-r--r-- 1 devesh staff 263 Apr 26 14:42 pytest.ini
-rw-r--r-- 1 devesh staff 1761 Apr 26 14:42 setup.cfg
-rw-r--r-- 1 devesh staff 661 Apr 26 14:42 setup.py
drwxr-xr-x 19 devesh staff 608 Apr 26 14:42 tests
-rw-r--r-- 1 devesh staff 1933 Apr 26 14:42 tox.ini
drwxr-xr-x 15 devesh staff 480 Apr 26 14:42 twine
drwxr-xr-x 8 devesh staff 256 Apr 26 14:43 twine.egg-info
AFAICT there is no way to tell, dist.location
is the closest we can get.
AFAICT there is no way to tell, dist.location is the closest we can get.
Indeed. With modern editables we'll get the project location in direct_url.json
.
AFAICT there is no way to tell,
dist.location
is the closest we can get.
Though from one of the line in the logs I pasted says Cloning https://github.com/pypa/twine to ./.env/src/master
, and dist.location
does point to it. Doesn't that mean dist.location
points to where the source is checked out?
@deveshks yes, that is a frequent case where the package installed is at the repo root. But this is not always the case. For instance, try the same with pip
itself, which uses src
layout. Or a package where pyproject.toml
is not at the repo root and where you need #subdirectory=
in the url.
AFAICT there is no way to tell, dist.location is the closest we can get.
Indeed. With modern editables we'll get the project location in
direct_url.json
.
Could you please expand on what a modern editable
is, and how is it different from a legacy editable
? Also I couldn't locate a direct_url.json
in the example I tried out. When is it generally created?
modern editable
@deveshks There is no tool doing that yet. The closest that exists today is https://github.com/takluyver/flit/pull/335.
Okay, so from this discussion, I gather that there is no sure-shot way yet to determine project location for a editable install via a VCS URL, until PEP-610 is implemented in pip, so we cannot show the source location in the warning message reliably in case of -e [remote/VCS URL]
currently.
Given that scenario, does it make sense to create a warning to just advice that the original source will still remain, and instead of doing install -e [VCS]
, a better approach is what is outlined at https://github.com/pypa/pip/issues/5835#issuecomment-425803178 (But then will this message still be helpful if they cannot locate how to remove the original source) ?
For instance, try the same with
pip
itself, which usessrc
layout.
So I tried doing the same with pip, and got the following (This was in a virtualenv where I am using pip for development purposes, and have installed pip from source earlier via pip install -e .
h$ pip install -e git+https://github.com/pypa/pip#egg=master
Obtaining master from git+https://github.com/pypa/pip#egg=master
Cloning https://github.com/pypa/pip to /Users/devesh/pip/.env/src/master
Running command git clone -q https://github.com/pypa/pip /Users/devesh/pip/.env/src/master
Installing build dependencies ... done
Getting requirements to build wheel ... done
Preparing wheel metadata ... done
WARNING: Generating metadata for package master produced metadata for project name pip. Fix your #egg=master fragments.
Installing collected packages: pip
Attempting uninstall: pip
Found existing installation: pip 20.1.dev1
Uninstalling package pip installed in editable mode. The source will not be removed, and will be located at /Users/devesh/pip/src
Uninstalling pip-20.1.dev1:
Successfully uninstalled pip-20.1.dev1
Running setup.py develop for pip
Successfully installed pip
So even though the source is cloned at /Users/devesh/pip/.env/src/master
, the warning shows dist.location
as /Users/devesh/pip/src
, which is the repo I have checked out for my development, so those two don't match. So is this a good example of dist.location not pointing to where the source is checked out
?
When uninstalling a package that was installed in editable mode, I think it would be helpful if the uninstall command would log that the original source still remains, and where it is. That way users will be less likely to have the mistaken impression that all the files were removed.