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

Add a pip check command. #1001

Closed
wants to merge 1 commit into from
Closed

Add a pip check command. #1001

wants to merge 1 commit into from

Conversation

Wilfred
Copy link
Contributor

@Wilfred Wilfred commented Jun 19, 2013

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.

@thedrow
Copy link

thedrow commented Jun 19, 2013

+1 looks great.

@pnasrat
Copy link
Contributor

pnasrat commented Jun 19, 2013

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" % \
Copy link
Contributor

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

Copy link
Member

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.

@qwcode
Copy link
Contributor

qwcode commented Jun 24, 2013

let's just make sure this get's discussed on pypa-dev before anyone merges, since it's a new top-level command.

@Wilfred
Copy link
Contributor Author

Wilfred commented Jun 24, 2013

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.

@Wilfred
Copy link
Contributor Author

Wilfred commented Jun 24, 2013

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()
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@Wilfred
Copy link
Contributor Author

Wilfred commented Jun 27, 2013

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')
Copy link
Contributor

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.

Copy link
Contributor

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.

@qwcode
Copy link
Contributor

qwcode commented Aug 14, 2013

It's helpful to know that pkg_resources.WorkingSet.resolve has all this logic, although it's written for another purpose, and it's written to raise exceptions when the first problem is found.

@Wilfred
Copy link
Contributor Author

Wilfred commented Aug 21, 2013

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
@Wilfred
Copy link
Contributor Author

Wilfred commented Aug 27, 2013

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 pkg_resources.WorkingSet.resolve, I found the documentation a little tricky to read, so I'd missed that. Looking at http://pythonhosted.org/distribute/pkg_resources.html#workingset-methods-and-attributes I can't see anything about .resolve raising exceptions. Sounds like WorkingSet.resolve isn't quite suitable here sadly.

@randfur
Copy link

randfur commented May 27, 2014

It's a shame this change seems to have stagnated, this check would be very useful in scripts.

@Wilfred
Copy link
Contributor Author

Wilfred commented May 27, 2014

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.

@Ivoz
Copy link
Contributor

Ivoz commented May 27, 2014

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.

@Wilfred
Copy link
Contributor Author

Wilfred commented May 27, 2014

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.

@msabramo
Copy link
Contributor

This seems like it would be useful. I sent a PR to @Wilfred to update a couple of small bugs:

Wilfred#1

@msabramo
Copy link
Contributor

msabramo commented Mar 6, 2015

This needs to be updated because it's using pip.log and that doesn't exist anymore.

767d11e

@msabramo
Copy link
Contributor

msabramo commented Mar 6, 2015

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.

@msabramo
Copy link
Contributor

msabramo commented Mar 6, 2015

OK, I would recommend closing this and looking at #2492 instead. It's @Wilfred's work but updated to work with the latest code on develop and refactored a bit.

@msabramo
Copy link
Contributor

msabramo commented Mar 7, 2015

Replaced by #2492

@dstufft dstufft closed this May 18, 2016
@dstufft
Copy link
Member

dstufft commented May 18, 2016

Accidentally closed this, reopening. Sorry!

@dstufft dstufft reopened this May 18, 2016
@BrownTruck
Copy link
Contributor

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 master branch. However, this Pull Request was made against the develop branch so it will need to be resubmitted against master.

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 master branch, closing and referencing the original Pull Request.

If you choose to migrate this Pull Request yourself, here is an example message that you can copy and paste:

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.

---

*This was migrated from pypa/pip#1001 to reparent it to the ``master`` branch. Please see original pull request for any previous discussion.*

If this pull request is no longer needed, please feel free to close it.

@BrownTruck
Copy link
Contributor

This Pull Request has been automatically migrated to #3745 to reparent it to the master branch. Please see the new pull request for any new discussion.

@BrownTruck BrownTruck closed this May 26, 2016
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.