Skip to content

Fix #4292: Fix freeze when package occurs more than once #4293

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

Jump to bottom
Merged
merged 4 commits into from
Oct 31, 2017
Merged

Fix #4292: Fix freeze when package occurs more than once #4293

merged 4 commits into from
Oct 31, 2017

Conversation

eukaryote
Copy link
Contributor

@eukaryote eukaryote commented Feb 20, 2017 •
edited by pradyunsg
Loading

Tests and a fix for the behavior described in #4292, wherein 'pip freeze -r requirements.txt' displays a false warning says a package isn't installed if it appears twice in the requirements file.

Before the pull request, this is the behavior in a new venv:

$ pip install simplejson
$ echo simplejson > reqs.txt
$ echo simplejson >> reqs.txt
$ pip freeze -r reqs.txt
simplejson==3.10.0
Requirement file [req.txt] contains simplejson, but that package is not installed
## The following requirements were added by pip freeze:

The pull request adds two tests that catch this error due to (1) multiple in a single requirements file, or (2) multiple requirements file that contain the same package, and changes the behavior to log one warning for each package that appears more than once:

$ pip freeze -r reqs.txt
simplejson==3.10.0
Requirement simplejson declared multiple times [req.txt]
## The following requirements were added by pip freeze:

@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch 2 times, most recently from b30dfdd to 08ad817 Compare March 4, 2017 04:40
@eukaryote
Copy link
Contributor Author

All recent pull requests are failing due to an issue with appdirs. I'll verify that tests are passing for this PR after the appdirs issue is resolved.

@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch 2 times, most recently from e883132 to 1b73374 Compare March 12, 2017 00:09
@dstufft
Copy link
Member

dstufft commented Apr 1, 2017

You should be able to merge in master on this and have the appdirs issue fixed.

@dstufft dstufft added the needs rebase or merge PR has conflicts with current master label Apr 1, 2017
@BrownTruck BrownTruck removed the needs rebase or merge PR has conflicts with current master label Apr 1, 2017
dstufft
dstufft previously requested changes Apr 1, 2017
View reviewed changes
Copy link
Member

@dstufft dstufft left a comment

Choose a reason for hiding this comment

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

Please merge master into this PR to get tests working again (re-post for review system).

@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch 2 times, most recently from 2308fb5 to 7635eea Compare April 15, 2017 20:40
@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch from aa10215 to edbbdf5 Compare April 16, 2017 06:33
@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch from edbbdf5 to ffd587e Compare April 16, 2017 07:30
@eukaryote
Copy link
Contributor Author

eukaryote commented Apr 16, 2017 •
edited
Loading

I rebased my topic branch on master and the tests are now passing. I also added a news file describing the change..

@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch from 3136d37 to 1c8fa01 Compare April 22, 2017 13:48
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label May 20, 2017
@JelleZijlstra JelleZijlstra mentioned this pull request May 21, 2017
Closed
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 22, 2017
@eukaryote
Copy link
Contributor Author

eukaryote commented May 22, 2017 •
edited
Loading

I merged master again to resolve the conflict.
/request-review

@pypa-bot pypa-bot dismissed dstufft’s stale review May 23, 2017 02:20

Automated review dismissal at the request of @eukaryote

@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch from 44b5a3d to 5992474 Compare May 28, 2017 20:06
@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch from 5992474 to 46dfcc3 Compare June 2, 2017 14:59
@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch from 46dfcc3 to 3b21fc1 Compare June 14, 2017 14:34
@pypa-bot pypa-bot dismissed pradyunsg’s stale review July 2, 2017 17:39

Automated review dismissal at the request of @eukaryote

@eukaryote
Copy link
Contributor Author

By the way, isn't this a bug rather than an enhancement? 'pip freeze' is printing a warning that says the package is not installed even though it is installed.

@pradyunsg
Copy link
Member

Enhancement as in a bug fix.

@eukaryote
Copy link
Contributor Author

I'm not sure what you mean by "Enhancement as in a bug fix". Do you mean that this is not a bug for some reason but an enhancement, even though the current behavior says "package is not installed" when the package is actually installed?

In case it wasn't clear, I specifically was talking about the label 'kind - bug' vs the 'kind - enhancement'.

@pradyunsg
Copy link
Member

Okay, firstly, sorry for being so terse last time around.

The terminology I'm using here is - #4292 is a bug and this PR enhances pip by fixing that bug. Since that issue links to this PR, I think it's fine.

Anyway, if you still want a bug tag on this PR, just say so and I'll add it.

@eukaryote
Copy link
Contributor Author

Thanks for the explanation. Makes sense to me.

@eukaryote
Copy link
Contributor Author

eukaryote commented Jul 15, 2017 •
edited
Loading

@dstufft @pradyunsg Is there anything else I can do on this issue & PR in order to help move things along?

@pradyunsg
Copy link
Member

@eukaryote Sorry for the delay in responding.

This needs to be reviewed by someone from @pypa/pip-committers. So, I just pinged them. :)

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017
@eukaryote
tests illustrating incorrect warnings
138ad56
@eukaryote
update to track packages seen and issue warnings correctly
817cd9e 
 - keeps track of packages seen and which requirements file
   they were in so that the erroneous message about a package
   not being installed is not output and a message that identifies
   the files that contain the duplicate package can be output.
@eukaryote
add news entry file for issue pypa#4292 [pr pypa#4293]
60df29a
@eukaryote
six.iteritems instead of try-except for py-2/3 compat
ff42bdb
@eukaryote eukaryote force-pushed the freeze-not-installed-4292 branch from 52f8b23 to ff42bdb Compare September 3, 2017 01:23
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 3, 2017
pradyunsg
pradyunsg approved these changes Oct 14, 2017
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM.

I'll wait for someone else to review this too though.

@xavfernandez xavfernandez added this to the 10.0 milestone Oct 19, 2017
@pradyunsg pradyunsg added the C: freeze 'pip freeze' related label Oct 31, 2017
@pradyunsg pradyunsg merged commit 4e97d3d into pypa:master Oct 31, 2017
@pradyunsg
Copy link
Member

Thanks @eukaryote! 🎉

This was referenced Nov 3, 2017
Merged
Merged
kianasun pushed a commit to kianasun/pip that referenced this pull request Mar 28, 2018
@eukaryote @kianasun
Freeze not installed (pypa#4292, pypa#4293)
b7a10e7
@cjerdonek cjerdonek mentioned this pull request Feb 19, 2019
Closed
@cjerdonek cjerdonek changed the title Freeze not installed 4292 Fix #4292: Fix freeze when package occurs more than once Feb 19, 2019
@lock
Copy link

lock bot commented May 29, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

@dstufft dstufft dstufft left review comments

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: freeze 'pip freeze' related type: enhancement Improvements to functionality
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
6 participants
@eukaryote @dstufft @BrownTruck @pradyunsg @xavfernandez @pypa-bot