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(211288444200000000)
UNIX developer since approximately 421664400
-- In my real life, I talk too much.
