Fix verbose output being captured as command output #147
Conversation
Alien::Base::ModuleBuild::do_command captures the output from Module::Build::Base::do_command, but the later may introduce debugging output when Module::Build verbose mode is set. This patch disables Mode::Build verbose mode before calling its do_command method.
This bug appears when calling |
Is stuff breaking because there is extra diagnostics in the captured output? |
That extra output appears later on the value of In general, any code calling do_command in order to get the output from some external command would probably expect that output to be unaltered and may be confused by any extra data. |
It is sometimes hard to completely avoid contamination of output, for example if PkgConfig.pm throws a warning, or if a LD_PRELOADd library starts spewing debug messages or something. Anything parsing the output should make some effort to contend with that possibility, although it probably doesn't. I'm a little leery of turning off verbosity, but not totally against it. If you can show actual breakage then there would be a stronger case for it. |
Note that this patch only disables verbose mode when calling do_command and afterwards, immediately restores it. As currently implemented in Module::Build, the only message actually suppressed is the command to be run and it is dumped anyway by Alien::Base::ModuleBuild::do_method. |
I'm going with Graham on this one: when he's happy, I'm happy, not before. |
I looked at the code and my read is that turning of verbose will probably produce some false positives, in that AB::MB may think that a library is installed when it isn't if you run in verbose mode. I haven't been able to test this myself yet. Easy fix is the one that is proposed, and as @salva mentioned, the current implementation only prints out the command.
I think the more complicated, but more future proof answer is to override the log_* methods so that we can save any output and print them out after the capture. This way users expecting verbosity will get it and if future verbosity is added we don't silence it. |
To be sure I am not super crazy about either and am open to alternatives. |
Another solution would be to duplicate STDOUT and STDERR when the Module::Build/Alien::Base object is initialized and use those new file handles for logging. |
That could work, have to make sure it is bullet proof on both windows and unix, but probably doable. |
IIRC, Test::More or Test::Builder do something similar. |
We'd need to replicate the logic in |
If the command fails, but there is some output we get a false positive. This problem was discovered when using perl Build.PL verbose=1 (and demonstrated with Alien::LibYAML). While this exact problem will hopefully be fixed with PR #147, there are other strangeisms that could infect the captured output with bogus data.
Confirmed that this causes breakage on |
The only other thing I can think of would be to default ABMB::Verbose to MB's verbose, though that's from a sanity argument; from looking at the proposed patch I don't suppose that that would fix the noted problems. I'm ok with the proposed fix: if MB is getting info that it shouldn't be, then don't hand it to it. Aye |
rebased and merged. |
If the command fails, but there is some output we get a false positive. This problem was discovered when using perl Build.PL verbose=1 (and demonstrated with Alien::LibYAML). While this exact problem will hopefully be fixed with PR #147, there are other strangeisms that could infect the captured output with bogus data.
Alien::Base::ModuleBuild::do_command captures the output from
Module::Build::Base::do_command, but the later may introduce debugging
output when Module::Build verbose mode is set.
This patch disables Mode::Build verbose mode before calling its
do_command method.