Fix verbose output being captured as command output

[salva]

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.

[salva]

This bug appears when calling perl Build.PL verbose=1.

[plicease]

Is stuff breaking because there is extra diagnostics in the captured output?

[salva]

That extra output appears later on the value of version stored in PkgConfig.pm and it would probably also break the version check performed inside Alien::Base.

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.

[plicease]

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.

[salva]

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.

[mohawk2]

I'm going with Graham on this one: when he's happy, I'm happy, not before.

[plicease]

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.

• (aside) alien_check_installed_version should check the return status of the command that it executes. This probably doesn't solve the issue completely because the version stored on true positives will include the command executed which we don't want.

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.

[plicease]

To be sure I am not super crazy about either and am open to alternatives.

[salva]

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.

[plicease]

That could work, have to make sure it is bullet proof on both windows and unix, but probably doable.

[salva]

IIRC, Test::More or Test::Builder do something similar.

[plicease]

We'd need to replicate the logic in Module::Build::Base of log_* for either of these alternatives. I am beginning to think that the original PR may actually be the lesser of all evils.

[plicease]

Confirmed that this causes breakage on Alien::LibYAML when you set verbose=1. While I'd like a better solution, there seem to be only more complicated and at least as fragile solutions. Aye.

[jberger]

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

[plicease]

rebased and merged.