Closed Bug 1251100 Opened 8 years ago Closed 8 years ago

checksetup.pl no longer tells admins which modules are installed and which version is installed

Categories

(Bugzilla :: Installation & Upgrading, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: LpSolit, Assigned: dylan)

References

Details

(Keywords: regression)

Attachments

(1 file, 6 obsolete files)

checksetup.pl no longer says anything about missing or too old modules, nor have you any idea about which features cannot be enabled due to this. |perl Makefile.PL| only mentions required dependencies, but doesn't say anything about optional dependencies, so you won't know that something is missing till your users hit the problem.

--check-modules and normal checks should be brought back into checksetup.pl
Rough plan for this:

use Test::CheckDeps (note: this requires a little glue to feed it the optional dependencies.)
This will not add a dependency as it is acceptable to vendor it (e.g. ship it with bugzilla).
Patch soon. Some formatting issues remain, and still todo: feature detection (when possible, e.g. when _CAN_HAS_FEATURES is true)

dylan@bugzilla htdocs/1251100(master)% perl checksetup.pl                                                                                                                                             19:51
* This is Bugzilla 5.1 on perl 5.16.3
* Running on Linux 3.10.0-327.4.5.el7.x86_64 #1 SMP Mon Jan 25 22:07:14 UTC 2016
Requirements for configure:
Checking for  ExtUtils::MakeMaker (v6.55)     ok: found v7.04
Requirements for runtime:
Checking for                  CGI (v3.51)     ok: found v3.63
Checking for                  DBI (v1.614)    ok: found v1.627
Checking for         Date::Format (v2.23)     ok: found v2.24
Checking for             DateTime (v0.75)     ok: found v1.20
Checking for   DateTime::TimeZone (v1.64)     ok: found v1.92
Checking for          Digest::SHA (any)       ok: found v5.95
Checking for          Email::MIME (v1.904)    ok: found v1.929
Checking for        Email::Sender (v1.300011) ok: found v1.300018
Checking for          File::Slurp (v9999.13)  ok: found v9999.19
Checking for             JSON::XS (v2.01)     ok: found v3.02
Checking for      List::MoreUtils (v0.32)     ok: found v0.413
Checking for  Math::Random::ISAAC (vv1.0.1)   ok: found v1.004
Checking for             Template (v2.24)     ok: found v2.26
Checking for                  URI (v1.55)     ok: found v1.60
Checking for                 perl (v5.010001) ok: found v5.016003
Requirements for build:
Checking for  ExtUtils::MakeMaker (any)       ok: found v7.04
Requirements for test:
Checking for        Pod::Coverage (any)       ok: found v0.23
Checking for           Test::More (any)       ok: found v1.001014
Checking for   Test::Perl::Critic (any)       ok: found v1.03
Checking for              mod_env (any)       ok
Checking for          mod_expires (any)       ok
Checking for          mod_headers (any)       ok
Checking for          mod_rewrite (any)       ok
Checking for          mod_version (any)       ok
Assignee: installation → dylan
There are two really future-proof/not-hacky ways to add this behavior back:

1) Add an explicit dependency on CPAN::Meta::Check
2a) Vendor CPAN::Meta::Check
2b) Vendor CPAN::Meta::Check, but only for checksetup.pl

Note that 2a also allows feature detection to always work, so we can remove CAN_HAS_FEATURE.
Once this is resolved I intent to fix Bug 1256003.

Which option is the least hated?
Flags: needinfo?(dkl)
Flags: needinfo?(LpSolit)
(In reply to Dylan William Hardison [:dylan] from comment #3)
> There are two really future-proof/not-hacky ways to add this behavior back:
> 
> 1) Add an explicit dependency on CPAN::Meta::Check
> 2a) Vendor CPAN::Meta::Check
> 2b) Vendor CPAN::Meta::Check, but only for checksetup.pl
> 
> Note that 2a also allows feature detection to always work, so we can remove
> CAN_HAS_FEATURE.
> Once this is resolved I intent to fix Bug 1256003.
> 
> Which option is the least hated?

As stated in IRC, I vote for number 1 where checksetup.pl just dies early if CPAN::Meta::Check is not installed.

dkl
Flags: needinfo?(dkl)
It's really a shame that Perl doesn't come with a core module to check dependencies. I like none of the solutions, really. :(

Is there no core ExtUtils::MakeMaker::* module to do that?
Flags: needinfo?(LpSolit)
Attached patch 1251100_1.patch (obsolete) — Splinter Review
To make the patch easier to review, this doesn't contain the .checksetup_lib that will be commited. run perl Makefile.PL; make checksetup_lib before testing. 

This should also fix bug 1256003
Attachment #8739508 - Flags: review?(dkl)
Comment on attachment 8739508 [details] [diff] [review]
1251100_1.patch

mst: do you see anything grossly insane with this?
Attachment #8739508 - Flags: feedback?(mst)
Attached patch 1251100_2.patch (obsolete) — Splinter Review
Remove stupid canary from xmlrpc description
Attachment #8739508 - Attachment is obsolete: true
Attachment #8739508 - Flags: review?(dkl)
Attachment #8739508 - Flags: feedback?(mst)
Attachment #8739512 - Flags: review?(dkl)
Attachment #8739512 - Flags: feedback?(mst)
for the sake of bugmail spam, just assume the final patch will not include the .perl-version or the v1 *.patch file.
Despite I have all the required modules installed and they are all correctly detected by checksetup.pl, it still throws:

*** Installation aborted. Read the messages above. ***

But despite this message, it continues till the end.

I think the error is in check_cpan_requirements() where it throws this message if $output is true. But the output is not necessarily an error ($silent = 0 will be verbose).
Attached patch 1251100_3.patch (obsolete) — Splinter Review
cleanup, added some documentation for the new functions
Attachment #8739512 - Attachment is obsolete: true
Attachment #8739512 - Flags: review?(dkl)
Attachment #8739512 - Flags: feedback?(mst)
Attachment #8740535 - Flags: review?(dkl)
Comment on attachment 8740535 [details] [diff] [review]
1251100_3.patch

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

::: Bugzilla/Constants.pm
@@ +25,4 @@
>      LOCAL_FILE
>  
>      bz_locations
> +    

whitespace

::: Bugzilla/Install/Requirements.pm
@@ +42,3 @@
>      check_webdotbase
>      check_font_file
> +    map_files_to_features    

whitespace

@@ +158,5 @@
> +}
> +
> +sub check_cpan_feature {
> +    my ($feature, $dirs, $output) = @_;
> +    

whitespace

@@ +166,5 @@
> +sub check_cpan_requirements {
> +    my ($meta, $dirs, $output) = @_;
> +
> +    my $result = _check_prereqs($meta->effective_prereqs, $dirs, $output);
> +    print colored(install_string('installation_failed'), COLOR_ERROR), "\n" if $output;

print colored(install_string('installation_failed'), COLOR_ERROR), "\n" if (!$result->{ok} && $output);

::: Makefile.PL
@@ +291,5 @@
> +\t-rm -fr .checksetup_lib/lib/perl5/ok.pm
> +\t-find .checksetup_lib '(' -name '*.pod' -or -name .packlist ')' -print0 | xargs -0 rm -f
> +
> +META.json: Makefile.PL
> +\tmake distmeta 2>/dev/null >/dev/null; mv */META.json .

could shorten to 2>&1 /dev/null ?

::: checksetup.pl
@@ +19,4 @@
>  use File::Basename;
>  BEGIN { chdir dirname($0); }
>  use lib qw(. lib local/lib/perl5);
> +use lib qw(.checksetup_lib/lib/perl5);

use lib qw(. lib local/lib/perl5 .checksetup_lib/lib/perl5);

@@ +48,4 @@
>  GetOptions(\%switch, 'help|h|?',
>                       'no-templates|t', 'verbose|v|no-silent',
>                       'make-admin=s', 'reset-password=s', 'version|V',
> +                     'no-permissions|p', 'check-modules');

check-modules not used anywhere
Attachment #8740535 - Flags: review?(dkl) → review-
Attached patch 1251100_4.patch (obsolete) — Splinter Review
This should also work on the old ExtUtils::MakeMaker that ubuntu ships with.
Attachment #8740535 - Attachment is obsolete: true
Attachment #8745434 - Flags: review?(dkl)
Attachment #8745434 - Flags: feedback?(gerv)
Comment on attachment 8745434 [details] [diff] [review]
1251100_4.patch

Not sure what you want from me, but fixing this would be great :-)

Gerv
Attachment #8745434 - Flags: feedback?(gerv) → feedback+
Comment on attachment 8745434 [details] [diff] [review]
1251100_4.patch

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

t/011pod.t ........... 6/379
#   Failed test 'Bugzilla/Install/Requirements.pm has incorrect POD syntax --ERROR'
#   at t/011pod.t line 81.
t/011pod.t ........... 230/379
#   Failed test 'Bugzilla/Install/Requirements.pm POD coverage is 55%. Undocumented methods: check_cpan_module, load_cpan_meta, check_cpan_feature, check_apache_requirements'
#   at t/011pod.t line 109.

#   Failed test 'Bugzilla/Install/Util.pm POD coverage is 95%. Undocumented methods: feature_description'
#   at t/011pod.t line 109.
# Looks like you failed 3 tests of 379.
t/011pod.t ........... Dubious, test returned 3 (wstat 768, 0x300)
Failed 3/379 subtests
t/012throwables.t .... 1/227
#   Failed test 'Bugzilla/Install/Requirements.pm has 1 error(s):
# code error tag 'cpan_meta_missing' is used at line(s) (121) but not defined for language(s): any'
#   at t/012throwables.t line 202.
# Looks like you failed 1 test of 227.
t/012throwables.t .... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/227 subtests

Also META.* will need to be regenerated before committing as the core requirements were removed.

Did you run perltidy against Makefile.pl and others? Seems like a lot of unnecessary formatting changes :)

