1246528 - Use Makefile.PL and allow Bugzilla use cpanm-compatible local dependencies

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement)

Description

[Dylan Hardison [:dylan] (he/him)]

Carrying are own toolchain stuff (dependencies, install-module.pl, etc) is really silly.

Comment 1

[Frédéric Buclin]

install-module.pl use CPAN to install stuff. What do you mean exactly?

Comment 2

[Dylan Hardison [:dylan] (he/him)]

install-module.pl ignores the arch prefix. 

Once this works, we can use Carton (or Carmel) and have all sorts of fun things with releases.

We can ship bugzilla tarballs with a vendor/cache directory that is effectively a local cpan mirror, including only the modules Bugzilla is using. These would be the source cpan dists -- nothing pre-compiled.
 (I've tested this for the BMO upstream merge -- it ends up being only 22MB)

That's just one benefit. Another benefit from Carton would be cpanfile.snapshot, which records the version of every module and all its dependencies. No more upgrade scares.

The benefits to using parts of the standard pool toolchain are incredible for the safety of deployments.

Meanwhile, I file patches to make Module::CPANfile work under taint mode.

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_1.patch

This is one hell of a patch.

1) add a cpanfile (that is slightly different than what checksetup would generate, explained below)
2) add documentation on how to use it to install modules
3) because asking the user to use cpanm to poop all over their filesystem seems bad, I'd rather have them install to a local-lib style directory. This lets us be compatible with carton packaging from day 1. Because of this I've added something to every 'use lib'.

#3 is the bulk of the changes -- but this will let the upstream-merge in BMO.
Actually it makes Bug 1247006 almost moot.

Note that feature detection(!) still works as expected.

Also note: this will not work without dylanwh/cpanfile until Module::CPANfile accepts my pull request (or fixes taint mode in another way).

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Ah yes, I didn't explain why my cpanfile is a bit different.

The way optional features in bugzilla are translated by the --cpanfile checksetup parameter is not what recommended means for the CPAN::Meta spec, so I changed all recommends inside features to requires.

Comment 5

[Frédéric Buclin]

Why is Plack suddenly a required module? Why are all the Module:: modules also required? My distro has all the modules I need to run Bugzilla; why should I bother installing Module::? Also, you removed the ability for checksetup.pl to check which modules I have on my machine. This is a major regression! I don't want cpanm to install modules; I want to use my distro package manager, so I need to know what needs to be installed and/or upgraded. And assuming my distro has a too old or missing module, you don't explain how to install one specific module only.

Comment 6

[Dylan Hardison [:dylan] (he/him)]

I can back out plack being a required module (especially since I stub in Bugzilla->feature(), the first iteration of this didn't allow that.

Meanwhile, Module::Runtime is already a dependency of Email::Sender and is the preferred way of loading a module by class name.
Module::CPANfile is required to parse the cpanfile and implement the Bugzilla->feature() method.

As for distro packages -- this is still supported, but you need to use your package manager to install them.
When I build other software from source, I use the package manage (yum or apt) to install all the build-deps of the current release and then I try building/running the git checkout version. Why should Bugzilla be any different?

Using these tools, we can build a tar release of Bugzilla that includes all CPAN dependencies. This isn't theoretical,
I could do this right now by installing carmel (only on my dev machine)
and doing "carmel install && carmel package".

Comment 7

[Dylan Hardison [:dylan] (he/him)]

and doing "carmel install && carmel package" works right now, I mean.

Comment 8

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #6)
> As for distro packages -- this is still supported, but you need to use your
> package manager to install them.

Of course it's still supported. And I know I must use my package manager, as we always did. But this is not my question. My question is:

"Also, you removed the ability for checksetup.pl to check which modules I have on my machine. This is a major regression!"

So how do I get this information now? Having checksetup.pl list them is the right place for it, IMO.


> When I build other software from source, I use the package manage (yum or
> apt) to install all the build-deps of the current release and then I try
> building/running the git checkout version. Why should Bugzilla be any
> different?

That's not what I'm talking about. I *do* want to use my package manager first, so if you remove the feature from checksetup.pl to list required and optional modules, how am I supposed to guess which ones to install/update? And if I miss some, how can I easily install only *some* of the modules listed in cpanfile?


> Using these tools, we can build a tar release of Bugzilla that includes all
> CPAN dependencies. This isn't theoretical,
> I could do this right now by installing carmel (only on my dev machine)
> and doing "carmel install && carmel package".

I honestly hope we are not going to release such tarballs, or that we will at least offer two distinct tarballs.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

(In reply to Frédéric Buclin from comment #8)
> So how do I get this information now? Having checksetup.pl list them is the
> right place for it, IMO.

cpanm (with -l instead of -L) will not install modules that are already installed.
It might upgrade them -- but I can update the docs to add --skip-satisfied which means it won't bother at all.

> 
> > When I build other software from source, I use the package manage (yum or
> > apt) to install all the build-deps of the current release and then I try
> > building/running the git checkout version. Why should Bugzilla be any
> > different?
> 
> That's not what I'm talking about. I *do* want to use my package manager
> first, so if you remove the feature from checksetup.pl to list required and
> optional modules, how am I supposed to guess which ones to install/update?
> And if I miss some, how can I easily install only *some* of the modules
> listed in cpanfile?

They should be listed by the package metadata for the respective distro.

> > Using these tools, we can build a tar release of Bugzilla that includes all
> > CPAN dependencies. This isn't theoretical,
> > I could do this right now by installing carmel (only on my dev machine)
> > and doing "carmel install && carmel package".
> 
> I honestly hope we are not going to release such tarballs, or that we will
> at least offer two distinct tarballs.

A separate build to be sure. But it doesn't add much! If we turn on all features, it's 22M and much, much smaller with just core dependencies.

I can add the ability to check modules using only core perl modules -- but I would do this in a separate script.

Comment 10

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #9)
> cpanm (with -l instead of -L) will not install modules that are already
> installed.
> It might upgrade them -- but I can update the docs to add --skip-satisfied
> which means it won't bother at all.

Yes, please.


> They should be listed by the package metadata for the respective distro.

Which package are you talking about?? I only want to use my package manager to install dependencies, not Bugzilla itself (no Linux distro package development snapshots of Bugzilla).


> I can add the ability to check modules using only core perl modules -- but I
> would do this in a separate script.

So when you run checksetup.pl, is it going to tell you if you have missing or too old dependencies? If not, then I consider this a severe regression and is a showstopper to me.

Comment 11

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #4)
> Ah yes, I didn't explain why my cpanfile is a bit different.
> 
> The way optional features in bugzilla are translated by the --cpanfile
> checksetup parameter is not what recommended means for the CPAN::Meta spec,
> so I changed all recommends inside features to requires.

So does the current --cpanfile functionality need to be fixed apart of this bug? The docker image generation pulls in the extra modules fine currently using --with-all-features so it seem 'recommends' works as well.

dkl

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8717757 [details] [diff] [review]
1246528_1.patch

Review of attachment 8717757 [details] [diff] [review]:
-----------------------------------------------------------------

Some t/001compile.t found the following errors:

1. Known issue: # Insecure dependency in eval while running with -T switch at local/lib/perl5/Module/CPANfile/Environment.pm line 64.
   for: rest.cgi, jsonrpc.cgi and xmlrpc.cgi
2. Missing use lib updates for the following:
   app.psgi
   extensions/create.pl
   t/004template.t
   t/005whitespace.t
   t/007util.t
   t/008filter.t
   t/009bugwords.t
   t/010dependencies.t

t/011pod.t complained that 'has_feature' is undocumented in Bugzilla.pm

docs/en/rst/installing/linux.rst:
Needs to be 'command:`cpanm --installdeps -l local .' for Red Hat based systems.

Also need to add instructions for installing cpanm locally if not installed. (Red Hat uses perl-App-cpanminus).
curl -L https://cpanmin.us | perl - --sudo App::cpanminus
or
yum install perl\(App::cpanminus\)

and then do 'cpanm -l local Module::CPANfile' maybe right after?

Alow would be useful to add another example below:
cpanm --installdeps -l local --with-all-features --without-feature oracle --without-feature mysql --without-feature pg .

if they want the all features of Bugzilla.

I agree with LpSolit it would be nice to have an easy way to see what is installed and what the installed versions are similar to what checksetup.pl does currently. As discussed,
./checksetup.pl --check-modules could load in cpanfile and then do the same output based on version comparisons. You also mentioned the addition of the META.json file for bootstrapping the minimum
required dependencies.

dkl

Comment 13

[mst]

Is there a reason we haven't just used a Makefile.PL here?

Makefile.PL already has the 'check deps' logic, already integrates with cpanm --installdeps, and ExtUtils::MakeMaker has always been core.

I've been recommending you create a Makefile.PL since I've been in #bugzilla and I'm not sure why we're going down overly complicated hipster routes here when the standard approach already has the features required.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_2.patch

No cpanfile, most of the dependencies are core perl things. If you want features, some modules are needed (which we list with 'recommends').

lib is a symlink to local/lib/perl5. Eventually I want to drop the lib dir -- and I will do that at about the same time we drop support for CGI.

Comment 15

[:glob ✱]

how with this work with existing installations with packages already installed with install-module.pl in the lib/ directory?  i don't see any migration code.

Comment 16

[Dylan Hardison [:dylan] (he/him)]

Ah, right. That is why we wanted to add to the path. I'm afraid I would have to touch every file then.

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_3.patch

I'm more confident that this updates every "use lib".
Note that it declares a feature for auth_delegation (which is a bug I uncovered while testing this).
Also it adds more files to the rest feature, as tests failed there too.

Comment 18

[Frédéric Buclin]

Err... why do you remove runtests.pl??

Comment 19

[mst]

(In reply to Frédéric Buclin from comment #18)
> Err... why do you remove runtests.pl??

Because it was an overengineered pointless special snowflake of a script.

Now we're doing things properly, you can just run 'make test' as standard for a well behaved piece of perl software.

Comment 20

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_4.patch

Incremental improvement with requiring the right version of ExtUtils::MakeMaker. Weirdly there are a few build failures on centos6, however I'm not sure these are unique to this approach. dkl: does previous checksetup.pl cpanfile work on centos6?

Comment 21

[Frédéric Buclin]

Why do you require ExtUtils::MakeMaker 6.65? You need Perl 5.18 to have such a high version by default. I know you can use cpanm, but it's painful to have to reinstall a newer version.

Comment 22

[Dylan Hardison [:dylan] (he/him)]

isn't 6.65 in centos 7?

Comment 23

[Dylan Hardison [:dylan] (he/him)]

I can play a bit with the lower bound, but I thought 6.65 was low enough. 
Too low a version and all features won't work because META_MERGE won't be available. 6.55_02 of centos 6 for instance is too old.

Comment 24

[Frédéric Buclin]

Perl 5.14, which we plan to require as the min version, has ExtUtils::MakeMaker 6.57_05.

http://cpansearch.perl.org/src/JESSE/perl-5.14.0/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MakeMaker.pm

Comment 25

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #23)
> Too low a version and all features won't work because META_MERGE won't be
> available. 6.55_02 of centos 6 for instance is too old.

http://perldoc.perl.org/ExtUtils/MakeMaker.html#META_MERGE says that META_MERGE is available since 6.46.

Comment 26

[Dylan Hardison [:dylan] (he/him)]

(In reply to Frédéric Buclin from comment #24)
> Perl 5.14, which we plan to require as the min version, has
> ExtUtils::MakeMaker 6.57_05.
> 
> http://cpansearch.perl.org/src/JESSE/perl-5.14.0/cpan/ExtUtils-MakeMaker/lib/
> ExtUtils/MakeMaker.pm

I question the wisdom in a new release that specifies a new minimum release that is unsupported by p5p.
I guess perl 5.14 would be supported under ubuntu 12.04...
In order of my confidence with a group being able to support perl versions:
* p5p
* redhat
* ubuntu

Comment 27

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_6.patch

1) Warn about old ExtUtils::MakeMaker, rather than die
2) Include a static list of dependencies, including features, in META.* files.
   The cost here is that these need to be updated when the Makefile.PL is updated.
3) Make gen-cpanfile (which is ONLY needed for carton builds) fail at generating a cpanfile rather than output an empty file.
4) Fix the File::Glob :bsd_glob error that happens in 5.10
5) Fix the split() in scalar context in Bugzilla::Markdown. 

