Hi,
Michael G Schwern wrote:
> 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.
[...]
> From 47a723a860cded6b16a716ea74c5bc029ee5b0ac Mon Sep 17 00:00:00 2001
> From: "Michael G. Schwern" <schwern@xxxxxxxxx>
> 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.
> 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.
> 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?
[...]
> --- 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html