-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add a pip check
command.
#1001
Add a pip check
command.
#1001
Conversation
Could you also write some tests for this new functionality. |
|
||
missing_requirements = get_missing_requirements(dist, installed) | ||
for requirement in missing_requirements: | ||
f.write("%s %s requires %s, which is not installed.\n" % \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nicer to use pip's logging system instead of sys.stdout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use backslashes in braces, line continuation is implied inside of them. Also what @hltbra said.
let's just make sure this get's discussed on pypa-dev before anyone merges, since it's a new top-level command. |
Thanks for the review. I'll make changes based on the feedback, add tests, and start a thread on pypa-dev. Any further code-related abuse welcomed, I'm not familiar with pip internals. |
I've started a thread on pypa-dev: https://groups.google.com/d/msg/pypa-dev/BI6mT9lkWF0/jmriRWOCQgkJ I'm going to make the additional changes as additional commits, unless anyone would like me to squash my commits. |
""" | ||
installed_names = set(d.project_name for d in installed_dists) | ||
|
||
missing_requirements = set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sets aren't ordered but you seem to use the return values in an iteration. This should be a list instead.
Also, can you put these functions into the check command class? There is no need to offer them as module level functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the functions to be methods.
I don't understand your point regarding sets. Sets aren't ordered, but they're iterable, and order isn't necessary here. I just used sets to ensure there aren't duplicates. Could you clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Wilfred.
Regarding using a set, get_installed_distributions iterates over the pkg_resources working set, which is a reprensentation of the currently installed packages, a list behind the scenes (https://bitbucket.org/pypa/setuptools/src/eb58c3fe7c0881cbb28de1c523f083ad8e7f427d/pkg_resources.py?at=default#cl-542). Being able to retain all information about this working set is very useful when you want to debug a situation with setuptools since it may add packages to that working set in a specific order due to the use of egg files et al. In other words, this subcommand should retain the order of requirements to be closer to the underlying technology.
Done. Let me know if there are further changes you'd like. Travis is showing one failing test: https://travis-ci.org/pypa/pip/builds/8419951 but this is an unrelated test that was fixed in this commit: 3ea56ea. |
def test_check_missing_dependency(): | ||
reset_env() | ||
# this will also install ipython, a dependency | ||
run_pip('install', 'ipdb==0.7') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're trying to convert over to local test packages, vs calling out to pypi.
ideally, you should engineer packages to go into tests/data/packages (or just include these)
e.g. see the use of pip_install_local
in test_install_wheel.py
and other files that installs packages from our local test data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otoh, @dstufft may soon have our tests spinning up a fake pypi, and we'd halt on converting to pip_install_local
, so maybe don't bother your time right now.
It's helpful to know that |
Thanks for the feedback; I'll update the pull request. |
This command ensures that all packages installed have all the requirements they need, and that requirements have compatible versions. This is useful because pip can install incompatible dependencies[1], or a user may have manually (un)installed a package. [1] pypa#775
OK, I've rebased on develop, updated the tests correspondingly, and squashed everything into one commit. I see the way to call pip in the tests has changed, are there further changes you'd like me to make to the tests? Re |
It's a shame this change seems to have stagnated, this check would be very useful in scripts. |
I'm still happy to do any further work to get it merged, FWIW. I'm not sure if the core team is still interested in the feature. |
IMHO I would prefer to solve the root cause by fixing pip to stop itself from producing such a broken state in the first place. |
The proper solution is #988, but I think that's a major undertaking. Even once it's fixed, there will be environments in a broken state as a result of using older pip versions (or easy_install, or manually changing files in the current virtualenv), so this feature would have value then. |
This needs to be updated because it's using |
This has a few problems with it that I noticed while pulling it down and testing it out. I'll try to update it, because it seems useful to me. |
Replaced by #2492 |
Accidentally closed this, reopening. Sorry! |
Hello! As part of an effort to ease the contribution process and adopt a more standard workflow pip has switched to doing development on the If you do nothing, this Pull Request will be automatically migrated by @BrownTruck for you. If you would like to retain control over this pull request then you should resubmit it against the If you choose to migrate this Pull Request yourself, here is an example message that you can copy and paste:
If this pull request is no longer needed, please feel free to close it. |
This Pull Request has been automatically migrated to #3745 to reparent it to the |
This command ensures that all packages installed have all the
requirements they need, and that requirements have compatible
versions. This is useful because pip can install incompatible
dependencies[1], or a user may have manually (un)installed a
package.
[1] #775
This is loosely inspired by Haskell's
ghc-pkg check
, which has a similar purpose.