Ticket #586 (closed bug: fixed)

Opened 6 years ago

Last modified 6 years ago

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 Download (1.2 KB) - added by jkeenan 6 years ago.
Only 'installable_' versions of executables s/b listed in MANIFEST.generated

Change History

  Changed 6 years ago by jkeenan

  • platform changed from all to win32

in reply to: ↑ description   Changed 6 years ago by jkeenan

  • owner set to jkeenan
  • platform win32 deleted
  • status changed from new to assigned

Here's what I've been able to learn so far. Testing below done primarily on Linux, but confirmed on Darwin.

With MANIFEST.generated edited to include installable_pirc and installable_pirc.exe, but without correcting tools/dev/install_files.pl:

perl Configure.pl --prefix=/topdir/pseudoinstall

Install was successful.

[li11-226:parrot] 517 $ pwd
/topdir/parrot
[li11-226:parrot] 515 $ ll install*
-rw-r--r-- 1 jimk jimk 22004 Apr 28 06:53 install_config.fpmc
-rwxr-xr-x 1 jimk jimk 68578 Apr 28 06:53 installable_parrot
-rwxr-xr-x 1 jimk jimk 34493 Apr 28 06:53 installable_parrot_config
-rwxr-xr-x 1 jimk jimk 78139 Apr 28 06:53 installable_parrot_debugger
-rwxr-xr-x 1 jimk jimk 47258 Apr 28 06:53 installable_pbc_disassemble
-rwxr-xr-x 1 jimk jimk 97435 Apr 28 06:53 installable_pbc_dump
-rwxr-xr-x 1 jimk jimk 45653 Apr 28 06:53 installable_pbc_info
-rwxr-xr-x 1 jimk jimk 83941 Apr 28 06:53 installable_pbc_merge
-rwxr-xr-x 1 jimk jimk 44386 Apr 28 06:53 installable_pbc_to_exe

Note absence of installable_pirc.

With MANIFEST.generated edited to include installable_pirc and installable_pirc.exe, and with correcting tools/dev/install_files.pl:

perl Configure.pl --prefix=/topdir/pseudoinstall

Install was successful.

[li11-226:parrot] 528 $ ll install*
-rw-r--r-- 1 jimk jimk 22004 Apr 28 07:00 install_config.fpmc
-rwxr-xr-x 1 jimk jimk 68578 Apr 28 07:00 installable_parrot
-rwxr-xr-x 1 jimk jimk 34493 Apr 28 07:00 installable_parrot_config
-rwxr-xr-x 1 jimk jimk 78139 Apr 28 07:00 installable_parrot_debugger
-rwxr-xr-x 1 jimk jimk 47258 Apr 28 07:00 installable_pbc_disassemble
-rwxr-xr-x 1 jimk jimk 97435 Apr 28 07:00 installable_pbc_dump
-rwxr-xr-x 1 jimk jimk 45653 Apr 28 07:00 installable_pbc_info
-rwxr-xr-x 1 jimk jimk 83941 Apr 28 07:00 installable_pbc_merge
-rwxr-xr-x 1 jimk jimk 44386 Apr 28 07:00 installable_pbc_to_exe

Inferences: Correcting install_files.pl, at the very least, does no harm. Moreover, when MANIFEST.generated is corrected to include two more files, everything DTRT. However, no pirc executable gets built in either case. It's not clear under what circumstances it does get built.

So I'm inclined to add the two files to MANIFEST.generated and to correct tools/dev/install_files.pl. Any comment?

Thank you very much.

kid51

in reply to: ↑ description   Changed 6 years ago by jkeenan

  • cc fperrad, allison added
  • patch set to new

Replying to 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.

...

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.

Follow-up, hoping for feedback from fperrad and allison:

In responding to TT #347 with r38393, Allison removed from MANIFEST.generated those executable files whose names did not begin with installable_. That may enable further simplification of tools/dev/install_files.pl. But, more immediately, that leads me to believe that pirc and pirc.exe should not be in MANIFEST.generated but that installable_pirc and installable_pirc.exe should be.

Do you agree? If so, we can apply the patch I'm about to attach when we correct tools/dev/install_files.pl as well.

Thank you very much.
kid51

Changed 6 years ago by jkeenan

Only 'installable_' versions of executables s/b listed in MANIFEST.generated

follow-up: ↓ 5   Changed 6 years ago by fperrad

  • cc kjs added

pirc is still a work in progress (see kjs). So currently, no target installable_pirc exists.

A executable has two flavors installable & not, when it needs to be linked with config which exists in different versions :

  • parrot_config
  • install_config
  • null_config

As I see, pirc don't need a config.

in reply to: ↑ 4 ; follow-up: ↓ 6   Changed 6 years ago by jkeenan

Replying to fperrad:

pirc is still a work in progress (see kjs). So currently, no target installable_pirc exists. A executable has two flavors installable & not, when it needs to be linked with config which exists in different versions : - parrot_config - install_config - null_config As I see, pirc don't need a config.

Follow-up question: How is 'pirc' currently built?

It doesn't appear for me with an ordinary configuration on either Linux or Darwin. So I'm wondering how make generates it, thereby warranting its inclusion in MANIFEST.generated.

in reply to: ↑ 5   Changed 6 years ago by jkeenan

Replying to jkeenan:

Replying to fperrad:

As I see, pirc don't need a config.

Follow-up question: How is 'pirc' currently built?

I now see that it is built via 'make pirc'. Since it is not (currently) built by 'make', I don't believe it should be in 'MANIFEST.generated'.

In r38446, I:

-- corrected the faulty logic described in my OP to this TT;
-- replaced $_ with named lexicals in many locations;
-- removed pirc and pirc.exe from MANIFEST.generated;
-- removed one comment which referred to a ticket which was earlier rejected;
-- moved the POD to the end of the file so that you don't have to scroll down 5 screens to get to the code.

tools/dev/install_files.pl now has a few of its problems corrected and conducts a successful installation.

Thank you very much.
kid51

  Changed 6 years ago by jkeenan

  • component changed from none to install

  Changed 6 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

There have been no complaints in this ticket in three weeks. And since in r39165 we just committed a major refactoring of the installer programs, I think it's best we resolve this ticket now so that any further issues are discussed with reference to the code now in trunk.

Thank you very much.
kid51

Note: See TracTickets for help on using tickets.