Fix/5147 #5170

venthur · 2018-04-05T10:47:58Z

Suppress the output of the stack trace when pip install is called with an invalid requirement.

pradyunsg · 2018-04-05T11:04:50Z

Hey @venthur! Thanks for this PR!

We're currently in the beta period for pip 10.0, which means we won't be merging any new features or enhancements until pip 10.0.0 is released.

Sorry to keep you waiting but I hope you'd understand. :)

pradyunsg · 2018-04-05T11:06:54Z

src/pip/_internal/req/req_install.py

@@ -257,7 +257,7 @@ def from_line(
                elif '=' in req and not any(op in req for op in operators):
                    add_msg = "= is not a valid operator. Did you mean == ?"
                else:
-                    add_msg = traceback.format_exc()
+                    add_msg = ''


As I mentioned in #5147 (comment), it's probably not right to print the traceback or nothing here.

Could you look into extracting any useful info from the exception raised?

Hi @pradyunsg thanks for the review!

Well, we have

pip._vendor.pyparsing.ParseException: Expected W:(abcd...) (at char 0), (line:1, col:1)

at the end of the first stack trace and

pip._vendor.packaging.requirements.InvalidRequirement: Invalid requirement, parse error at "'-'"

at the end of the second one.

And of course the error message

Invalid requirement: '-'

I think for a user facing error message the Invalid requirement: '-' should be explanation enough for a user to fix the problem. The extra info about the actual problem -- the parser error -- does not really add value don't you think?

Looking at the issue it seems that @gvanrossum and @pfmoore had a similar fix in mind.

A quick clarification: the Error message Invalid requirement: '$FOO' message will still be shown with this patch.

I agree with @pradyunsg. I think it would be better to display something about why / what caused the requirement to be invalid as opposed to swallowing the details completely and just saying it's "invalid." For now this could be done simply by displaying the exception type and message for what caused it. For the case that was originally provided, this could look something like--

Invalid requirement: '-' Details: ParseException: Expected W:(abcd...) (at char 0), (line:1, col:1)

Over time, these inner causes could be made more user-friendly as they come up. For example, in this case it might be something like, "First character must be one of ...." (or whatever the condition is).

Very well, I've updated the PR and it prints out now:

$ pip install "toto 42" Invalid requirement: 'toto 42' Expected stringEnd (at char 5), (line:1, col:6)

and

$ pip install - Invalid requirement: '-' Expected W:(abcd...) (at char 0), (line:1, col:1)

I'm not sure if value of the extra info justifies the fragility of the implementation to get it: Python2 doesn't support the __context__ attribute yet and peeling out the message from the middle of a chained stack trace then seems not easy. So on Python2 it will simply print the Invalid requirement message without the additional hint.

$ pip install - Invalid requirement: '-'

The fact that you need to reach into the __context__ makes it seem like something else is wrong. The info should already be contained in the message for the InvalidRequirement exception that you are handling. If it's not, then it seems like that message should be adjusted. This would also address the issue for Python 2 because then you'll no longer need to access __context__ which isn't available. It will be available at the immediate exception being handled.

@cjerdonek Ok, that'll make things a lot easier, I wasn't sure if I'm allowed to modify stuff in src/pip/_vendor/

I just investigated, too. It looks like an issue would need to be filed with the packaging project to make that error message more informative. It's packaging that would need to provide a more detailed explanation since it's currently not providing that info:

pip/src/pip/_vendor/packaging/requirements.py

Line 95 in 0007825

raise InvalidRequirement(

So I guess @pradyunsg's option (2) is probably best here (combined with filing an issue with packaging).

PS - just to be clear, you're probably not allowed to modify what's in the _vendor directory (which is why it would need to be improved in the project itself). Stuff in the vendor directory is most likely copied as is from wherever it came from.

Ok, how about the current compromise:

$ pip install - Invalid requirement: '-' parse error at "'-'"

and

$ pip install 'toto 42' Invalid requirement: 'toto 42' parse error at "'42'"

The patch is very digestible and runs on all current Python's.

cjerdonek · 2018-04-06T11:53:36Z

src/pip/_internal/req/req_install.py

                if os.path.sep in req:
                    add_msg = "It looks like a path."
                    add_msg += deduce_helpful_msg(req)
                elif '=' in req and not any(op in req for op in operators):
                    add_msg = "= is not a valid operator. Did you mean == ?"
                else:
-                    add_msg = traceback.format_exc()
+                    msg = str(e)
+                    msg = msg.replace('Invalid requirement, ', '')


This replace() seems brittle and hacky to me, but I'll let others weigh in. The packaging project probably shouldn't be including this string to begin with since it's redundant with the exception class being InvalidRequirement.

Agreed I've created a PR against packaging fixing that: pypa/packaging#129

venthur · 2018-06-07T09:39:27Z

Refreshed the patch.

pradyunsg · 2018-06-07T10:15:37Z

Let's improve the error message in packaging and then we'll modify this to just print whatever that's printing.

venthur · 2018-06-07T10:52:29Z

@pradyunsg pypa/packaging#129

pradyunsg · 2018-06-08T06:06:19Z

@venthur Yep. I posted a comment there. :)

pradyunsg · 2018-07-06T21:51:02Z

Pushing this down the road to the next release since I don't think this'd happen in time for 18.0.

BrownTruck · 2018-08-21T16:30:03Z

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!

pradyunsg · 2018-09-20T05:55:48Z

Dropping this from 18.1 since I don't think this'll be ready in time (by 30th this month).

lock · 2019-05-31T14:47:59Z

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.

venthur added 4 commits April 5, 2018 12:34

Don't show stacktrace only error message on invalid requirement.

c47970f

Closes: pypa#5147

Added news fragment

f9e71b6

Fixed brain-o

fd7a5ec

Modified test for traceback to test that it is *not* there.

ff6b4e2

pradyunsg added the type: enhancement Improvements to functionality label Apr 5, 2018

pradyunsg modified the milestones: 10.0, 10.1 Apr 5, 2018

pradyunsg requested changes Apr 5, 2018

View reviewed changes

venthur added 3 commits April 6, 2018 11:51

Provide condensed error message

bdb931b

fail gracefully in python 2.x

9434932

Simplify

00e6689

cjerdonek reviewed Apr 6, 2018

View reviewed changes

venthur mentioned this pull request Apr 6, 2018

Improved error message in ParseException pypa/packaging#129

Merged

Merge branch 'master' into fix/5147

ece5c30

pradyunsg added the state: blocked Can not be done until something else is done label Jun 7, 2018

pradyunsg modified the milestones: 18.0, 18.1 Jul 6, 2018

BrownTruck added the needs rebase or merge PR has conflicts with current master label Aug 21, 2018

pradyunsg removed this from the 18.1 milestone Sep 20, 2018

psinghsodhi mentioned this pull request Oct 27, 2018

Don't print stacktrace due to invalid req #5923

Merged

pradyunsg closed this in #5923 Nov 8, 2018

lock bot added the auto-locked Outdated issues that have been locked by automation label May 31, 2019

lock bot locked as resolved and limited conversation to collaborators May 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/5147 #5170

Fix/5147 #5170

venthur commented Apr 5, 2018 •

edited

Loading

pradyunsg commented Apr 5, 2018

pradyunsg Apr 5, 2018

venthur Apr 5, 2018

venthur Apr 5, 2018 •

edited

Loading

cjerdonek Apr 6, 2018

venthur Apr 6, 2018

cjerdonek Apr 6, 2018

venthur Apr 6, 2018

cjerdonek Apr 6, 2018

cjerdonek Apr 6, 2018

venthur Apr 6, 2018

cjerdonek Apr 6, 2018

venthur Apr 6, 2018

venthur commented Jun 7, 2018

pradyunsg commented Jun 7, 2018

venthur commented Jun 7, 2018

pradyunsg commented Jun 8, 2018

pradyunsg commented Jul 6, 2018

BrownTruck commented Aug 21, 2018

pradyunsg commented Sep 20, 2018

lock bot commented May 31, 2019

Fix/5147 #5170

Fix/5147 #5170

Conversation

venthur commented Apr 5, 2018 • edited Loading

pradyunsg commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venthur Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venthur commented Jun 7, 2018

pradyunsg commented Jun 7, 2018

venthur commented Jun 7, 2018

pradyunsg commented Jun 8, 2018

pradyunsg commented Jul 6, 2018

BrownTruck commented Aug 21, 2018

pradyunsg commented Sep 20, 2018

lock bot commented May 31, 2019

venthur commented Apr 5, 2018 •

edited

Loading

venthur Apr 5, 2018 •

edited

Loading