id	summary	reporter	owner	description	type	status	priority	milestone	component	version	severity	resolution	keywords	cc	lang	patch	platform
586	tools/dev/install_files.pl:  bug masked by missing file in MANIFEST.generated	jkeenan	jkeenan	"The '''short version''':  Should `installable_pirc.exe` have been included in ''MANIFEST.generated''?  If so, then a bug in ''tools/dev/install_files.pl'' has been masking that deficiency.

The '''long version''':  An entry for file ''pirc.exe'' was added to MANIFEST.generated in r37438.  Unlike other ''.exe'' files, no corresponding ''installable_pirc.exe'' file was added to MANIFEST.generated at that time.
{{{
$ grep -n 'installable_.*\.exe' MANIFEST MANIFEST.generated 
MANIFEST.generated:50:installable_parrot_config.exe                     [main]bin
MANIFEST.generated:52:installable_parrot_debugger.exe                   [main]bin
MANIFEST.generated:54:installable_parrot.exe                            [main]bin
MANIFEST.generated:56:installable_pbc_disassemble.exe                   [main]bin
MANIFEST.generated:58:installable_pbc_dump.exe                          [main]bin
MANIFEST.generated:60:installable_pbc_info.exe                          [main]bin
MANIFEST.generated:62:installable_pbc_merge.exe                         [main]bin
MANIFEST.generated:64:installable_pbc_to_exe.exe                        [main]bin
}}}
I think this may have been an oversight -- I don't know enough about pirc to be certain -- but we never noticed this oversight until now due to what I think is a bug in the trunk version of ''tools/dev/install_files.pl''.  Starting at line 258 of that file, we have two `foreach` loops:

In the first such loop, we loop over the list of files which have been classified as installables and mark those files to use the ''installable_'' version of those files during installation.
{{{
258 for (@installable_exe) {
259     my ( $i, $dest ) = @$_;
260     my ($file) = $i =~ /installable_(.+)$/;
261     next unless $file;
262     my @f = map { $_ ? $_->[0] : '' } @files;
263     if (grep(/^$file$/, @f)) {
264         if (-e $file) {
265             print ""skipping $file, using installable_$file instead\n"";
266             @files = map {$_ and $_->[0] !~ /^$file$/ ? $_ : undef} @files;
267         }
268     }
269 }
}}}
Prior to the second `foreach` loop, an inline comment tells us:
{{{
270 # 2. for every .exe check if there's an installable. Fail if not
}}}
Here's the second loop:
{{{
271 foreach my $f (@files ) {
272     next unless $_;
273     my ( $f, $dest ) = @$_;
274     my $i;
275     # This logic will fail on non-win32 if the generated files are really
276     # generated as with rt #40817. We don't have [main]bin here.
277     $i = ""installable_$f"" if $f =~ /\.exe$/;
278     next unless $i;
279     unless (map {$_->[0] =~ /^$i$/} @installable_exe) {
280         die ""$i is missing in MANIFEST or MANIFEST.generated\n"";
281     }
282 }
}}}
Now, when I see a loop start like this:
{{{
  foreach my $f (@files ) {
      next unless
}}}
... I expect the very next word of code to be:  `$f`, i.e.,
{{{
  foreach my $f (@files ) {
      next unless $f;
}}}
But here, in lines 272 and 273, we are looking at global `$_` which, AFAICT, is always a false value at this point (`undef` if my debugging is correct).  If `$_` is never true, then the second loop never gets past the `next` in line 272.  In turn, this means that we never have the chance to execute or not execute the `die` in line 280.  Lines 273 through 281 are unreachable.

In the course of the work wayland and I have been doing in TT #426, I had occasion to replace global `$_` with named lexical variables.  If I did the corresponding substitution in the trunk version of ''install_files.pl'', however, namely,
{{{
  foreach my $file (@files ) {
      next unless $file;
      my ( $f, $dest ) = @$file;
}}}
... I would then get past the `next` test in line 272.  If `$file->[0]` is `pirc.exe`, then `$i` is assigned `installable_pirc.exe` in line 277.  It's a true value, so it gets past the `next` in line 278.  However, since it's not found in ''MANIFEST.generated'', the `die` in line 280 is activated.  ''tools/dev/install_files.pl'' fails, as does `make install`, resulting in this `die` message:
{{{
/usr/bin/perl tools/dev/install_files.pl \
    --buildprefix= \
    --prefix=/topdir/pseudoinstall \
    --exec-prefix=/topdir/pseudoinstall \
    --bindir=/topdir/pseudoinstall/bin \
    --libdir=/topdir/pseudoinstall/lib \
    --includedir=/topdir/pseudoinstall/include \
    --destdir= \
    --docdir=/topdir/pseudoinstall/share/doc \
    --versiondir=/parrot/1.1.0-devel \
    MANIFEST MANIFEST.generated
skipping parrot_debugger, using installable_parrot_debugger instead
skipping pbc_disassemble, using installable_pbc_disassemble instead
skipping pbc_dump, using installable_pbc_dump instead
skipping pbc_info, using installable_pbc_info instead
skipping pbc_merge, using installable_pbc_merge instead
skipping pbc_to_exe, using installable_pbc_to_exe instead
installable_pirc.exe is missing in MANIFEST or MANIFEST.generated
make: *** [install] Error 2
}}}
So, should `installable_pirc.exe` have been included in ''MANIFEST.generated''?  Since this is a Win32 file and I don't have access to that OS, it would be good if someone can see whether `installable_pirc.exe` is, in fact, generated during the Parrot build process.  If so, then we can safely add it to ''MANIFEST.generated''.  I will then fix the code in ''install_files.pl''.

Thank you very much.[[BR]]
kid51"	bug	closed	normal		install	trunk	medium	fixed		fperrad allison kjs		new