install_files.pl and install_dev_files.pl have a fair bit of duplicate code

[wayland]

Hopefully the attached patch will fix that.

[wayland]

Attached patch should hopefully fix the problem.

[jkeenan]

1. Since I've done a lot of work over the last 2-1/2 years refactoring code out of our .pl programs into .pm modules, I'm predisposed to like this patch. However, I'd also like to get some test files for the subroutines now extracted into Parrot::Install. Do you think you could work on that (or work on that with me)?

2. For consistency with other .pl files in the distro, I recommend:

use lib qw( lib );

... rather than

#! perl -Ilib

3. Could sub install_files() be refactored so that the list of files is passed by reference rather than by value? I.e.,

my($destdir, $dryrun, $filesref) = @_; 
    my($src, $dest, $mode);

    print("Installing ...\n");
    foreach ( @{ $filesref } ) {

... with appropriate changes in the two callers?

Thank you very much.
kid51

[wayland]

1. As regards testing, sure. Tell me where you want the test file, and what you want it called, and I should hopefully be able to handle it from there.

2. Ok, fixed in my local copy. I'll post a revised patch when I've done some more work. And good idea :)

3. Also fixed in my local copy. It makes the code uglier in the calls to install_files, IMHO, but it should work anyway.

[jkeenan]

Replying to wayland:

1. As regards testing, sure. Tell me where you want the test file, and what you want it called, and I should hopefully be able to handle it from there.

Excellent. I have created t/tools/install/ for this purpose. If you grep the tests in t/configure/ and t/steps/ for File::Temp and IO::CaptureOutput, you'll find plenty of examples of how we capture the print statements' output, how we set up temporary directories for safe testing, etc.

I'll write some documentation for Parrot::Install.

Because we're looking to release 1.0 next week, I recommend planning to commit this to trunk only after the release. But in the meantime I've created the install_tools branch to hold the code. You can submit patches to TT and I'll apply them to the branch as we go along.

Thank you very much.
kid51

[wayland]

Update based on the svn branch

[wayland]

