-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix debug logging for build
with a build script
#8760
Conversation
32ae194
to
be332a4
Compare
I've eventually realized improving the third-party decoration logic in |
Thank you for going down this rabbit hole and providing such a detailed explanation.
Trying Without having taken a closer look, I'd suppose to have one test for the third party logging logic in
There are tests where we check the output via |
Good points and pointers @radoering , thanks for giving it a whirl under Windows. I'll see what I can do, despite Win not being readily available around my premises. |
The virtualenv creation resets any existing root logger, so the build command that uses a custom build script loses the ability to log anything below WARNING. Not mentioning the usual IOHandler gets swapped for the default stdout StreamHandler.
It turns out the logger name might not be enough for figuring out which third-party package is currently logging, because nothing guarantees such package even uses named loggers (or a sane naming convention). As the LogRecord lacks the name of the package inside a module, this change adds logic that parses it out of the LogRecord's pathname.
dcf6735
to
1f9cf13
Compare
Thank you @radoering for finishing up this PR, it's an honor. I'm sorry I wasn't able to be of a better further assistance. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Pull Request Check List
Resolves: #8756
Haven't seen any existing tests for log output 🤔 should I hack it nevertheless?
This shouldn't need any change.
This change aims to fix the muted debug level logs for builds with a custom build script.
Trying to understand in depth what the optional build script can do, I ran into a very scarce output despite using
-vvv
. Roaming the building/packaging code, I found out there's definitely some debug logging that might come in handy but is not getting out.First I found a piece of logging (a7a42905 by @abn) that lacked a newline. I tried to fix that in line with what I understood the original code tries to do (I still have a feeling I didn't hit the exact spot, but it sure works better now). It also seemed like a lost opportunity not to log the pip call there, so I added it for debug. It looks quite good as is, but we it might want to prefix those lines with something to make it clear it's a 3rd party log. Either way, this doesn't really tackle the original problem...
I found that despite the
poetry-core
builders' loggers have their log level set properly, they have no handlers of their own, so rely on the parent's (root logger's) custom handler set a couple lines above. This works nice, until something touches the root logger, which is exactly what happens by default in thevirtualenv.cli_run
call. With the defaultsetup_logging=True
it pretty much resets the root logger level toWARNING
(or to its desired verbosity inargs
, but that's not being used). I'm setting thesetup_logging=False
, which then adopts the root logger as is. That fixes the original problem, now every Poetry'sinfo
anddebug
logs pass through again, but also allinfo
anddebug
logs fromvirtualenv
hitch a ride, which is not the prettiest thing under the sun. But it's the debug logs we're after, right, so this might be a good thing!Either way, this surely requires at least your aesthetic opinion, as I'm not aware of any logging guideline you stand by. Thank you for helping me out.
PS: I think there's a way how to avoid the
virutalenv
's verbose logs.setup_logging=True
forvirtualenv
. But that tramples the root logger, which definitely sucks and seems hardly as a conceptual thing to do.Another way might be to monkey-patch theWon't work, they directly usevirtualenv.report.LOGGER
(root logger) to some logger we control.logging.info
etc. for logging 😞 that always goes back to the root logger.