By the way, tests wouldn't have been passing under perl 5.10 for a pretty long time. :(

Comment 28

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_7.patch

I moved the modules required for features from "recommends" to a feature called "features" and made them required. The docs will still need to be updated, but for now everything seems to work without --recommends

Comment 29

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_8.patch

Fix required under taint mode for some configurations

Comment 30

[Frédéric Buclin]

Extensions define their lists of required and optional modules in extensions/Foo/Config.pm. How does this now work with your code?

Comment 31

[David Lawrence [:dkl]]

Comment on attachment 8721471 [details] [diff] [review]
1246528_8.patch

Review of attachment 8721471 [details] [diff] [review]:
-----------------------------------------------------------------

1. The section in the docs for installing using yum/apt-get needs to be updated to the latest minimal package set instead of manually specifying all of the perl modules.
2. As LpSolit mentioned, you need to also add the OPTIONAL_MODULES from extensions to Makefile.PL.

Otherwise looking really good. I will need to file dependent bugs to update the default docker config and some of the QA script libraries as well.

dkl

::: Bugzilla.pm
@@ +250,5 @@
> +        require Module::Runtime;
> +        require CPAN::Meta::Check;
> +        Module::Runtime->import(qw(require_module));
> +        CPAN::Meta::Check->import(qw(verify_dependencies));
> +        1;

nit: use 'return 1;' for style consistency.

@@ +300,2 @@
>  
> +        \%map;

nit: use 'return \%map;' for style consistency.

@@ +1052,5 @@
> +Wrapper around C<has_feature()> that also loads all of required modules into the runtime.
> +
> +=item C<has_feature>
> +
> +Consults F<MYMETA.yml> for optional Bugzilla features and returns true if all the requirements

... F<MYMETA.json> or F<META.json> ...

::: Bugzilla/Markdown.pm
@@ -507,5 @@
>  sub _has_multiple_underscores {
>      my $string = shift;
> -    return 0 unless defined($string) && length($string);
> -    return 0 if $string =~ /[\t\s]+/;
> -    return 1 if scalar (split /_/, $string) > 1;

These changes seem unrelated to this bug. Explain?

::: META.json
@@ +1,1 @@
> +{

This seems redundant as it duplicates what is already in Makefile.PL. Are we to add to this file when adding new features or update Makefile.PL?

::: Makefile.PL
@@ +30,5 @@
> +    next if -f File::Spec->catfile($dir, "disabled");
> +    require $file;
> +    my $class = "Bugzilla::Extension::$name";
> +    next unless $class->can("REQUIRED_MODULES");
> +    push @extension_prereqs, map { $_->{module}, $_->{version} } @{ $class->REQUIRED_MODULES() };

Need part to add in OPTIONAL_MODULES from extensions dynamically.

@@ +35,5 @@
> +}
> +
> +WriteMakefile(
> +    NAME               => 'Bugzilla',
> +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},

Is this a real address? Never heard of it.

@@ +37,5 @@
> +WriteMakefile(
> +    NAME               => 'Bugzilla',
> +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},
> +    VERSION            => BUGZILLA_VERSION,
> +    ABSTRACT           => 'Bugzilla bug tracking system',

nit: 'Bugzilla Bug Tracking System',

@@ +39,5 @@
> +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},
> +    VERSION            => BUGZILLA_VERSION,
> +    ABSTRACT           => 'Bugzilla bug tracking system',
> +    LICENSE            => 'Mozilla_2_0',
> +    MIN_PERL_VERSION   => '5.10.1',

