Ticket #586 (closed bug: fixed)
tools/dev/install_files.pl: bug masked by missing file in MANIFEST.generated
| Reported by: | jkeenan | Owned by: | jkeenan |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | install | Version: | trunk |
| Severity: | medium | Keywords: | |
| Cc: | fperrad, allison, kjs | Language: | |
| Patch status: | new | Platform: |
Description
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.
kid51
Attachments
-
diff.MANIFEST.generated.re.pirc
(1.2 KB) - added by jkeenan
13 years ago.
- Only 'installable_' versions of executables s/b listed in MANIFEST.generated