Actual testing was good. Should be a quick r+ after the above are addressed.

dkl
Attachment #8745434 - Flags: review?(dkl) → review-
Why are these modules suddenly mandatory to run Bugzilla?

ExtUtils::MakeMaker
Pod::Checker
Pod::Coverage
Test::More
Test::Perl::Critic

You only need them to run the testsuite. Bugzilla itself doesn't need them. Is ExtUtils::MakeMaker used to call "perl Makefile.PL"?
Comment on attachment 8745434 [details] [diff] [review]
1251100_4.patch

>+++ b/Makefile.PL

> WriteMakefile(

>+    TEST_REQUIRES    => {
>+        'Test::More'    => 0,
>+        'Pod::Checker'  => 0,
>+        'Pod::Coverage' => 0,
>+    },
>     CONFIGURE_REQUIRES => { 'ExtUtils::MakeMaker' => '6.55' },
>     PREREQ_PM          => \%requires,
>+    TEST_REQUIRES      => {
>         'Test::More'         => 0,
>         'Pod::Checker'       => 0,
>         'Pod::Coverage'      => 0,
>         'Test::Perl::Critic' => 0
>     },

TEST_REQUIRES is listed twice.
Unless I missed it, Apache::SizeLimit is missing in your code. It's required to run mod_perl.
Your patch introduces a regression: the "Diff" links are gone in the attachments table, despite checksetup.pl reports that PatchReader is installed:

Feature 'patch_viewer': Patch Viewer
  Checking for          PatchReader (v0.9.6)    ok: found v0.9.6
(In reply to Frédéric Buclin from comment #16)
> Why are these modules suddenly mandatory to run Bugzilla?
> 
> ExtUtils::MakeMaker
> Pod::Checker
> Pod::Coverage
> Test::More
> Test::Perl::Critic
> 
> You only need them to run the testsuite. Bugzilla itself doesn't need them.
> Is ExtUtils::MakeMaker used to call "perl Makefile.PL"?

Those are required for testing. If you don't want to run tests, you can use --notest
(In reply to Frédéric Buclin from comment #18)
> Unless I missed it, Apache::SizeLimit is missing in your code. It's required
> to run mod_perl.

Apache2::SizeLimit is provided by mod_perl2
(In reply to Frédéric Buclin from comment #19)
> Your patch introduces a regression: the "Diff" links are gone in the
> attachments table, despite checksetup.pl reports that PatchReader is
> installed:
> 
> Feature 'patch_viewer': Patch Viewer
>   Checking for          PatchReader (v0.9.6)    ok: found v0.9.6

Likely the feature 'features' is not enabled. The modules in %all_features are required. In the next patch I'll make that explicit.
Attached patch 1251100_5_master.patch (obsolete) — Splinter Review
Same patch also applies to the 'dylan' branch.

LpSolit: Does it still fail to detect PatchReader is installed?

Note this addresses checksetup looking for 'test' and 'build' dependencies.
Attachment #8745434 - Attachment is obsolete: true
Attachment #8748799 - Flags: review?(dkl)
Attachment #8748799 - Flags: feedback?(LpSolit)
Attached patch 1251100_6_master.patch (obsolete) — Splinter Review
forgot error tag.
Attachment #8748799 - Attachment is obsolete: true
Attachment #8748799 - Flags: review?(dkl)
Attachment #8748799 - Flags: feedback?(LpSolit)
Attachment #8748803 - Flags: review?(dkl)
(In reply to Dylan William Hardison [:dylan] from comment #21)
> Apache2::SizeLimit is provided by mod_perl2

But we require Apache2::SizeLimit 0.96, which is only available since mod_perl2 2.0.6. As we require mod_perl2 1.999022 or newer, we need a separate check for it. Either that, or we should bump the min version for mod_perl2 to 2.0.6.

Also, why did you remove the minimum requirement for mod_perl2? 1.999022 has been dropped.

And about TheSchwartz, you should write 1.10 instead of 1.1, which is confusing (there is no such version upstream).
Comment on attachment 8748803 [details] [diff] [review]
1251100_6_master.patch

This patch still breaks patchreader. Without this patch, the "Diff" links are displayed correctly.
Attachment #8748803 - Flags: feedback-
I found why Diff links are missing: CAN_HAS_FEATURE always evaluates to false. If I convert it into a subroutine, I get the cause of the problem:

Module::Metadata version 1.000023 required--this is only version 1.000019 at Bugzilla/Constants.pm line 230.

Why is such a recent version of Module::Metadata required? Even perl 5.20.3 only has 1.000019.
(In reply to Frédéric Buclin from comment #27)
> I found why Diff links are missing: CAN_HAS_FEATURE always evaluates to
> false. If I convert it into a subroutine, I get the cause of the problem:
> 
> Module::Metadata version 1.000023 required--this is only version 1.000019 at
> Bugzilla/Constants.pm line 230.
> 
> Why is such a recent version of Module::Metadata required? Even perl 5.20.3
> only has 1.000019.

The output of checksetup for me also has a warning before that line that no optional features can work? Are you not getting that warning?

As for the version of Module::Metadata, that is inherited from CPAN::Meta::Check by way of https://rt.cpan.org/Public/Bug/Display.html?id=101095
# ./checksetup.pl 
* This is Bugzilla 5.1 on perl 5.20.1
* Running on Linux 4.4.9-desktop-1.mga5 #1 SMP Tue May 3 20:38:36 UTC 2016
Checking for                  CGI (4.09)      ok: found v4.25 
Checking for                  DBI (1.614)     ok: found v1.631 
Checking for         Date::Format (2.23)      ok: found v2.24 
Checking for             DateTime (0.75)      ok: found v1.12 
Checking for   DateTime::TimeZone (1.64)      ok: found v1.74 
Checking for          Digest::SHA (any)       ok: found v5.88 
Checking for          Email::MIME (1.904)     ok: found v1.926 
Checking for        Email::Sender (1.300011)  ok: found v1.300014 
Checking for  ExtUtils::MakeMaker (6.55)      ok: found v6.98 
Checking for          File::Slurp (9999.13)   ok: found v9999.19 
Checking for             JSON::XS (2.01)      ok: found v3.01 
Checking for      List::MoreUtils (0.32)      ok: found v0.33 
Checking for  Math::Random::ISAAC (v1.0.1)    ok: found v1.004 
Checking for             Template (2.24)      ok: found v2.25 
Checking for                  URI (1.55)      ok: found v1.64 
Checking for                 perl (5.014000)  ok: found v5.020001 

Optional features:
Feature 'auth_delegation': Auth Delegation
  Checking for       LWP::UserAgent (any)       ok: found v6.06 
Feature 'auth_ldap': LDAP Authentication
  Checking for            Net::LDAP (any)       ok: found v0.62 
Feature 'auth_radius': RADIUS Authentication
  Checking for       Authen::Radius (any)       ok: found v0.24 
Feature 'detect_charset': Automatic charset detection for text attachments
  Checking for               Encode (2.21)      ok: found v2.60 
  Checking for       Encode::Detect (any)       ok: found v1.01 
Feature 'documentation': Documentation
  Checking for File::Copy::Recursive (any)       ok: found v0.38 
  Checking for          File::Which (any)       ok: found v1.09 
Feature 'graphical_reports': Graphical Reports
  Checking for                   GD (1.20)      ok: found v2.53 
  Checking for            GD::Graph (any)       ok: found v1.48 
  Checking for             GD::Text (any)       ok: found v0.86 
  Checking for Template::Plugin::GD::Image (any)       ok: found v1.56 
Feature 'html_desc': More HTML in Product/Group Descriptions
  Checking for         HTML::Parser (3.67)      ok: found v3.71 
  Checking for       HTML::Scrubber (any)       ok: found v0.11 
Feature 'inbound_email': Inbound Email
  Checking for         Email::Reply (any)       ok: found v1.203 
  Checking for HTML::FormatText::WithLinks (0.13)      ok: found v0.14 
Feature 'jobqueue': Mail Queueing
  Checking for      Daemon::Generic (any)       not found 
  Checking for          TheSchwartz (1.1)       not found
Feature 'jsonrpc': JSON-RPC Interface
  Checking for            JSON::RPC (any)       ok: found v1.04 
  Checking for          Test::Taint (1.06)      ok: found v1.06 
Feature 'markdown': Markdown syntax support for comments
  Checking for  Text::MultiMarkdown (1.000034)  ok: found v1.000035 
Feature 'memcached': Memcached Support
  Checking for Cache::Memcached::Fast (0.17)      not found 
Feature 'mod_perl': mod_perl support under Apache
  Checking for            mod_perl2 (any)       not found 
Feature 'moving': Move Bugs Between Installations
  Checking for         MIME::Parser (5.406)     ok: found v5.505 
  Checking for            XML::Twig (any)       ok: found v3.48 
Feature 'mysql': MySQL database support
  Checking for           DBD::mysql (4.001)     ok: found v4.028 
Feature 'new_charts': New Charts
  Checking for         Chart::Lines (v2.4.10)   ok: found v2.4.10 
  Checking for                   GD (1.20)      ok: found v2.53 
Feature 'old_charts': Old Charts
  Checking for         Chart::Lines (v2.4.10)   ok: found v2.4.10 
  Checking for                   GD (1.20)      ok: found v2.53 
Feature 'oracle': Oracle database support
  Checking for          DBD::Oracle (1.19)      not found 
Feature 'patch_viewer': Patch Viewer
  Checking for          PatchReader (v0.9.6)    ok: found v0.9.6 
Feature 'pg': Postgres database support
  Checking for              DBD::Pg (v2.19.3)   ok: found v3.4.1 
Feature 'psgi': Plack/PSGI support
  Checking for         CGI::Compile (any)       ok: found v0.17 
  Checking for   CGI::Emulate::PSGI (any)       ok: found v0.20 
  Checking for                Plack (1.0031)    ok: found v1.0039 
Feature 'rest': REST Interface
  Checking for        HTTP::Request (any)       ok: found v6.00 
  Checking for       HTTP::Response (any)       ok: found v6.04 
  Checking for      Module::Runtime (any)       ok: found v0.014 
  Checking for                  Moo (2)         ok: found v2.000002 
  Checking for          Test::Taint (1.06)      ok: found v1.06 
Feature 'smtp_auth': SMTP Authentication
  Checking for         Authen::SASL (any)       ok: found v2.16 
Feature 'smtp_ssl': SSL Support for SMTP
  Checking for       Net::SMTP::SSL (1.01)      ok: found v1.01 
Feature 'sqlite': SQLite database support
  Checking for          DBD::SQLite (1.29)      ok: found v1.42 
Feature 'typesniffer': Sniff MIME type of attachments
  Checking for File::MimeInfo::Magic (any)       ok: found v0.26 
  Checking for           IO::Scalar (any)       ok: found v2.110 
Feature 'updates': Automatic Update Notifications
  Checking for       LWP::UserAgent (any)       ok: found v6.06 
  Checking for            XML::Twig (any)       ok: found v3.48 
Feature 'xmlrpc': XML-RPC Interface
  Checking for           SOAP::Lite (0.712)     ok: found v1.11 
  Checking for          Test::Taint (1.06)      ok: found v1.06 
  Checking for         XMLRPC::Lite (0.712)     ok: found v0.717 
Reading ./localconfig...
Checking for                MySQL (5.0.15)    ok: found v10.0.24-MariaDB 

Removing existing compiled templates...
Precompiling templates...done.
Fixing file permissions...
Checking for             GraphViz (any)       ok 
Checking for            Font file (any)       ok 
checksetup.pl complete.
Interesting. I get this:

All optional features above require the following modules to be found:
  Checking for  CPAN::Meta::Prereqs (2.132830)  ok: found v2.150005
  Checking for CPAN::Meta::Requirements (2.121)     ok: found v2.125
  Checking for     Module::Metadata (1.000023)  found v1.000019
  Checking for      Module::Runtime (any)       ok: found v0.014

which means *some other* dependency is under-specified.
Addressed the concerns from LpSolit's feedback. Hopefully we'll see why the checking for the 'features' feature was not working.

If this continues to be a problem on platforms and the burden of requiring them for all optional features, we will have to vendor (bundle) those dependencies for the regular runtime (instead of just for checksetup)
Attachment #8748803 - Attachment is obsolete: true
Attachment #8748803 - Flags: review?(dkl)
Attachment #8749230 - Flags: review?(dkl)
Blocks: 1270536
Good news, I think we can safely drop the version of Module::Metadata for the time being.
(In reply to Dylan William Hardison [:dylan] from comment #30)
> Interesting. I get this:

To get these lines displayed, I had to run "perl Makefile.PL" before running checksetup.pl. No idea why checksetup.pl needs this step.
Comment on attachment 8749230 [details] [diff] [review]
1251100_7_master.patch

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

Mostly documentation comments. Fix on commit. r=dkl

::: Bugzilla/Install/Requirements.pm
@@ +131,5 @@
> +    foreach my $feature (@features) {
> +        next if $feature->identifier eq 'features';
> +        printf "Feature '%s': %s\n", $feature->identifier, $feature->description if $output;
> +        my $result = check_cpan_feature($feature, $dirs, $output);
> +        print "\n" if $output;

Personally, I liked not having the extra newline in between each feature output before. Now it takes up needless extra vertical space.

@@ +403,5 @@
> +=over
> +
> +=item B<Description>
> +
> +This checks that the optional perl required for a feature are installed.

Incorrect sentence

"This checks that the optional Perl modules required for a feature are installed."

@@ +438,5 @@
> +
> +=item B<Description>
> +
> +This checks what optional perl modules are installed, like
> +C<checksetup.pl> does.

Strange sentence as well. I would not mention anything about what checksetup.pl does or used to do as checksetup.pl just calls this function anyway.

"This checks which optional Perl modules are currently installed which can enable optional features.
Attachment #8749230 - Flags: review?(dkl) → review+
(In reply to David Lawrence [:dkl] from comment #34)
> Personally, I liked not having the extra newline in between each feature
> output before. Now it takes up needless extra vertical space.

Personally, I prefer the extra newline. This makes the output more readable. :)
(In reply to Frédéric Buclin from comment #35)
> (In reply to David Lawrence [:dkl] from comment #34)
> > Personally, I liked not having the extra newline in between each feature
> > output before. Now it takes up needless extra vertical space.
> 
> Personally, I prefer the extra newline. This makes the output more readable.
> :)

OK OK, I do not have a strong opinion either way :)
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   516ac2a..af3c716  master -> master

Includes a downgraded Module::Metadata and a warning about older EU::MM's. It is not possible to update the META.json file under a pure 5.14 install, but everything else works.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: