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

Better git detection mechanism for pip freeze and pip list -e #3258

Merged
merged 9 commits into from Dec 12, 2015

Conversation

Projects
None yet
4 participants
@sbidoul
Contributor

sbidoul commented Nov 21, 2015

pip freeze and pip list -e do not correctly detect the source is under git control when setup.py is not at the repo root and/or the package_dir setup option is used.

This PR should fix #713 and #3186 at least in editable mode. This also complements #717.

The first commit refactors the vcs detection mechanism to let specific vcs implementations hook their own algorithm.

The second commit implements it for git using git rev-parse.

The third commit implements subdirectory support.

The last one adds the test.

Review on Reviewable

@sbidoul sbidoul changed the title from Better vcs detection mechanism to Better git detection mechanism for pip freeze and pip list -e Nov 22, 2015

src_path = subdir_path.join('src')
src_path.mkdir()
pkg_path = src_path.join('pkg')
pkg_path.mkdir()

This comment has been minimized.

@xavfernandez

xavfernandez Nov 24, 2015

Contributor

might be cleaner to directly use os.makedirs, cf https://github.com/pypa/pip/blob/develop/tests/unit/test_req.py#L43-L44

@xavfernandez

xavfernandez Nov 24, 2015

Contributor

might be cleaner to directly use os.makedirs, cf https://github.com/pypa/pip/blob/develop/tests/unit/test_req.py#L43-L44

@@ -486,6 +508,10 @@ def main():
entry_points=dict(console_scripts=['{name}={name}:main'])
)
""".format(name=name)))
return _vcs_add(script, version_pkg_path, vcs)

This comment has been minimized.

@xavfernandez

xavfernandez Nov 24, 2015

Contributor

Why should we always need a vcs in our tests ?

@xavfernandez

xavfernandez Nov 24, 2015

Contributor

Why should we always need a vcs in our tests ?

This comment has been minimized.

@sbidoul

sbidoul Nov 24, 2015

Contributor

@xavfernandez thanks for the review. I don't understand this comment. I simply split the existing _create_tests_package method in two, extracting the _vcs_add part in order to reuse it in _create_test_package_with_srcdir.

@sbidoul

sbidoul Nov 24, 2015

Contributor

@xavfernandez thanks for the review. I don't understand this comment. I simply split the existing _create_tests_package method in two, extracting the _vcs_add part in order to reuse it in _create_test_package_with_srcdir.

This comment has been minimized.

@xavfernandez

xavfernandez Nov 24, 2015

Contributor

my bad 😲

@xavfernandez

xavfernandez Nov 24, 2015

Contributor

my bad 😲

Show outdated Hide outdated pip/vcs/git.py Outdated
Show outdated Hide outdated pip/vcs/git.py Outdated
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Nov 24, 2015

Contributor

I globally like it. If at least an other dev could have a look 👍
@dstufft @qwcode @pfmoore

Contributor

xavfernandez commented Nov 24, 2015

I globally like it. If at least an other dev could have a look 👍
@dstufft @qwcode @pfmoore

@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Nov 30, 2015

Contributor

@xavfernandez I did some more testing, and I discovered that pip looks at the presence of a vcs to decide if a distribution is editable.

This means that, for example, when the developer creates it's development virtualenv inside the local clone, all distributions installed in the venv' site-packages are considered editable. That's not good, obvisously.

To solve that, pip must decouple the mechanism to decide if a distribution is editable from the dection of the vcs. And there is a TODO in the code for that [1].

Now my question is, how to detect that a distribution is editable? I understand from [2] that detecting the presence of the .egg-info directory could work. Any other idea or hint?

[1] https://github.com/pypa/pip/blob/develop/pip/utils/__init__.py#L308
[2] https://pythonhosted.org/setuptools/pkg_resources.html#overview

Contributor

sbidoul commented Nov 30, 2015

@xavfernandez I did some more testing, and I discovered that pip looks at the presence of a vcs to decide if a distribution is editable.

This means that, for example, when the developer creates it's development virtualenv inside the local clone, all distributions installed in the venv' site-packages are considered editable. That's not good, obvisously.

To solve that, pip must decouple the mechanism to decide if a distribution is editable from the dection of the vcs. And there is a TODO in the code for that [1].

Now my question is, how to detect that a distribution is editable? I understand from [2] that detecting the presence of the .egg-info directory could work. Any other idea or hint?

[1] https://github.com/pypa/pip/blob/develop/pip/utils/__init__.py#L308
[2] https://pythonhosted.org/setuptools/pkg_resources.html#overview

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Nov 30, 2015

Contributor

Now my question is, how to detect that a distribution is editable? I understand from [2] that detecting the presence of the .egg-info directory could work. Any other idea or hint?

You could check for the presence of a <your_package_name>.egg-link file in site-packages or check the distribution location for an usual place but I don't think there are clear identifiers...

Contributor

xavfernandez commented Nov 30, 2015

Now my question is, how to detect that a distribution is editable? I understand from [2] that detecting the presence of the .egg-info directory could work. Any other idea or hint?

You could check for the presence of a <your_package_name>.egg-link file in site-packages or check the distribution location for an usual place but I don't think there are clear identifiers...

@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Dec 1, 2015

Contributor

@xavfernandez I implemented the editable detection by looking at egg-link in sys.path (sbidoul@4f80195).

At first glance I'd say the 2 test failures should be fixed by adapting the tests.

What do you think?

Contributor

sbidoul commented Dec 1, 2015

@xavfernandez I implemented the editable detection by looking at egg-link in sys.path (sbidoul@4f80195).

At first glance I'd say the 2 test failures should be fixed by adapting the tests.

What do you think?

Show outdated Hide outdated pip/__init__.py Outdated
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Dec 2, 2015

Contributor

The egg-link check makes more sense to me than the search for a vcs but I'd like the advice of other @pypa/pip-developers

This would also need a changelog.

Contributor

xavfernandez commented Dec 2, 2015

The egg-link check makes more sense to me than the search for a vcs but I'd like the advice of other @pypa/pip-developers

This would also need a changelog.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Dec 3, 2015

Member

I'm confused. The original comment on the PR was that pip doesn't correctly detect if a project is under VCS control. But it seems to now be about detecting if a distribution is editable. You guys seem to know what you're talking about so I'm happy for you to continue working on this, but if you're looking for comments on the change, I'd need a summary of what the problem is, and what you want to do to fix it. (Bear in mind that I don't use -e much, and when I do it's largely irrelevant to me what pip freeze or pip list -e shows).

OTOH, if one of the other devs understands the issue and wants to comment, that's fine - don't bother with further explanations just for me :-)

Member

pfmoore commented Dec 3, 2015

I'm confused. The original comment on the PR was that pip doesn't correctly detect if a project is under VCS control. But it seems to now be about detecting if a distribution is editable. You guys seem to know what you're talking about so I'm happy for you to continue working on this, but if you're looking for comments on the change, I'd need a summary of what the problem is, and what you want to do to fix it. (Bear in mind that I don't use -e much, and when I do it's largely irrelevant to me what pip freeze or pip list -e shows).

OTOH, if one of the other devs understands the issue and wants to comment, that's fine - don't bother with further explanations just for me :-)

@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Dec 3, 2015

Contributor

@pfmoore correctly detecting if the project is under VCS control was indeed my first goal (so pip freeze works correctly).

Then I noticed that pip list -e used the VCS detection mechanism to determine that the distribution is in editable mode. So my fix changed the behaviour of pip list -e too.

This meant that in common situations where you have the following directory structure (with a venv for development inside the git clone):

myproject/venv/
myproject/.git
myproject/setup.py
myproject/src
...

all packages installed in venv were considered as editable because they are kind of "inside" the git clone.

So I had to resolve the TODO for editable detection so the whole thing is consistent, which I did by detecting the presence of egg-link in sys.path.

I hope this clarifies, let me know if not.

Contributor

sbidoul commented Dec 3, 2015

@pfmoore correctly detecting if the project is under VCS control was indeed my first goal (so pip freeze works correctly).

Then I noticed that pip list -e used the VCS detection mechanism to determine that the distribution is in editable mode. So my fix changed the behaviour of pip list -e too.

This meant that in common situations where you have the following directory structure (with a venv for development inside the git clone):

myproject/venv/
myproject/.git
myproject/setup.py
myproject/src
...

all packages installed in venv were considered as editable because they are kind of "inside" the git clone.

So I had to resolve the TODO for editable detection so the whole thing is consistent, which I did by detecting the presence of egg-link in sys.path.

I hope this clarifies, let me know if not.

@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Dec 3, 2015

Contributor

@xavfernandez thanks for the review, I'll fix the tests later this week.

Contributor

sbidoul commented Dec 3, 2015

@xavfernandez thanks for the review, I'll fix the tests later this week.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Dec 3, 2015

Member

@sbidoul OK, thanks. So, to summarise, this patch:

  1. Gives VCS implementations control over saying whether a project is under their control.
  2. Fixes pip list -e to use a more robust check for whether a distribution is editable than "is it under VCS control?"

I like (1), and having briefly checked how you've implemented it, it looks fine to me. (The Windows user in me is a bit sad at having an extra subprocess call in there, but that's hardly the fault of your code ;-)) I don't have any objection to (2), and it certainly needs to be done, but TBH I don't have the time to do a proper review of that code.

@xavfernandez I see the need to switch from "is it under VCS control" as a test for being editable. Looking for an egg-link file sounds messy but is probably the best option (really, it's something pkg_resources should provide, but I don't see anything from a quick check of the docs - maybe @jaraco can comment?).

Member

pfmoore commented Dec 3, 2015

@sbidoul OK, thanks. So, to summarise, this patch:

  1. Gives VCS implementations control over saying whether a project is under their control.
  2. Fixes pip list -e to use a more robust check for whether a distribution is editable than "is it under VCS control?"

I like (1), and having briefly checked how you've implemented it, it looks fine to me. (The Windows user in me is a bit sad at having an extra subprocess call in there, but that's hardly the fault of your code ;-)) I don't have any objection to (2), and it certainly needs to be done, but TBH I don't have the time to do a proper review of that code.

@xavfernandez I see the need to switch from "is it under VCS control" as a test for being editable. Looking for an egg-link file sounds messy but is probably the best option (really, it's something pkg_resources should provide, but I don't see anything from a quick check of the docs - maybe @jaraco can comment?).

refactor vcs backend detection mechanism
Give VCS implementations control over saying whether
a project is under their control
@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Dec 3, 2015

Contributor

@xavfernandez @pfmoore I rebased, cleaned up the commit history, fixed the tests and added the changelog entries.

Contributor

sbidoul commented Dec 3, 2015

@xavfernandez @pfmoore I rebased, cleaned up the commit history, fixed the tests and added the changelog entries.

Show outdated Hide outdated CHANGES.txt Outdated
Show outdated Hide outdated pip/utils/__init__.py Outdated
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Dec 3, 2015

Contributor

@sbidoul thanks a lot for your time.

I'd like to merge it soon but would love to first have more feedback on the new editable detection system.

Contributor

xavfernandez commented Dec 3, 2015

@sbidoul thanks a lot for your time.

I'd like to merge it soon but would love to first have more feedback on the new editable detection system.

@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Dec 4, 2015

Contributor

@xavfernandez I processed your last 2 comments.

Contributor

sbidoul commented Dec 4, 2015

@xavfernandez I processed your last 2 comments.

@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Dec 7, 2015

Contributor

So anyone else willing to review this?

I looked for alternative editable detection methods but could not find any better idea than looking for egg-links. In any case it works much better than the VCS detection method, and it should be quite safe.

Contributor

sbidoul commented Dec 7, 2015

So anyone else willing to review this?

I looked for alternative editable detection methods but could not find any better idea than looking for egg-links. In any case it works much better than the VCS detection method, and it should be quite safe.

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Dec 8, 2015

Contributor

@sbidoul I'll merge it this weekend if nobody objects in the meantime.

Contributor

xavfernandez commented Dec 8, 2015

@sbidoul I'll merge it this weekend if nobody objects in the meantime.

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Dec 11, 2015

Member

[Regarding] a test for being editable. Looking for an egg-link file sounds messy but is probably the best option (really, it's something pkg_resources should provide, but I don't see anything from a quick check of the docs).

I don't believe pkg_resources provides any indication if a package is "editable". The term editable comes from pip and is somewhat of a misnomer. After all, isn't a package installed in user-site-packages also editable by the user... or a package that's symlinked into system site packages from some user folder? I'm not even quite sure what you're seeking when you want to detect an editable package, so we'll want to clarify that... but if you're specifically seeking to identify packages that were installed by setup.py develop (which is also what pip -e uses), then the .egg-link is probably the most reliable indication.

The only code I can find in the setuptools code base that references the .egg-link file is in setuptools.package_index:scan_egg_links.

I don't disagree that pkg_resources could provide some abstraction here, but for now, that's the best the library has to offer. If adding some sort of egg-link discovery support to pkg_resources or setuptools would be helpful here, I welcome a proposal (preferably with patch) to the project.

Member

jaraco commented Dec 11, 2015

[Regarding] a test for being editable. Looking for an egg-link file sounds messy but is probably the best option (really, it's something pkg_resources should provide, but I don't see anything from a quick check of the docs).

I don't believe pkg_resources provides any indication if a package is "editable". The term editable comes from pip and is somewhat of a misnomer. After all, isn't a package installed in user-site-packages also editable by the user... or a package that's symlinked into system site packages from some user folder? I'm not even quite sure what you're seeking when you want to detect an editable package, so we'll want to clarify that... but if you're specifically seeking to identify packages that were installed by setup.py develop (which is also what pip -e uses), then the .egg-link is probably the most reliable indication.

The only code I can find in the setuptools code base that references the .egg-link file is in setuptools.package_index:scan_egg_links.

I don't disagree that pkg_resources could provide some abstraction here, but for now, that's the best the library has to offer. If adding some sort of egg-link discovery support to pkg_resources or setuptools would be helpful here, I welcome a proposal (preferably with patch) to the project.

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Dec 12, 2015

Contributor

Thanks for the clarification :)

Contributor

xavfernandez commented Dec 12, 2015

Thanks for the clarification :)

xavfernandez added a commit that referenced this pull request Dec 12, 2015

Merge pull request #3258 from sbidoul/better-vcs-check-sbi
Better git detection mechanism for pip freeze and pip list -e

@xavfernandez xavfernandez merged commit 441d0cf into pypa:develop Dec 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Dec 12, 2015

Contributor

@sbidoul thanks a lot for this PR 👍

Contributor

xavfernandez commented Dec 12, 2015

@sbidoul thanks a lot for this PR 👍

@sbidoul sbidoul deleted the sbidoul:better-vcs-check-sbi branch Dec 13, 2015

@sbidoul

This comment has been minimized.

Show comment
Hide comment
@sbidoul

sbidoul Dec 13, 2015

Contributor

Thanks @xavfernandez. If you have a moment, would you comment on #3291 as well as #3296 or #3292? These are the last two roadblocks towards having a fully working pip freeze | pip wheel -r /dev/stdin and replacing buildout in our workflow. I'm willing to provide PR's for those.

Contributor

sbidoul commented Dec 13, 2015

Thanks @xavfernandez. If you have a moment, would you comment on #3291 as well as #3296 or #3292? These are the last two roadblocks towards having a fully working pip freeze | pip wheel -r /dev/stdin and replacing buildout in our workflow. I'm willing to provide PR's for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment