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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 25, 2018, 18:56 UTC
Message-ID: <87sh9oew4d.fsf@evledraar.gmail.com>
In-Reply-To: <20180214225754.GC136185@aiede.svl.corp.google.com>

On Wed, Feb 14 2018, Jonathan Nieder jotted:

Show 35 quoted lines

> Æ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.

Show 12 quoted lines

> [...]
>> +++ 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.

Show 6 quoted lines

> [...]
>> +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.

Show 11 quoted lines

> [...]
>> +    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).

Show 15 quoted lines

>> --- 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