RE: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Randall S. Becker]

On February 25, 2018 1:57 PM, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Feb 14 2018, Jonathan Nieder jotted:
> 
> > Ævar Arnfjörð Bjarmason wrote:
> >
> >> Change the two wrappers to load from CPAN (local OS) or our own copy
> >> to do so via the same codepath.
> >
> > nit: I think with s/to load/that load/ this will be easier to read.
> >
> >> I added the Error.pm wrapper in 20d2a30f8f ("Makefile: replace
> >> perl/Makefile.PL with simple make rules", 2017-12-10), and shortly
> >> afterwards Matthieu Moy added a wrapper for Mail::Address in
> >> bd869f67b9 ("send-email: add and use a local copy of Mail::Address",
> >> 2018-01-05).
> >>
> >> His was simpler since Mail::Address doesn't have an "import" method,
> >> but didn't do the same sanity checking, e.g. a missing FromCPAN
> >> directory (which OS packages are likely not to have) wouldn't be
> >> explicitly warned about.
> >
> > I'm having trouble parsing this.  Mail::Address didn't do the same
> > sanity checking or his didn't?
> >
> > The comma before e.g. should be a period or semicolon, since it's
> > starting a new independent clause.
> >
> >> Now both use a modification of the previously Error.pm-specific
> >> codepath, which has been amended to take the module to load as
> >> parameter, as well as whether or not that module has an import method.
> >
> > Does "now" mean before this patch or after this patch?  Usually commit
> > messages describe the status quo without the patch in the present
> > tense and the change the patch will make in the imperative.
> > So this could say:
> >
> > Update both to use a common implementation based on the
> previous
> > Error.pm loader.
> 
> All good feeedback, thanks. Incorporated into v2 which I'm about to submit.
> 
> > [...]
> >> +++ b/perl/Git/LoadCPAN.pm
> >> @@ -0,0 +1,74 @@
> > [...]
> >> +The Perl code in Git depends on some modules from the CPAN, but we
> >> +don't want to make those a hard requirement for anyone building from
> >> +source.
> >
> > not about this patch: have we considered making it a hard requirement?
> > Both Mail::Address and Error.pm are pretty widely available, and I
> > wonder if we could make the instructions in the INSTALL file say that
> > they are required dependencies to simplify things.
> 
> I can't remember when, but at some point this was discussed on list, and the
> consensus was that us using perl should be kept as a non-invasive
> implementation detail that would be as small of a pain as possible for users.

That would include the platform I'm maintaining, where perl is currently pretty 
handcuffed and blindfolded (including completion code misinterprets). CPAN 
isn't currently an option, but might be soon.

> It's easy for distros to package these modules, but for users building from
> source who know nothing about perl it can be a pain.

Know perl I do. Use it not, can I. ;-)

> I also think it's very useful to avoid the side-discussion about not using 
> some
> useful CPAN module in the future just because it's not widely used, but
> would be perfect for some use-case of ours.
> 
> > I admit part of my bias here is coming from the distro world, where we
> > have to do extra work to get rid of the FromCPAN embedded copies and
> > would be happier not to have to.
> 
> I think there's a very good argument to be made for inverting the
> NO_PERL_CPAN_FALLBACKS logic, but my soon to be submitted v2 keeps it
> off by default.

Cool, thanks.

Cheers,
Randall

-- Brief whoami:
  NonStop developer since approximately NonStop(2112884442)
  UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Ævar Arnfjörð Bjarmason]

On Wed, Feb 14 2018, Jonathan Nieder jotted:

> Ævar Arnfjörð Bjarmason wrote:
>
>> Change the two wrappers to load from CPAN (local OS) or our own copy
>> to do so via the same codepath.
>
> nit: I think with s/to load/that load/ this will be easier to read.
>
>> I added the Error.pm wrapper in 20d2a30f8f ("Makefile: replace
>> perl/Makefile.PL with simple make rules", 2017-12-10), and shortly
>> afterwards Matthieu Moy added a wrapper for Mail::Address in
>> bd869f67b9 ("send-email: add and use a local copy of Mail::Address",
>> 2018-01-05).
>>
>> His was simpler since Mail::Address doesn't have an "import" method,
>> but didn't do the same sanity checking, e.g. a missing FromCPAN
>> directory (which OS packages are likely not to have) wouldn't be
>> explicitly warned about.
>
> I'm having trouble parsing this.  Mail::Address didn't do the same
> sanity checking or his didn't?
>
> The comma before e.g. should be a period or semicolon, since it's
> starting a new independent clause.
>
>> Now both use a modification of the previously Error.pm-specific
>> codepath, which has been amended to take the module to load as
>> parameter, as well as whether or not that module has an import method.
>
> Does "now" mean before this patch or after this patch?  Usually
> commit messages describe the status quo without the patch in the
> present tense and the change the patch will make in the imperative.
> So this could say:
>
>   Update both to use a common implementation based on the previous
>   Error.pm loader.

All good feeedback, thanks. Incorporated into v2 which I'm about to
submit.

> [...]
>> +++ b/perl/Git/LoadCPAN.pm
>> @@ -0,0 +1,74 @@
> [...]
>> +The Perl code in Git depends on some modules from the CPAN, but we
>> +don't want to make those a hard requirement for anyone building from
>> +source.
>
> not about this patch: have we considered making it a hard requirement?
> Both Mail::Address and Error.pm are pretty widely available, and I
> wonder if we could make the instructions in the INSTALL file say that
> they are required dependencies to simplify things.

I can't remember when, but at some point this was discussed on list, and
the consensus was that us using perl should be kept as a non-invasive
implementation detail that would be as small of a pain as possible for
users.

It's easy for distros to package these modules, but for users building
from source who know nothing about perl it can be a pain.

I also think it's very useful to avoid the side-discussion about not
using some useful CPAN module in the future just because it's not widely
used, but would be perfect for some use-case of ours.

> I admit part of my bias here is coming from the distro world, where we
> have to do extra work to get rid of the FromCPAN embedded copies and
> would be happier not to have to.

I think there's a very good argument to be made for inverting the
NO_PERL_CPAN_FALLBACKS logic, but my soon to be submitted v2 keeps it
off by default.

> [...]
>> +Usually OS's will not ship with Git's Git::FromCPAN tree at all,
>> +preferring to use their own packaging of CPAN modules instead.
>
> nit: I think the plural of OS is OSes, or something like
> "distributors" or "operating systems".

Thanks.

> [...]
>> +eval {
>> +require $package_pm;
>> +1;
>> +} or do {
>
> also not about this patch: this mixed tabs/spacing formatting feels a
> bit unusual.  I don't know if it's idiomatic for perl, and if it is
> then no complaints; it just stood out a little.  Can
> Documentation/CodingGuidelines say something about the preferred
> indentation in Perl to avoid having to think about such questions?

Thanks, that sucked. Changed to \t. I still haven't gotten around to
hacking my $EDITOR settings for git.git (like for C & SH).

>> --- a/perl/Git/LoadCPAN/Error.pm
>> +++ b/perl/Git/LoadCPAN/Error.pm
>> @@ -2,45 +2,9 @@ package Git::LoadCPAN::Error;
>>  use 5.008;
>>  use strict;
>>  use warnings;
>> +use Git::LoadCPAN (
>> +module => 'Error',
>> +import => 1,
>> +);
>
> Nice!
>
> Thanks and hope that helps,
> Jonathan

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

I wrote:
> Hi Jonathan,
> 
> Jonathan Nieder wrote:
>> Todd Zullinger wrote:
> [...]
>>> +++ b/Makefile
>>> @@ -296,6 +296,9 @@ all::
>>>  #
>>>  # Define NO_PERL if you do not want Perl scripts or libraries at all.
>>>  #
>>> +# Define NO_PERL_CPAN_FALLBACKS if you do not want to install fallbacks for
>>> +# perl CPAN modules.
>> 
>> nit: Looking at this as a naive user, it's not obvious what kind of
>> fallbacks are meant. How about:
>> 
>>  Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
>>  copies of CPAN modules that serve as a fallback in case the modules are
>>  not available on the system.
>> 
>> Or perhaps:
>> 
>>  Define HAVE_CPAN_MODULES if you have Error.pm and Mail::Address 
>> installed
>>  and do not want to install the fallback copies from perl/FromCPAN.
> 
> Hmm, a positive variable like HAVE_CPAN_MODULES is
> appealing.
> 
> I don't know about listing the modules, as those seem likely
> to change and then the comment becomes stale.  It's nice to
> have a shorter name.  I could easily go back and forth.
> Hopefully some other folks will chime in with preferences.
> 
>> Would this patch need to update the loader to expect the modules in
>> the new location?
> 
> That's a good catch.  In checking how this ends up when not
> setting NO_PERL_CPAN_FALLBACKS, we end up installing
> FromCPAN at the root of $perllibdir rather than under the
> Git dir.
> 
> While we could probably fix Git::LoadCPAN, I doubt we want
> to pollute the namespace. ;) So we'll want to ensure the
> files get put in the right place from the start.  I'll try
> to fix that up.

I ended up adding a second variable to hold the FromCPAN
wildcard match, as I couldn't come up with any clean way to
do it in the same LIB_PERL{,_GEN} vars.

I tested it with and without NO_PERL_CPAN_FALLBACKS set and
with and without the system Error and Mail::Address modules
installed.  There's nothing which automatically removes the
perl/build/lib/Git/FromCPAN tree if make was run without
NO_PERL_CPAN_FALLBACKS set and then re-run with it.  I don't
know if that's something we'd care to support or not.  I
suspect it's simpler to require 'make clean' between such
runs.

(I had to restore the FromCPAN copy of Address.pm, as noted
in <87tvuif10e@evledraar.gmail.com>, of course.)

Here's what it looks like now (still subject to changing the
NO_PERL_CPAN_FALLBACKS variable).

 8> 
From: Todd Zullinger 
Subject: [PATCH] Makefile: add NO_PERL_CPAN_FALLBACKS knob

We include some perl modules which are not part of the core perl
install, as a convenience.  This allows us to rely on those modules in
our perl-based tools and scripts without requiring users to install the
modules from CPAN or their operating system packages.

Users whose operating system provides these modules and packagers of Git
often don't want to ship or use these bundled modules.  Allow these
users to set NO_PERL_CPAN_FALLBACKS to avoid installing the bundled
modules.

Signed-off-by: Todd Zullinger 
---
 Makefile| 14 ++
 perl/{Git => }/FromCPAN/.gitattributes  |  0
 perl/{Git => }/FromCPAN/Error.pm|  0
 perl/{Git => }/FromCPAN/Mail/.gitattributes |  0
 perl/{Git => }/FromCPAN/Mail/Address.pm |  0
 5 files changed, 14 insertions(+)
 rename perl/{Git => }/FromCPAN/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Error.pm (100%)
 rename perl/{Git => }/FromCPAN/Mail/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Mail/Address.pm (100%)

diff --git a/Makefile b/Makefile
index bdd50069af..49244fb116 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,10 @@ all::
 #
 # Define NO_PERL if you do not want Perl scripts or libraries at all.
 #
+# Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
+# copies of CPAN modules that serve as a fallback in case the modules are
+# not available on the system.
+#
 # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
 # but /usr/bin/python2.7 on some platforms).
 #
@@ -2300,15 +2304,25 @@ po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
 
 LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
perl/Git/*/*/*.pm)
 LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
+ifndef NO_PERL_CPAN_FALLBACKS
+LIB_CPAN := $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
+LIB_CPAN_GEN := $(patsubst perl/%.pm,perl/build/lib/Git/%.pm,$(LIB_CPAN))
+endif
 
 ifndef NO_PERL
 all:: $(LIB_PERL_GEN)
+ifndef NO_PERL_CPAN_FALLBACKS
+all:: $(LIB_CPAN_GEN)
+endif
 endif
 
 perl/build/lib/%.pm: perl/%.pm
$(QUIET_GEN)mkdir -p $(dir $@) && \
sed -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' < $< > $@
 
+perl/build/lib/Git/FromCPAN/%.pm: perl/FromCPAN/%.pm
+   $(QUIET_GEN)mkdir -p $(dir $@) && cp $< $@
+
 perl/build/man/man3/Git.3pm: perl/Git.pm
$(QUIET_GEN)mkdir -p $(dir $@) && \
pod2man $< $@
diff --git 

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Hi Jonathan,

Jonathan Nieder wrote:
> Todd Zullinger wrote:
[...]
>> +++ b/Makefile
>> @@ -296,6 +296,9 @@ all::
>>  #
>>  # Define NO_PERL if you do not want Perl scripts or libraries at all.
>>  #
>> +# Define NO_PERL_CPAN_FALLBACKS if you do not want to install fallbacks for
>> +# perl CPAN modules.
> 
> nit: Looking at this as a naive user, it's not obvious what kind of
> fallbacks are meant. How about:
> 
>   Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
>   copies of CPAN modules that serve as a fallback in case the modules are
>   not available on the system.
> 
> Or perhaps:
> 
>   Define HAVE_CPAN_MODULES if you have Error.pm and Mail::Address 
> installed
>   and do not want to install the fallback copies from perl/FromCPAN.

Hmm, a positive variable like HAVE_CPAN_MODULES is
appealing.

I don't know about listing the modules, as those seem likely
to change and then the comment becomes stale.  It's nice to
have a shorter name.  I could easily go back and forth.
Hopefully some other folks will chime in with preferences.

> Would this patch need to update the loader to expect the modules in
> the new location?

That's a good catch.  In checking how this ends up when not
setting NO_PERL_CPAN_FALLBACKS, we end up installing
FromCPAN at the root of $perllibdir rather than under the
Git dir.

While we could probably fix Git::LoadCPAN, I doubt we want
to pollute the namespace. ;) So we'll want to ensure the
files get put in the right place from the start.  I'll try
to fix that up.

Thanks for the careful eyes, as usual.

-- 
Todd
~~
Happiness is like peeing on yourself. Everyone can see it, but only
you can feel its warmth

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Jonathan Nieder]

Hi,

Todd Zullinger wrote:

> Subject: [PATCH] Makefile: add NO_PERL_CPAN_FALLBACKS to disable fallback 
> module install
>
> As noted in perl/Git/LoadCPAN.pm, operating system packages often don't
> want to ship Git::FromCPAN tree at all, preferring to use their own
> packaging of CPAN modules instead.  Allow such packagers to set
> NO_PERL_CPAN_FALLBACKS to easily avoid installing these fallback perl
> CPAN modules.
>
> Signed-off-by: Todd Zullinger 
> ---
>  Makefile| 6 ++
>  perl/{Git => }/FromCPAN/.gitattributes  | 0
>  perl/{Git => }/FromCPAN/Error.pm| 0
>  perl/{Git => }/FromCPAN/Mail/.gitattributes | 0
>  perl/{Git => }/FromCPAN/Mail/Address.pm | 0
>  5 files changed, 6 insertions(+)
>  rename perl/{Git => }/FromCPAN/.gitattributes (100%)
>  rename perl/{Git => }/FromCPAN/Error.pm (100%)
>  rename perl/{Git => }/FromCPAN/Mail/.gitattributes (100%)
>  rename perl/{Git => }/FromCPAN/Mail/Address.pm (100%)

Nice!  I like it.

[...]
> +++ b/Makefile
> @@ -296,6 +296,9 @@ all::
>  #
>  # Define NO_PERL if you do not want Perl scripts or libraries at all.
>  #
> +# Define NO_PERL_CPAN_FALLBACKS if you do not want to install fallbacks for
> +# perl CPAN modules.

nit: Looking at this as a naive user, it's not obvious what kind of
fallbacks are meant. How about:

Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
copies of CPAN modules that serve as a fallback in case the modules are
not available on the system.

Or perhaps:

Define HAVE_CPAN_MODULES if you have Error.pm and Mail::Address 
installed
and do not want to install the fallback copies from perl/FromCPAN.

Would this patch need to update the loader to expect the modules in
the new location?

Thanks,
Jonathan

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Ævar Arnfjörð Bjarmason wrote:
> On Thu, Feb 15 2018, Todd Zullinger jotted:
>> What about moving perl/Git/FromCPAN to perl/FromCPAN and
>> then including perl/FromCPAN in LIB_PERL{,_GEN} only if
>> NO_PERL_CPAN_FALLBACKS is unset?
>>
>>  LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
>> perl/Git/*/*/*.pm)
>> +ifndef NO_PERL_CPAN_FALLBACKS
>> +LIB_PERL += $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
>> +endif
>>  LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
>>
>> I haven't tested that at all, so it could be broken in many
>> ways.
> 
> Yes that's a much better idea, it evades the whole problem of conflating
> the perl/Git* glob.

I did test this yesterday and it seems to work well.  Here
it is in patch form, in case it's helpful to you for the
next version.  It might well be better as part of a commit
teaching Git::LoadCPAN to respect NO_PERL_CPAN_FALLBACKS.

 8< 
From: Todd Zullinger 
Subject: [PATCH] Makefile: add NO_PERL_CPAN_FALLBACKS to disable fallback
 module install

As noted in perl/Git/LoadCPAN.pm, operating system packages often don't
want to ship Git::FromCPAN tree at all, preferring to use their own
packaging of CPAN modules instead.  Allow such packagers to set
NO_PERL_CPAN_FALLBACKS to easily avoid installing these fallback perl
CPAN modules.

Signed-off-by: Todd Zullinger 
---
 Makefile| 6 ++
 perl/{Git => }/FromCPAN/.gitattributes  | 0
 perl/{Git => }/FromCPAN/Error.pm| 0
 perl/{Git => }/FromCPAN/Mail/.gitattributes | 0
 perl/{Git => }/FromCPAN/Mail/Address.pm | 0
 5 files changed, 6 insertions(+)
 rename perl/{Git => }/FromCPAN/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Error.pm (100%)
 rename perl/{Git => }/FromCPAN/Mail/.gitattributes (100%)
 rename perl/{Git => }/FromCPAN/Mail/Address.pm (100%)

diff --git a/Makefile b/Makefile
index 5bcd83ddf3..838b3c6393 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,9 @@ all::
 #
 # Define NO_PERL if you do not want Perl scripts or libraries at all.
 #
+# Define NO_PERL_CPAN_FALLBACKS if you do not want to install fallbacks for
+# perl CPAN modules.
+#
 # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
 # but /usr/bin/python2.7 on some platforms).
 #
@@ -2297,6 +2300,9 @@ po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
 
 LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
