-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix install failures that silently pass #5046
Conversation
…r not and exit with error when there are still failed dependencies
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 think we discussed this click\crayons issue before without a conclusion. Maybe we should consider not adding more of those.
pipenv/core.py
Outdated
crayons.red( | ||
f"Failed to install some dependency or packages. " | ||
f"The following have failed installation and attempted retry: {failed_list}" | ||
) |
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.
Why do we keep using click.echo(...crayons.{color}...
when click has native color support?
I believe click has a huge user base and good support for Linux and Windows terminals.
pipenv
s own crayons has some fixes to issues with crayons (which hasn't seen an update for a while).
However, I think we shoud consider it obsolete and not use it further. These fixes are probably for issues
which don't exist in click.
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'm not versed in click colors and was copying an existing pattern, but I am open to changing it here. I think you were talking about at one point being the one that would tackle a PR to remove crayons?
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.
Yes. I wanted to tackle that. But instead of a big PR, I think we should simply remove this pattern slowly, and avoid it in new code.
Click uses colorama and has easy API:
https://click.palletsprojects.com/en/7.x/utils/#ansi-colors
Tested the latest changes with the demo example:
|
It seems to me that pipenv can't install from VCS on Windows. Can you test this assumption?
|
@oz123 It works fine though without any changes the install commands from the test run fine on windows CLI. The tests themselves initially failed in pytest failed on my Windows because of the spinner, so I committed change that gets them to pass locally in pytest but they still fail in the CI for some unknown reason. The actual error the CI is getting back is meeting the condition |
@oz123 There is a chance that the new pip vendoring would actually fix these windows tests: pypa/pip#9452 |
Well unfortunately those 3 windows tests still fail in the CI and they pass locally in a terminal with these changes. I think the path forward is to mark skipping those 3 tests on windows and open a ticket to revisit getting them to work in the CI. To be clear, the install in the windows CI was always failing but the exit code was 0 because of this bug and fixing it revealed that behavior. |
The issue
Pipenv can fail to install dependencies but the return code doesn't represent this fact.
#5031
The fix
This checks at the end of the install routine after it has retried and cleaned up if there are still failed dependencies in the queue. If so it logs an error and exits with error.
The checklist
news/
directory to describe this fix with the extension.bugfix
,.feature
,.behavior
,.doc
..vendor
. or.trivial
(this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.