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

Improve logging in the case of a failed legacy build. #6260

Merged
merged 1 commit into from
Feb 15, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Feb 12, 2019

Fixes #6259.

In at least one case, if the legacy build fails, it's not a critical failure, so this PR changes the log message from an error to a warning and gives a better explanation. I'm not sure if there are any cases where this failure results in a critical failure for pip. If we knew for sure that there weren't any such cases, we could tell the user that here.

The one thing I'm not sure about is whether the command output should be truncated since it's not a critical failure (in at least one case). My concern is the possibility of some users facing a wall of text on a regular basis. Before, the behavior was for this failure to be swallowed with no logging message, which doesn't seem right to me either.

@cjerdonek cjerdonek added the C: PEP 517 impact Affected by PEP 517 processing label Feb 12, 2019
@cjerdonek cjerdonek added this to the 19.0 milestone Feb 12, 2019
@cjerdonek cjerdonek added the skip news Does not need a NEWS file entry (eg: trivial changes) label Feb 12, 2019
@cjerdonek cjerdonek mentioned this pull request Feb 12, 2019
@cjerdonek
Copy link
Member Author

@pfmoore See here. This is a tweak to a recent fix for a regression in 19.0.2. My question: do you think the full command output for legacy build failures should be included in the logs? Would that be useful? Or should the output be truncated? My concern is a wall of text if there are any cases where builds have lots of output.

@pfmoore
Copy link
Member

pfmoore commented Feb 13, 2019

@cjerdonek I honestly don't know. We have no control over what output the build process produces so the "wall of test" problem is a very real one. On the other hand, if we suppress it we could end up in a situation where pip says "there's a problem" but gives the user no details on how to debug it. Obviously the user can always specify -vvv to get more output, so maybe we should assume that's sufficient, and keep the default output short.

(Much longer term, maybe PEP 517 backends should provide richer error details than just the raw stdout/stderr, but that will take a new PEP and a lot of backend changes, so that won't happen soon).

@cjerdonek
Copy link
Member Author

@pfmoore Okay, sounds good. Thanks. I'll tweak the PR to use a short message by default in both cases, and include the command output only when the log level is DEBUG. And to be clear, I'll make this change in both cases covered by this PR of (1) the build command exiting with 0 status code but creating no file, and (2) the build command exiting with 0 status code and creating more than one file (exactly one file is supposed to be created).

@pradyunsg
Copy link
Member

@cjerdonek Do you want me to hold off 19.0.3 for this PR? I am OK to do so.

@cjerdonek
Copy link
Member Author

@pradyunsg Yes, thank you. I'll be able to get to it about 12 hours from now, and it will be quick.

@pradyunsg
Copy link
Member

No hurries, I have a looong weekend so I'll be able cut a release whenever this is ready. :)

@cjerdonek cjerdonek force-pushed the issue-6259 branch 2 times, most recently from 1a014a0 to c74c683 Compare February 14, 2019 12:58
@cjerdonek
Copy link
Member Author

@pradyunsg Okay, I think this is good to go now.

@cjerdonek cjerdonek force-pushed the issue-6259 branch 2 times, most recently from a2db686 to 5f7ea19 Compare February 14, 2019 13:23
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.

The error message should suggest using --verbose instead of suggesting raising at logging level.

@cjerdonek
Copy link
Member Author

@pradyunsg Done.

@pradyunsg
Copy link
Member

Minor code style not of using single quotes in the newly added changes but not something that'd block approval from me.

@cjerdonek
Copy link
Member Author

Thanks for the review, @pradyunsg. I believe I was using single quotes everywhere I was able (the places where I'm not are strings that contain single quotes). That's normally my preference, but I'm not dogmatic about it.

@cjerdonek cjerdonek merged commit b6a2be0 into pypa:master Feb 15, 2019
@cjerdonek cjerdonek deleted the issue-6259 branch February 15, 2019 00:30
@pradyunsg
Copy link
Member

I was specifically refering to https://github.com/pypa/pip/pull/6260/files#diff-8eace9d800faa06b69c7a7ccd7571e36R192; where there's inconsistency.

@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.
Labels
auto-locked Outdated issues that have been locked by automation C: PEP 517 impact Affected by PEP 517 processing skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tweak fix for #6252
3 participants