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. > * 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. > @@ -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