While starting to write tests, I discovered the code had a fair bit of room for improvement. This patch: 1. Replaced our "create_directories" with some code that is just a call to mkpath (should this function also return 1, or is it find with the mkpath return value, which is what it's returning at the moment)? 2. Converted the files list to a has instead of an array, which gave more flexibility, and allowed the elimination of even more duplicate code. 3. Adds a test for create_directories. Unfortunately I didn't get on to the other tests, but I will do them.

I recommend that someone who actually understands the build system do a diff between install_files.pl and install_dev_files.pl because by doing so, I discovered a number of places where one file or the other needed fixing. I suspect that some of these still exist, and if they could be fixed, there would be potential for even more elimination of duplicate code.

[jkeenan]

Replying to wayland:

While starting to write tests, I discovered the code had a fair bit of room for improvement.

[snip] I discovered a number of places where one file or the other needed fixing. I suspect that some of these still exist, and if they could be fixed, there would be potential for even more elimination of duplicate code.

When you start to look closely at code for the purpose of refactoring it into modules, writing unit tests, etc., this is what usually happens. This is good progress. I hope to look at your second patch tonight.

[wayland]

Just tests this time

[wayland]

As requested, here's a less ambitious patch that just adds a few tests at the moment. Is that better?

[jkeenan]

Replying to wayland:

As requested, here's a less ambitious patch that just adds a few tests at the moment. Is that better?

Will review within next two days. Thanks for getting back on this.

[jkeenan]

Replying to jkeenan:

Replying to wayland:

As requested, here's a less ambitious patch that just adds a few tests at the moment. Is that better?

Will review within next two days. Thanks for getting back on this.

Have made some progress starting from your patch. See  coverage so far.

[wayland]

Another patch against current SVN for this file; includes additional tests

[wayland]

Ok, I've updated the big fix patch so that it now also has the tests working with it. Feel free to apply. However, I still haven't made the other changes (ie. open vs. ARGV, and making it into an object). I guess my expectation is that I'll make those changes after this is applied to SVN -- I'd rather not have to resolve more SVN conflicts.

HTH,

[jkeenan]

Replying to wayland:

Ok, I've updated the big fix patch so that it now also has the tests working with it. Feel free to apply. However, I still haven't made the other changes (ie. open vs. ARGV, and making it into an object). I guess my expectation is that I'll make those changes after this is applied to SVN -- I'd rather not have to resolve more SVN conflicts. HTH,

Applied to install_tools branch in r38062. Great progress on improving test coverage!

kid51

[wayland]

Patches documentation for Parrot::Manifest, etc

[wayland]

Thanks for the patch application. jkeenan suggested elsewhere that the next step would be to turn Parrot::Install into an object. I agree, but I've noticed the object Parrot::Manifest hanging around, and I think it should be merged into that.

As a first step towards that end, I've attached a patch that documents Parrot::Manifest. I've also moved the documentation for the MANIFEST format to lib/Parrot/Manifest.pm, so that diffs between install_files and install_dev_files are less confusing.

[jkeenan]

Here is a patch which represents the culmination of the work which wayland and I have been doing in the install_tools branch. It satisfies the original objective of this TT, namely, refactoring duplicated code out of two scripts. It also provides for tests for the subroutines thus refactored.

We would appreciate it if people could review this patch and give the new versions of install_files.pl and install_dev_files.pl some road testing. I would like to apply this patch to trunk this weekend.

Thank you very much.
kid51

[jkeenan]

Replying to jkeenan:

We would appreciate it if people could review this patch and give the new versions of install_files.pl and install_dev_files.pl some road testing.

I decided to take my own advice and try to use make install for the first time, albeit with --prefix set to a disposable directory. Results were not good. In both make install and make install-dev I got failures like this:

/usr/local/bin/perl tools/dev/install_files.pl \
    --buildprefix= \
    --prefix=/pseudoinstall \
    --exec-prefix=/pseudoinstall \
    --bindir=/pseudoinstall/bin \
    --libdir=/pseudoinstall/lib \
    --includedir=/pseudoinstall/include \
    --destdir= \
    --docdir=/pseudoinstall/share/doc \
    --versiondir=/parrot/1.1.0-devel \
    MANIFEST MANIFEST.generated
Unknown install location in MANIFEST for file 'LICENSE                                                     [main]doc'
make: *** [install] Error 9

LICENSE is the first file in MANIFEST that has a defined location: [main]doc. At first I wondered whether this might have been due to the fact that in config/init/install.pm, we had not yet changed doc_dir to docdir. I made that replacement globally, but I continued to get the same results.

I then ran install_files.pl through the Perl 5 debugger. I discovered that the stumbling block is in this loop inside Parrot::Install::lines_to_files():

            foreach my $tkey (@$transformorder) {
                $thash = $metatransforms->{$tkey};

### NEXT LINE IS PROBLEMATIC

                unless($thash->{ismeta} ? $metadata{$tkey} : $entry =~ /$tkey/) { next; }
                $filehash = &{ $thash->{transform} }($filehash);
                ref($filehash) eq 'HASH' or die "Error: transform didn't return a hash for key '$tkey'\n";
                $filehash->{Dest} = File::Spec->catdir(
                    $options_ref->{$thash->{optiondir} . 'dir'},
                    @{ $filehash->{DestDirs} },
                    $filehash->{Dest}
                );
                last FIXFILE;
            }
            die "Unknown install location in MANIFEST for file '$entry'\n";

I found that as we loop through the elements of @$transformorder, LICENSE never gets past the next in the line marked PROBLEMATIC. That suggests that $filehash -- and $filehash->{Dest} in particular -- is never well defined. Consequently, we fall through to the die beneath the loop.

I don't yet understand what's going on here, but I must say that the control flow of this line is really baffling:

   unless($thash->{ismeta} ? $metadata{$tkey} : $entry =~ /$tkey/) { next; }

An unless statement whose condition is itself a ternary!

So our last submitted patch is not yet ready for merging into trunk. Suggestions? Stay tuned!

Thank you very much.
kid51

[jkeenan]

Revised: svn diff --old= https://svn.parrot.org/parrot/trunk@37309 --new= https://svn.parrot.org/parrot/branches/install_tools@HEAD

[jkeenan]

I have uploaded a replacement for my last submitted patch. The bug -- which was of my own creation -- was actually located several lines above the foreach loop I discussed in my previous post.

I was able to test this safely by following this procedure:

1. Create a throwaway directory, say /home/user/pseudoinstall.

2. Do a fresh svn co of trunk, then apply diff.install_tools.patch.

3. Configure with a prefix: perl Configure.pl --prefix=/home/user/pseudoinstall. When configuration is done you can check Makefile and lib/Parrot/Config/Generated.pm to check where everything is going to be installed.

4. Call make and either make install or make install_dev. You should see everything you need being installed under /home/user/pseudoinstall. For example, you should be able to call: /home/user/pseudoinstall/bin/parrot --version.

5. You can then dispose of the pseudoinstall directory when you're through with roadtesting install_files.pl and install_dev_files.pl.

Please let us know what results you get or problems you encounter. Thank you very much.

kid51

[wayland]

I'm very relieved -- I thought I'd tested it in my own (admittedly RPM-building, so therefore unusual) environment.

[wayland]

Let me ask a question; could we commit Install.pm now (or after the release), even if we don't commit the changes to install_files.pl?

[jkeenan]

Replying to wayland:

Let me ask a question; could we commit Install.pm now (or after the release), even if we don't commit the changes to install_files.pl?

I think it would be preferable to commit all changed files at once. I concede that this has been pending for a long time. But further study led me to believe that we need some clarification of the rules by which we determine which files get installed and where. (See TT #677.)

Thank you very much.
kid51

[wayland]

Can I suggest (related to my recent comments on item 677) that this commit be applied before we get around to cleaning up everything? I mean, sure, move install_files.pl to the build directory, but I'd hope that we'd feel we can commit this before we work on the cleanup :).

[jkeenan]

Replying to jkeenan:

But further study led me to believe that we need some clarification of the rules by which we determine which files get installed and where. (See TT #677.)

I didn't get the clarifications I was hoping for, so I decided to proceed on a second-best-solution course. Over the past three days, I wrote and committed to trunk two programs which test install_files.pl and install_dev_files.pl by wrapping them in a system call and testing with simulated MANIFEST, MANIFEST.generated and source code files. In hindsight, this is what we should have done at the outset of this ticket, because otherwise it is difficult to ensure that we're not changing the overall functionality of these two programs while refactoring the hell oout of them. This exercise was also useful because it meant that I paid a lot more attention to how the two installer programs copied files into position.

Once those two test files were in trunk, I created a new branch called better_install_tools. I then carefully copied what wayland and I had done in the install_tools branch into that newer branch and spent a lot of time yesterday and earlier today in debugging. The overall tests did detect one significant error in lib/Parrot/Install.pm.

I then did the usual sort of coding standards corrections in the better_install_tools branch. Finally, I did --prefix installs of both make install and make install-dev in both trunk and branch and compared the lists of files installed. Once everything looked okay, I merged branch into trunk in r39165.

Thank you very much.
kid51

[jkeenan]

Please try out some installs (both --prefix and for-real) over the next few days. Then we'll close the ticket.

[jkeenan]

No major problems have been reported, though I believe wayland has more improvements to suggest. But we'll open new tickets for that. Closing this ticket.

Thank you very much.
kid51