This may change depending on if bug 1136137 makes it for 6.0.

@@ +66,5 @@
> +            features => {
> +                prereqs => { runtime => { requires => { 'CPAN::Meta::Check' => 0, 'Module::Runtime' => 0, 'CPAN::Meta' => 0, }, }, },
> +                description => "Base support for Features"
> +            },
> +

nit: remove extra new-line.

::: docs/en/rst/installing/linux.rst
@@ +158,2 @@
>  
> +:command:`curl http://cpanmin.us | perl - --installdeps -l local .`

curl -L

@@ +162,2 @@
>  
> +:command:`curl http://cpanmin.us | perl - --installdeps -l local --with-all-features --without-feature mod_perl --without-feature oracle --without-feature mysql --without-feature pg .`

curl -L

::: docs/en/rst/installing/mac-os-x.rst
@@ +78,2 @@
>  
> +:command:`curl http://cpanmin.us | perl - --installdeps -l local .`

curl -L

@@ +82,2 @@
>  
> +:command:`curl http://cpanmin.us | perl - --installdeps -l local --with-all-features --without-feature oracle --without-feature mysql --without-feature pg`

curl -L

::: gen-cpanfile.pl
@@ +9,5 @@
> +# This file has detailed POD docs, do "perldoc checksetup.pl" to see them.
> +
> +######################################################################
> +# Initialization
> +######################################################################

nit: unnecessary and inaccurate header

@@ +30,5 @@
> +            'with-feature|D=s@'    => \@with_feature,
> +            'without-feature|U=s@' => \@without_feature
> +        );
> +
> +

nit: remove extra new-line.

@@ +34,5 @@
> +
> +        my $meta = CPAN::Meta->load_file("MYMETA.json");
> +
> +        my @phases = qw(configure build test develop runtime);
> +        my @types  = qw(requires recommends suggests conflicts);

Do not see where either of these are actually used.

Comment 32

[Dylan Hardison [:dylan] (he/him)]

(In reply to David Lawrence [:dkl] from comment #31)
> Comment on attachment 8721471 [details] [diff] [review]
> 1246528_8.patch
> 
> Review of attachment 8721471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1. The section in the docs for installing using yum/apt-get needs to be
> updated to the latest minimal package set instead of manually specifying all
> of the perl modules.
> 2. As LpSolit mentioned, you need to also add the OPTIONAL_MODULES from
> extensions to Makefile.PL.

I already wrote that while testing it against BMO. :-D
Amusingly, the docs imply that each optional module get a features => [] list,
but in practice this is not the case (Both Bitly and Push do not do this on BMO).
So in that case I now generate a feature called extension_lc($name)_optional.
I avoid using 'recommends' because it is more reliable to install with cpanm without adding in recommended modules.

Note this will need to be relnoted for 6.0 -- the current extension API says you can define REQUIRED_MODULES and OPTIONAL_MODULES in either Extension.pm or Config.pm, and there is no way we can support the former.
Eventually I'd like to change how extensions depend on stuff anyway...

> Otherwise looking really good. I will need to file dependent bugs to update
> the default docker config and some of the QA script libraries as well.
> 
> dkl
> 

> 
> @@ +1052,5 @@
> > +Wrapper around C<has_feature()> that also loads all of required modules into the runtime.
> > +
> > +=item C<has_feature>
> > +
> > +Consults F<MYMETA.yml> for optional Bugzilla features and returns true if all the requirements
> 
> ... F<MYMETA.json> or F<META.json> ...
> 
> ::: Bugzilla/Markdown.pm
> @@ -507,5 @@
> >  sub _has_multiple_underscores {
> >      my $string = shift;
> > -    return 0 unless defined($string) && length($string);
> > -    return 0 if $string =~ /[\t\s]+/;
> > -    return 1 if scalar (split /_/, $string) > 1;

Changed to remove "Use of implicit split to @_ is deprecated" warning during testing. There is another change to use File::Glob for similar reasons.

As an added benefit, the /_/ code is faster and it makes it more obvious that the method name is wrong...

> These changes seem unrelated to this bug. Explain?
> 
> ::: META.json
> @@ +1,1 @@
> > +{
> 
> This seems redundant as it duplicates what is already in Makefile.PL. Are we
> to add to this file when adding new features or update Makefile.PL?

We can either force Makefile.PL to die when ExtUtils::MakeMaker is too old to generate a MYMETA.json
or include this file. I generate it with "perl Makefile.PL; make distmeta; mv Bugzilla/META.* ."

It is a side effect of our users expecting a checkout to work like a release.
I think this is pretty similar to committing a configure (which is done in autotools projects for similar reasons).

> 
> ::: Makefile.PL
> @@ +30,5 @@
> > +    next if -f File::Spec->catfile($dir, "disabled");
> > +    require $file;
> > +    my $class = "Bugzilla::Extension::$name";
> > +    next unless $class->can("REQUIRED_MODULES");
> > +    push @extension_prereqs, map { $_->{module}, $_->{version} } @{ $class->REQUIRED_MODULES() };
> 
> Need part to add in OPTIONAL_MODULES from extensions dynamically.
Yup, already there.

> @@ +35,5 @@
> > +}
> > +
> > +WriteMakefile(
> > +    NAME               => 'Bugzilla',
> > +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},
> 
> Is this a real address? Never heard of it.
No, but I'd love to know what address should go there.

> @@ +37,5 @@
> > +WriteMakefile(
> > +    NAME               => 'Bugzilla',
> > +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},
> > +    VERSION            => BUGZILLA_VERSION,
> > +    ABSTRACT           => 'Bugzilla bug tracking system',
> 
> nit: 'Bugzilla Bug Tracking System',
> 
> @@ +39,5 @@
> > +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},
> > +    VERSION            => BUGZILLA_VERSION,
> > +    ABSTRACT           => 'Bugzilla bug tracking system',
> > +    LICENSE            => 'Mozilla_2_0',
> > +    MIN_PERL_VERSION   => '5.10.1',
> 
> This may change depending on if bug 1136137 makes it for 6.0.

I put it at 5.10 for the sake of BMO, which I suspect will be on 5.10 for some longer period.

> 
> @@ +66,5 @@
> > +            features => {
> > +                prereqs => { runtime => { requires => { 'CPAN::Meta::Check' => 0, 'Module::Runtime' => 0, 'CPAN::Meta' => 0, }, }, },
> > +                description => "Base support for Features"
> > +            },
> > +
> 
> nit: remove extra new-line.
> 
> ::: docs/en/rst/installing/linux.rst
> @@ +158,2 @@
> >  
> > +:command:`curl http://cpanmin.us | perl - --installdeps -l local .`
> 
> curl -L
> 
> @@ +162,2 @@
> >  
> > +:command:`curl http://cpanmin.us | perl - --installdeps -l local --with-all-features --without-feature mod_perl --without-feature oracle --without-feature mysql --without-feature pg .`
> 
> curl -L
> 
> ::: docs/en/rst/installing/mac-os-x.rst
> @@ +78,2 @@
> >  
> > +:command:`curl http://cpanmin.us | perl - --installdeps -l local .`
> 
> curl -L
> 
> @@ +82,2 @@
> >  
> > +:command:`curl http://cpanmin.us | perl - --installdeps -l local --with-all-features --without-feature oracle --without-feature mysql --without-feature pg`
> 
> curl -L
> 
> ::: gen-cpanfile.pl
> @@ +9,5 @@
> > +# This file has detailed POD docs, do "perldoc checksetup.pl" to see them.
> > +
> > +######################################################################
> > +# Initialization
> > +######################################################################
> 
> nit: unnecessary and inaccurate header
> 
> @@ +30,5 @@
> > +            'with-feature|D=s@'    => \@with_feature,
> > +            'without-feature|U=s@' => \@without_feature
> > +        );
> > +
> > +
> 
> nit: remove extra new-line.
> 
> @@ +34,5 @@
> > +
> > +        my $meta = CPAN::Meta->load_file("MYMETA.json");
> > +
> > +        my @phases = qw(configure build test develop runtime);
> > +        my @types  = qw(requires recommends suggests conflicts);
> 
> Do not see where either of these are actually used.
woops, legacy before I discovered the effective_prereqs api.

Comment 33

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_9.patch

Optional module support + everything *except* the documentation fixes

Comment 34

[Frédéric Buclin]

>>> +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},
>> 
>> Is this a real address? Never heard of it.
>No, but I'd love to know what address should go there.

You could put the developers@ mailing-list.

Comment 35

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #34)
> >>> +    AUTHOR             => q{Bugzilla Developers <[e-mail]>},
> >> 
> >> Is this a real address? Never heard of it.
> >No, but I'd love to know what address should go there.
> 
> You could put the developers@ mailing-list.

Sounds good to me too.

Comment 36

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_10.patch

This patch adds the curl -L, fixes the author and makes the optional requirements of extensions work (the previous one didn't)

Comment 37

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_11.patch

Comment 38

[David Lawrence [:dkl]]

Comment on attachment 8722199 [details] [diff] [review]
1246528_11.patch

Review of attachment 8722199 [details] [diff] [review]:
-----------------------------------------------------------------

We need to be able to handle extensions strings.txt.pl, i.e.: extensions/Example/template/en/default/setup/strings.txt.pl

We could add a new description key to OPTIONAL_MODULES but that is not localizable. 

Also I am not able to get extension requires to show up in MYMETA.json. It is in Makefile and is written out to Bugzilla-5.1/META.json if I do 'make metafile' but that is not ideal.

dkl

Comment 39

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1246528_12.patch

I don't think the amount of work required to get feature descriptions from strings.pl is worth it. 

This patch fixes the problem introduced on systems with CPAN::Meta installed and the static META.json/yml files. We hide them from MakeMaker.
(also considered: monkey patching EU::MM).

Comment 40

[David Lawrence [:dkl]]

Comment on attachment 8722500 [details] [diff] [review]
1246528_12.patch

Review of attachment 8722500 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl. We also need the updated installation docs. New bug? or one more patch for quick review?

::: Makefile.PL
@@ +239,5 @@
> +    TEST_REQUIRES => { 'Test::More' => 0, 'Pod::Coverage' => 0, 'Test::Perl::Critic' => 0, },
> +    META_MERGE    => {
> +        "meta-spec"    => { url => "http://search.cpan.org/perldoc?CPAN::Meta::Spec", version => "2" },
> +        dynamic_config => 1,
> +        dog => 1,

nit: align =>

Comment 41

[Dylan Hardison [:dylan] (he/him)]

The install docs don't seem to be wrong at the moment -- with the cpanm instructions being there for OSX and linux. 
I think I'll rewrite the quickstart guide in a separate bug to be a bit more general (making use of some of the changes I've made to ease installation friction). And Bug 1250653 will in part be used to revise the windows instructions.

Comment 42

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   a8adffb..2c33712  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   2c33712..0856fd9  master -> master