perl/Git/*/*/*.pm)
+ifndef NO_PERL_CPAN_FALLBACKS
+LIB_PERL += $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
+endif
 LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
 
 ifndef NO_PERL
diff --git a/perl/Git/FromCPAN/.gitattributes b/perl/FromCPAN/.gitattributes
similarity index 100%
rename from perl/Git/FromCPAN/.gitattributes
rename to perl/FromCPAN/.gitattributes
diff --git a/perl/Git/FromCPAN/Error.pm b/perl/FromCPAN/Error.pm
similarity index 100%
rename from perl/Git/FromCPAN/Error.pm
rename to perl/FromCPAN/Error.pm
diff --git a/perl/Git/FromCPAN/Mail/.gitattributes 
b/perl/FromCPAN/Mail/.gitattributes
similarity index 100%
rename from perl/Git/FromCPAN/Mail/.gitattributes
rename to perl/FromCPAN/Mail/.gitattributes
diff --git a/perl/Git/FromCPAN/Mail/Address.pm b/perl/FromCPAN/Mail/Address.pm
similarity index 100%
rename from perl/Git/FromCPAN/Mail/Address.pm
rename to perl/FromCPAN/Mail/Address.pm
-- 
2.16.1

-- 
Todd
~~
Damn you and your estrogenical treachery!
-- Stewie Griffin

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 15 2018, Todd Zullinger jotted:

> [I dropped bbour...@slb.com from the Cc: list, as it bounced
> on my previous reply.]
>
> Ævar Arnfjörð Bjarmason wrote:
>> That makes sense, I'll incorporate that in a re-roll. I like
>> NO_PERL_CPAN_FALLBACKS or just NO_CPAN_FALLBACKS better.
>
> Either is an improvement.  Starting with NO_PERL_ seems
> like a slightly better bikeshed color. :)
>
>> I'd really like to find some solution that works differently though,
>> because with this approach we'll run the full test suite against a
>> system where our fallbacks will be in place (although if the OS
>> distributor has done as promised we won't use them), and then just
>> remove this at 'make install' time, also meaning we'll re-gen it before
>> running 'make install' again, only to rm it again.
>>
>> The former issue we could deal with by munging the Git::LoadCPAN file so
>> it knows about NO_PERL_CPAN_FALLBACKS, and will always refuse to use the
>> fallbacks if that's set. That's a good idea anyway, because right now if
>> you e.g. uninstall Error.pm on Debian (which strips the CPAN fallbacks)
>> you get a cryptic "BUG: ..." message, it should instead say "we couldn't
>> get this module the OS promised we'd have" or something to that effect.
>
> Teaching Git::LoadCPAN to never fallback sounds like a good
> idea.  At least then if the packager intended to avoid the
> fallbacks and didn't get it right the error message could be
> more useful.
>
> Hopefully that's not a common problem for packagers though.
> (And adding the Makefile knob was intended to help make it
> easier for packagers to achieve this common goal.)
>
>> The latter is trickier, I don't see an easy way to coerce the Makefile
>> into not copying the FromCPAN directory without going back to a
>> hardcoded list again, the easiest thing is probably to turn that:
>>
>> $(TAR) cf - .)
>>
>> Into:
>>
>> $(TAR) cf - $(find ... -not )
>>
>> Or something like that to get all the stuff that isn't the Git/FromCPAN
>> directory.
>>
>> Other suggestions most welcome.
>
> What about moving perl/Git/FromCPAN to perl/FromCPAN and
> then including perl/FromCPAN in LIB_PERL{,_GEN} only if
> NO_PERL_CPAN_FALLBACKS is unset?
>
>  LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
> perl/Git/*/*/*.pm)
> +ifndef NO_PERL_CPAN_FALLBACKS
> +LIB_PERL += $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
> +endif
>  LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
>
> I haven't tested that at all, so it could be broken in many
> ways.

Yes that's a much better idea, it evades the whole problem of conflating
the perl/Git* glob.

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

[I dropped bbour...@slb.com from the Cc: list, as it bounced
on my previous reply.]

Ævar Arnfjörð Bjarmason wrote:
> That makes sense, I'll incorporate that in a re-roll. I like
> NO_PERL_CPAN_FALLBACKS or just NO_CPAN_FALLBACKS better.

Either is an improvement.  Starting with NO_PERL_ seems
like a slightly better bikeshed color. :)

> I'd really like to find some solution that works differently though,
> because with this approach we'll run the full test suite against a
> system where our fallbacks will be in place (although if the OS
> distributor has done as promised we won't use them), and then just
> remove this at 'make install' time, also meaning we'll re-gen it before
> running 'make install' again, only to rm it again.
> 
> The former issue we could deal with by munging the Git::LoadCPAN file so
> it knows about NO_PERL_CPAN_FALLBACKS, and will always refuse to use the
> fallbacks if that's set. That's a good idea anyway, because right now if
> you e.g. uninstall Error.pm on Debian (which strips the CPAN fallbacks)
> you get a cryptic "BUG: ..." message, it should instead say "we couldn't
> get this module the OS promised we'd have" or something to that effect.

Teaching Git::LoadCPAN to never fallback sounds like a good
idea.  At least then if the packager intended to avoid the
fallbacks and didn't get it right the error message could be
more useful.

Hopefully that's not a common problem for packagers though.
(And adding the Makefile knob was intended to help make it
easier for packagers to achieve this common goal.)

> The latter is trickier, I don't see an easy way to coerce the Makefile
> into not copying the FromCPAN directory without going back to a
> hardcoded list again, the easiest thing is probably to turn that:
> 
> $(TAR) cf - .)
> 
> Into:
> 
> $(TAR) cf - $(find ... -not )
> 
> Or something like that to get all the stuff that isn't the Git/FromCPAN
> directory.
> 
> Other suggestions most welcome.

What about moving perl/Git/FromCPAN to perl/FromCPAN and
then including perl/FromCPAN in LIB_PERL{,_GEN} only if
NO_PERL_CPAN_FALLBACKS is unset?

 LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
perl/Git/*/*/*.pm)
+ifndef NO_PERL_CPAN_FALLBACKS
+LIB_PERL += $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
+endif
 LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))

I haven't tested that at all, so it could be broken in many
ways.

Thanks,

-- 
Todd
~~
The nice thing about egotists is that they don't talk about other
people.
-- Lucille S. Harper

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 15 2018, Todd Zullinger jotted:

> Hi Ævar,
>
> Ævar Arnfjörð Bjarmason wrote:
>> +Git::LoadCPAN - Wrapper for loading modules from the CPAN (OS) or Git's own 
>> copy
>> +
>> +=head1 DESCRIPTION
>> +
>> +The Perl code in Git depends on some modules from the CPAN, but we
>> +don't want to make those a hard requirement for anyone building from
>> +source.
>> +
>> +Therefore the L namespace shipped with Git contains
>> +wrapper modules like C that will first
>> +attempt to load C from the OS, and if that doesn't work
>> +will fall back on C shipped with Git
>> +itself.
>> +
>> +Usually OS's will not ship with Git's Git::FromCPAN tree at all,
>> +preferring to use their own packaging of CPAN modules instead.
>
> This is something I wondered about.  What's the recommended
> method to ensure git packaged for an OS/distribution doesn't
> ever use the fallbacks?  Remove $perllibdir/Git/FromCPAN
> after make install?
>
> If so, would it be useful to add a Makefile knob to not
> install the FromCPAN bits, which may be generally useful to
> packagers?
>
> Something like the following, perhaps?
>
> (I'd feel bad suggesting this without a patch, after all the
> work you've already done to simplify and improve the perl
> bits.)
>
>  8< 
> From: Todd Zullinger 
> Date: Wed, 14 Feb 2018 23:00:30 -0500
> Subject: [PATCH] Makefile: add NO_PERL_CPAN to disable fallback module install
>
> As noted in perl/Git/LoadCPAN.pm, operating system packages often don't
> want to ship Git::FromCPAN tree at all, preferring to use their own
> packaging of CPAN modules instead.  Allow such packagers to set
> NO_PERL_CPAN to easily avoid installing these fallback perl CPAN
> modules.
>
> Signed-off-by: Todd Zullinger 
> ---
>  Makefile | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 5bcd83ddf3..c4e035e5bf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -296,6 +296,9 @@ all::
>  #
>  # Define NO_PERL if you do not want Perl scripts or libraries at all.
>  #
> +# Define NO_PERL_CPAN if you do not want to install fallbacks for perl CPAN
> +# modules.
> +#
>  # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
>  # but /usr/bin/python2.7 on some platforms).
>  #
> @@ -2572,6 +2575,7 @@ ifndef NO_GETTEXT
>  endif
>  ifndef NO_PERL
>   $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
> + test -z "$(NO_PERL_CPAN)" || rm -rf perl/build/lib/Git/FromCPAN
>   (cd perl/build/lib && $(TAR) cf - .) | \
>   (cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
>   $(MAKE) -C gitweb install
> --
> 2.16.1
>
> I don't particularly like NO_PERL_CPAN, but I'm confident
> someone else will suggest an obviously better name.
>
> I thought about moving the 'rm -rf Git/FromCPAN' after the
> tar/untar, to keep the files in place for the tests.  No
> tests seem to rely on those local files, so I stuck with
> removing them before.  That diff was:
>
> --- a/Makefile
> +++ b/Makefile
> @@ -2574,6 +2574,7 @@
>   $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
>   (cd perl/build/lib && $(TAR) cf - .) | \
>   (cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
> + test -n "$(NO_PERL_CPAN)" && rm -rf 
> '$(DESTDIR_SQ)$(perllibdir_SQ)'/Git/FromCPAN
>   $(MAKE) -C gitweb install
>  endif
>  ifndef NO_TCLTK

That makes sense, I'll incorporate that in a re-roll. I like
NO_PERL_CPAN_FALLBACKS or just NO_CPAN_FALLBACKS better.

I'd really like to find some solution that works differently though,
because with this approach we'll run the full test suite against a
system where our fallbacks will be in place (although if the OS
distributor has done as promised we won't use them), and then just
remove this at 'make install' time, also meaning we'll re-gen it before
running 'make install' again, only to rm it again.

The former issue we could deal with by munging the Git::LoadCPAN file so
it knows about NO_PERL_CPAN_FALLBACKS, and will always refuse to use the
fallbacks if that's set. That's a good idea anyway, because right now if
you e.g. uninstall Error.pm on Debian (which strips the CPAN fallbacks)
you get a cryptic "BUG: ..." message, it should instead say "we couldn't
get this module the OS promised we'd have" or something to that effect.

The latter is trickier, I don't see an easy way to coerce the Makefile
into not copying the FromCPAN directory without going back to a
hardcoded list again, the easiest thing is probably to turn that:

$(TAR) cf - .)

Into:

$(TAR) cf - $(find ... -not )

Or something like that to get all the stuff that isn't the Git/FromCPAN
directory.

Other suggestions most welcome.

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Hi Jonathan,

Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:
> [...]
>> +++ b/perl/Git/LoadCPAN.pm
>> @@ -0,0 +1,74 @@
> [...]
>> +The Perl code in Git depends on some modules from the CPAN, but we
>> +don't want to make those a hard requirement for anyone building from
>> +source.
> 
> not about this patch: have we considered making it a hard requirement?
> Both Mail::Address and Error.pm are pretty widely available, and I
> wonder if we could make the instructions in the INSTALL file say that
> they are required dependencies to simplify things.
> 
> I admit part of my bias here is coming from the distro world, where we
> have to do extra work to get rid of the FromCPAN embedded copies and
> would be happier not to have to.

Heh, a similar bias is what led me to suggest a Makefile
knob to prevent installing the fallbacks.  I neglected to
add you to the Cc list on that reply.  But I was thinking of
Debian and other distributions whom I know would similarly
want to exclude Git/FromCPAN from their git packages.  I
know it's a simple rm, but it might as well be a simple rm
in one place than spread across each package.  :)

It may still be worth considering whether it's reasonable to
make Mail::Address and Error.pm hard requirements, but I'm
not sure if there are any platforms where such a requirement
would be a pain.  If there are folks here who are happy to
maintain this fallback method, then a simple Makefile knob
gives us the best of both worlds.

-- 
Todd
~~
Historian, n. A broad-gauge gossip.
-- Ambrose Bierce, "The Devil's Dictionary"

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Todd Zullinger]

Hi Ævar,

Ævar Arnfjörð Bjarmason wrote:
> +Git::LoadCPAN - Wrapper for loading modules from the CPAN (OS) or Git's own 
> copy
> +
> +=head1 DESCRIPTION
> +
> +The Perl code in Git depends on some modules from the CPAN, but we
> +don't want to make those a hard requirement for anyone building from
> +source.
> +
> +Therefore the L namespace shipped with Git contains
> +wrapper modules like C that will first
> +attempt to load C from the OS, and if that doesn't work
> +will fall back on C shipped with Git
> +itself.
> +
> +Usually OS's will not ship with Git's Git::FromCPAN tree at all,
> +preferring to use their own packaging of CPAN modules instead.

This is something I wondered about.  What's the recommended
method to ensure git packaged for an OS/distribution doesn't
ever use the fallbacks?  Remove $perllibdir/Git/FromCPAN
after make install?

If so, would it be useful to add a Makefile knob to not
install the FromCPAN bits, which may be generally useful to
packagers?

Something like the following, perhaps?

(I'd feel bad suggesting this without a patch, after all the
work you've already done to simplify and improve the perl
bits.)

 8< 
From: Todd Zullinger 
Date: Wed, 14 Feb 2018 23:00:30 -0500
Subject: [PATCH] Makefile: add NO_PERL_CPAN to disable fallback module install

As noted in perl/Git/LoadCPAN.pm, operating system packages often don't
want to ship Git::FromCPAN tree at all, preferring to use their own
packaging of CPAN modules instead.  Allow such packagers to set
NO_PERL_CPAN to easily avoid installing these fallback perl CPAN
modules.

Signed-off-by: Todd Zullinger 
---
 Makefile | 4 
 1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index 5bcd83ddf3..c4e035e5bf 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,9 @@ all::
 #
 # Define NO_PERL if you do not want Perl scripts or libraries at all.
 #
+# Define NO_PERL_CPAN if you do not want to install fallbacks for perl CPAN
+# modules.
+#
 # Define PYTHON_PATH to the path of your Python binary (often /usr/bin/python
 # but /usr/bin/python2.7 on some platforms).
 #
@@ -2572,6 +2575,7 @@ ifndef NO_GETTEXT
 endif
 ifndef NO_PERL
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
+   test -z "$(NO_PERL_CPAN)" || rm -rf perl/build/lib/Git/FromCPAN
(cd perl/build/lib && $(TAR) cf - .) | \
(cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
$(MAKE) -C gitweb install
-- 
2.16.1

I don't particularly like NO_PERL_CPAN, but I'm confident
someone else will suggest an obviously better name.

I thought about moving the 'rm -rf Git/FromCPAN' after the
tar/untar, to keep the files in place for the tests.  No
tests seem to rely on those local files, so I stuck with
removing them before.  That diff was:

--- a/Makefile
+++ b/Makefile
@@ -2574,6 +2574,7 @@
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perllibdir_SQ)'
(cd perl/build/lib && $(TAR) cf - .) | \
(cd '$(DESTDIR_SQ)$(perllibdir_SQ)' && umask 022 && $(TAR) xof -)
+   test -n "$(NO_PERL_CPAN)" && rm -rf 
'$(DESTDIR_SQ)$(perllibdir_SQ)'/Git/FromCPAN
$(MAKE) -C gitweb install
 endif
 ifndef NO_TCLTK

-- 
Todd
~~
Man has made use of his intelligence, he invented stupidity.
-- Remy De Gourmant

Re: [PATCH 3/8] perl: generalize the Git::LoadCPAN facility

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Change the two wrappers to load from CPAN (local OS) or our own copy
> to do so via the same codepath.

nit: I think with s/to load/that load/ this will be easier to read.

> I added the Error.pm wrapper in 20d2a30f8f ("Makefile: replace
> perl/Makefile.PL with simple make rules", 2017-12-10), and shortly
> afterwards Matthieu Moy added a wrapper for Mail::Address in
> bd869f67b9 ("send-email: add and use a local copy of Mail::Address",
> 2018-01-05).
>
> His was simpler since Mail::Address doesn't have an "import" method,
> but didn't do the same sanity checking, e.g. a missing FromCPAN
> directory (which OS packages are likely not to have) wouldn't be
> explicitly warned about.

I'm having trouble parsing this.  Mail::Address didn't do the same
sanity checking or his didn't?

The comma before e.g. should be a period or semicolon, since it's
starting a new independent clause.

> Now both use a modification of the previously Error.pm-specific
> codepath, which has been amended to take the module to load as
> parameter, as well as whether or not that module has an import method.

Does "now" mean before this patch or after this patch?  Usually
commit messages describe the status quo without the patch in the
present tense and the change the patch will make in the imperative.
So this could say:

Update both to use a common implementation based on the previous
Error.pm loader.

[...]
> +++ b/perl/Git/LoadCPAN.pm
> @@ -0,0 +1,74 @@
[...]
> +The Perl code in Git depends on some modules from the CPAN, but we
> +don't want to make those a hard requirement for anyone building from
> +source.

not about this patch: have we considered making it a hard requirement?
Both Mail::Address and Error.pm are pretty widely available, and I
wonder if we could make the instructions in the INSTALL file say that
they are required dependencies to simplify things.

I admit part of my bias here is coming from the distro world, where we
have to do extra work to get rid of the FromCPAN embedded copies and
would be happier not to have to.

[...]
> +Usually OS's will not ship with Git's Git::FromCPAN tree at all,
> +preferring to use their own packaging of CPAN modules instead.

nit: I think the plural of OS is OSes, or something like
"distributors" or "operating systems".

[...]
> +eval {
> + require $package_pm;
> + 1;
> +} or do {

also not about this patch: this mixed tabs/spacing formatting feels a
bit unusual.  I don't know if it's idiomatic for perl, and if it is
then no complaints; it just stood out a little.  Can
Documentation/CodingGuidelines say something about the preferred
indentation in Perl to avoid having to think about such questions?

> --- a/perl/Git/LoadCPAN/Error.pm
> +++ b/perl/Git/LoadCPAN/Error.pm
> @@ -2,45 +2,9 @@ package Git::LoadCPAN::Error;
>  use 5.008;
>  use strict;
>  use warnings;
> +use Git::LoadCPAN (
> +module => 'Error',
> +import => 1,
> +);

Nice!

Thanks and hope that helps,
Jonathan