Re: Find .pm files automatically (was Re: Fix git-svn tests for SVN 1.7.5.)

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 18, 2012, 00:01 UTC
Message-ID: <20120718000120.GG25325@burratino>
In-Reply-To: <5005EFC5.8060105@pobox.com>

Hi,

Michael G Schwern wrote:

Show 5 quoted lines

> Ok, here goes.
>
> First patch overhauls perl/Makefile.PL to make it easier to add .pm files,
> which I'm going to be doing a lot of.  Instead of having to manually add to
> the %pm hash, it scans for .pm files.

An excellent goal.

> It also moves Error.pm into a bundle directory.  This both makes it just
> another directory to scan (or not scan), but it also makes it possible to
> bundle additional modules in the future.  ExtUtils::MakeMaker uses this
> technique itself.

This is not so much "also" as "as an example to demonstrate the technique", no? I guess I'd prefer it to be in a separate patch, but this way's fine, too.

[...]

Show 11 quoted lines

> From 47a723a860cded6b16a716ea74c5bc029ee5b0ac Mon Sep 17 00:00:00 2001
> From: "Michael G. Schwern" <schwern@pobox.com>
> Date: Thu, 12 Jul 2012 00:05:38 -0700
> Subject: [PATCH 01/11] Make the process of adding a module less blecherous.
>
> * Scan for .pm files and build %pms rather than having to do it by hand.
> * Move the bundled Error into its own directory so we can bundle other modules.
>
> In addition...
> * Add all the .pm files to the all dependency in the alternative Makefile
> ---

You'll probably hate this. Because we have a bunch of patches to incorporate, I think it's worth spending the time to make that go as smoothly as possible for later patches.

 - the "From 47a723..." line is for your mailer.  Please do not
   include it in the body of your message.

 - likewise for the From: and Date: lines which are redundant next to
   the corresponding fields in the mail header

 - comments that are useful for posterity, like

	This patch overhauls perl/Makefile.PL to make it easier to add .pm files,
	which I'm going to be doing a lot of.  Instead of having to manually add to
	the %pm hash, it scans for .pm files.

   should go above the triple-dash marker, while comments that are
   less useful, like "Hi", go after the triple-dash.

 - using a different subject line for each patch makes the reader's
   life much easier, so please do use the subject lines from your
   commits in the mail header.

The git-format-patch(1) manpage has some instructions for using Thunderbird to send patches, which should take care of all this automatically.

If everything is right, then maintainers will love you because they can save a bunch of your patches that look ready into a single mbox and apply them all at once with "git am".

As Documentation/SubmittingPatches explains under "MUA specific hints", you can test that a patch is being sent correctly by emailing it to yourself, saving as an mbox, and trying to apply it with "git am <path to mbox file>". If the resulting commit is as expected, then you've succeeded.

Show 7 quoted lines

>  perl/Makefile                                     |  6 ++--
>  perl/Makefile.PL                                  | 42 +++++++++++++----------
>  perl/{private-Error.pm => bundles/Error/Error.pm} |  0
>  perl/bundles/README                               | 10 ++++++
>  4 files changed, 36 insertions(+), 22 deletions(-)
>  rename perl/{private-Error.pm => bundles/Error/Error.pm} (100%)
>  create mode 100644 perl/bundles/README

Ok, on to the patch proper.

Show 10 quoted lines

> diff --git a/perl/Makefile b/perl/Makefile
> index 6ca7d47..4f25930 100644
> --- a/perl/Makefile
> +++ b/perl/Makefile
> @@ -33,7 +33,7 @@ modules += Git/SVN/Prompt
>  modules += Git/SVN/Ra
> 
>  $(makfile): ../GIT-CFLAGS Makefile
> -	echo all: private-Error.pm Git.pm Git/I18N.pm > $@
> +	echo all: bundles/Error/Error.pm $(modules) > $@

The word "bundles/" left me a little nervous, because I (ignorantly) imagined that this might be some specialized facility like Python eggs or Ruby gems. Is the intent that this directory contains CPAN modules we want to be able to depend on? Is there really any intention of having more of them than Error.pm?

Before this patch, in the default case (with MakeMaker), "make install" wrote a manpage in <mandir>/man3/private-Error.3pm. Does it still do so after the patch? Will people who have installation scripts that expected that manpage have to change them, and if so, is the new behavior better to make up for that effort?

[...]

Show 10 quoted lines

> --- a/perl/Makefile.PL
> +++ b/perl/Makefile.PL
> @@ -2,11 +2,16 @@ use strict;
>  use warnings;
>  use ExtUtils::MakeMaker;
>  use Getopt::Long;
> +use File::Find;
> +
> +# Don't forget to update the perl/Makefile, too.
> +# Don't forget to test with NO_PERL_MAKEMAKER=YesPlease

Now the reader will have no reason to be looking at this file, so these comments are pretty much useless. In an ideal world, "make test" in the MakeMaker build would automatically "grep perl/Makefile" to catch modules that are not listed there, but that can wait, I imagine.

Alternatively, maybe there could be a perl/modules.list that both makefiles read? That way, if I drop in an unrelated .pm file for reference while coding the build system would not be confused by it, and since both build systems would use the same module list there would be no risk of it falling out of date.

Thanks for some food for thought, and hope that helps, Jonathan