Thanks to *considerable* testing assistance from Jason Cole in several
rounds of off-list correspondence, I have been able to identify the
problem that Jason reported and to trace it to a subtle, long-standing
bug in config/auto/icu.pm.

To diagnose this problem I created a new 'reautoicu' branch in the
repository and added considerable verbose output.  Jason and I then
re-configured repeatedly with --verbose-step=auto::icu, as well as
running prove -v t/steps/auto_icu*.t.  The verbose output that jason was
getting on Win32 was self-contradictory:

Determining whether ICU is installed...Discovered icu-config --exists
returns 1
icu-config found... good!
Trying icu-config with '--ldflags'
icushared:  captured
For icushared, found  and 1
Trying icu-config with '--prefix'
icuheaders:  captured
For icuheaders, found \include and 1
..........................not found.

The line 'icu-config found... good!' suggested that Jason had ICU
installed.  But it was contradicted by the '1' values in three locations
-- system failure indicators rather than Perl truth -- and by the final
result:  'not found'.  If someone really doesn't have ICU installed,
then the step should have exited with a different message at a point
*earlier* than the point at which 'not found' is the result.

To somewhat shorten a long story, there was code deep inside
config/auto/icu.pm like this:

        ( $arg->{ret} == -1 )
            ||
        ( ( $arg->{ret} >> 8 ) != 0 )

... which seems to reflect what's stated here in the man page:

    perldoc -f system

                   The return value is the exit status of the program as
returned
               by the "wait" call.  To get the actual exit value, shift
right
               by eight ...

That sounded correct, until I asked the question:  How is the value of
$arg->{ret} determined?  Ultimately, it is set by an invocation of
Parrot::Configure::Utils::capture_output:

            my ( undef, undef, $ret ) =
                capture_output( $self->{icuconfig_default}, "--exists" );

But if we look at the source code for capture_output() (lines 238-260 of
lib/Parrot/Configure/Utils.pm), we see at line 246 that capture_output
*already applies the bitshifting* to the system command it calls.  That
means that this line in config/auto/icu.pm:

        ( ( $arg->{ret} >> 8 ) != 0 )

... is *one bit-shifting too many*!

I tore out the superfluous bit-shifting and both Jason and I were able
to get the expected verbose output during actual configuration and pass
all the tests.  As a side benefit, I was able to rip out some branches
and conditions for which I had been unable to write tests and which were
lowering the coverage results.

So I have merged the 'reautoicu' branch into trunk as of r29083.  Thanks
again to Coleoid for assistance with testing.

kid51

Reply via email to