Re: [PATCH] Makefile: replace perl/Makefile.PL with simple make rules

From: Jeff King <peff@peff.net>
Date: Nov 30, 2017, 21:31 UTC
Message-ID: <20171130213105.GA8861@sigill.intra.peff.net>
In-Reply-To: <20171129195430.10069-1-avarab@gmail.com>

On Wed, Nov 29, 2017 at 07:54:30PM +0000, Ævar Arnfjörð Bjarmason wrote:

> Replace the perl/Makefile.PL and the fallback perl/Makefile used under
> NO_PERL_MAKEMAKER=NoThanks with a much simpler implementation heavily
> inspired by how the i18n infrastructure's build process works[1].

I'm very happy to see the recursive make invocation go away. The perl makefile generation was one of the few places where parallel make could racily get confused (though I haven't seen that for a while, so maybe it was fixed alongside some of the other .stamp work you did).

> The reason for having the Makefile.PL in the first place is that it
> was initially[2] building a perl C binding to interface with libgit,
> this functionality, that was removed[3] before Git.pm ever made it to
> the master branch.

Thanks for doing all this history digging. I agree that it doesn't seem like there's really any reason to carry the complexity. Of your functional changes, the only one that gives me pause is:

>  * This will not always install into perl's idea of its global
>    "installsitelib". This only potentially matters for packagers that
>    need to expose Git.pm for non-git use, and as explained in the
>    INSTALL file there's a trivial workaround.

This could be a minor hiccup for people using Git.pm from other scripts. But maybe only in funny setups? It seems like $prefix/share/perl5 would be in most people's @INC unless they are doing something exotic.

Show 5 quoted lines

>  * We don't build the Git(3) Git::I18N(3) etc. man pages from the
>    embedded perldoc. I suspect nobody really cares, these are mostly
>    internal APIs, and if someone's developing against them they likely
>    know enough to issue a "perldoc" against the installed file to get
>    the same result.

I don't have a real opinion on this, but it sounds from the rest of the thread like we should maybe build these to be on the safe side.

Show 5 quoted lines

> @@ -2291,6 +2273,17 @@ endif
>  po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
>  	$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
>  
> +PMFILES := $(wildcard perl/*.pm perl/*/*.pm perl/*/*/*.pm perl/*/*/*/*.pm)

Yuck. :) I don't think there's a better wildcard solution within make, though. And I'd rather see this than doing a $(shell) to "find" or similar.

The other option is to actually list the files, as we do for .o files. That's a minor pain to update, but it would allow things like differentiating which ones get their documentation built.

> +PMCFILES := $(patsubst perl/%.pm,perl/build/%.pmc,$(PMFILES))

TIL about pmc files. It sounds like they've had a storied history, but should be supported everywhere.

> [...]

The rest of the patch all looked good to me. Thanks for working on this.

-Peff