Last Comment Bug 646578 - Math::Random::Secure's dependencies frequently do not install properly
: Math::Random::Secure's dependencies frequently do not install properly
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 4.0
: All All
: P1 critical (vote)
: Bugzilla 3.4
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
Depends on: CVE-2010-4568
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-30 12:04 PDT by Frédéric Buclin
Modified: 2011-04-27 15:06 PDT (History)
7 users (show)
mkanat: approval+
mkanat: approval4.0+
mkanat: blocking4.0.1+
mkanat: approval3.6+
LpSolit: blocking3.6.5+
mkanat: approval3.4+
LpSolit: blocking3.4.11+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Remove MRS (3.2 and 3.4) (1.99 KB, patch)
2011-04-16 16:17 PDT, Max Kanat-Alexander
no flags Details | Diff | Review
Remove MRS (3.4 and 3.2) v2 (3.18 KB, patch)
2011-04-16 16:24 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6) (1.30 KB, patch)
2011-04-16 17:06 PDT, Max Kanat-Alexander
no flags Details | Diff | Review
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6) v2 (1.72 KB, patch)
2011-04-17 13:15 PDT, Max Kanat-Alexander
glob: review+
Details | Diff | Review

Description Frédéric Buclin 2011-03-30 12:04:41 PDT
There is an agreement on IRC between justdave, dkl, wicked, glob and myself that Math::Random::Secure has too many dependencies (especially Crypt::Random::Source and Any::Mouse) and is causing a fair amount of trouble to some admins. This module is optional for Bugzilla 4.0 and older, but is currently required to install 4.1.x.

We are pretty confident that all this trouble can be avoided by doing things differently, e.g. by calling Math::Random::ISAAC ourselves (which has no dependencies) and/or Win32::GuidGen() on Windows (see bug 619594 comment 158 and bug 619594 comment 163 about Win32::GuidGen() and how it could easily be used). This would avoid headaches to some admins.
Comment 1 Max Kanat-Alexander 2011-03-30 17:40:52 PDT
  All of the reports from admins resulted from Math::Random::ISAAC having broken dependencies. So your suggestion wouldn't even have helped. Math::Random::ISAAC now has fixed dependencies, so there shouldn't be an issue here except for people who have outdated CPAN access or mirroring.

  The only other present issue is 64-bit Windows, but the ActiveState maintainer is working closely with me on that, and by the time we actually have a stable 4.2 release, that problem should be gone.

  I do agree that if this is still a serious problem by the time that we become close to a stable 4.2, though, that we should switch back to the module being optional on the 4.2 branch.
Comment 2 Max Kanat-Alexander 2011-03-30 17:53:06 PDT
FWIW, here is the full dependency chain required to have a working Math::Random::Secure:

Any::Moose
  Mouse
Crypt::Random::Source
  Capture::Tiny
  Sub::Exporter
    Data::OptList
      Params::Util
  Sub::Install
  namespace::clean
    Package::Stash
    Sub::Name
    Sub::Identify
  Module::Find
Math::Random::ISAAC


For comparison, here's the dependency list for DateTime (which has identical problems as you are describing to Math::Random::Secure and yet has never been suggested for removal):

DateTime::Locale
  List::MoreUtils
DateTime::TimeZone
  parent
  Class::Load
  Class::Singleton
Params::Validate
  Attribute::Handlers

So if you're concerned about dependencies, it's Crypt::Random::Source you should really be worried about. Without that, M::R::S would have a shorter dependency list than DateTime. However, there are no user reports of Crypt::Random::Source being a source of problems in any way.
Comment 3 Teemu Mannermaa (:wicked) 2011-03-31 05:53:06 PDT
FWIW, I don't know what the right solution is but I definitely agree that something must be done to help people facing MRS/Mouse related install problems.

(In reply to comment #0)
> to some admins. This module is optional for Bugzilla 4.0 and older, but is
> currently required to install 4.1.x.

It seems that if people use the suggested --all switch (which they do) for install-module.pl it will try to install MRS too. So it really is "required" for 4.0 and that's where the problems I've seen have been. Including for me on getting cg-bugs01 up and running.

On the other hand, --all doesn't install enough. There have been cases on IRC where it says MRS is installed but especially Mouse and Math::Random::ISAAC are missing (so the irand() call will crash Bugzilla). And retry will only try to install direct dependencies so one needs to know what to install from CPAN (using install-module.pl), which has been a nightmare since CPAN dependency list fails for MRS (for some reason).

(In reply to comment #2)
> FWIW, here is the full dependency chain required to have a working

Are you sure that's all that's installed with |install-module.pl Math::Random::Source|? Since I've seen a lot of things scroll by with that and it seems way more than that. And this included some core modules too.

I guess this is related to the build time requires you mentioned (core ones were Test related thingies, IIRC) but they are as important to count here as long as we recommend install-module.pl for people.

> Math::Random::ISAAC

You're missing at least Math::Random::ISAAC::XS, which was the source of one the problems (circular dependency, which is indeed fixed now like you said).

I've also seen missing Mouse and MRO::Compat (!) issues for people coming to IRC. There's also the scary "Prototype mismatch: sub Mouse::Util::get_linear_isa ($;$) vs none" error coming from Mouse. It seems to only scare people since things seems to work without fixing that. Too bad it's not clearly labeled as a warning. I think this is also related to older Perl installs.

> For comparison, here's the dependency list for DateTime (which has identical

Difference is that these haven't been such a problem to install or that they are already included in core Perl. (On all major distributions.) Also, it doesn't require Mouse/Moose.

> So if you're concerned about dependencies, it's Crypt::Random::Source you
> should really be worried about. Without that, M::R::S would have a shorter

Can CRS be replaced or made truly optional?
Comment 4 Max Kanat-Alexander 2011-03-31 16:00:12 PDT
(In reply to comment #3)
> On the other hand, --all doesn't install enough. There have been cases on IRC
> where it says MRS is installed but especially Mouse and Math::Random::ISAAC are
> missing (so the irand() call will crash Bugzilla). 

  Hey wicked. The "Math::Random::ISAAC is missing" problem is the one I mentioned above, it was a dependency chain problem caused by the author of Math::Random::ISAAC, and he a few weeks ago he released a fix. People who have some sort of outdated CPAN cache or something may still have an older version, but otherwise it should be fine.

  I've only seen one report of Mouse not being installed, and that was from Wurblzap. Any::Moose explicitly lists Mouse as a dependency, so there's no standard way that somebody could install Math::Random::Secure without installing Mouse, and install-module.pl certainly doesn't do that in any of my tests. (This is a different story on 3.4 and 3.2, though, which does not have some install-module.pl fixes and is likely to fail on Perl 5.8 while installing MRS. I'm happy to discuss addressing this on those branches.)

> There's also the scary "Prototype mismatch: sub
> Mouse::Util::get_linear_isa ($;$) vs none" error coming from Mouse. It seems to
> only scare people since things seems to work without fixing that. Too bad it's
> not clearly labeled as a warning. I think this is also related to older Perl
> installs.

  Okay. Well, I'm sure that that is a bug that can be fixed. Perhaps we need to require a newer version of Mouse?

> Difference is that these haven't been such a problem to install or that they
> are already included in core Perl. (On all major distributions.)

  No, actually, they've been a terrific problem to install. In fact, I'd say the most-frequently-reported installation issue is the failure of the DateTime dependencies to install properly.

> Can CRS be replaced or made truly optional?

  Perhaps, but why replace it? There's no demonstration of it causing any problems whatsoever.

  As far as addressing all of your other points, talk to me on IRC. I've already talked to justdave and glob about it, and a lot of what you're bringing up simply requires discussion or a real-time explanation process. Remember that out of all of us, I spend the most time monitoring the support list, and I also spent over 100 hours researching cryptographically secure random number generators and writing/fixing Math::Random::Secure and its dependencies. I have a pretty broad view on what really is happening with our users, what any actual problems are, and what some good solutions could be.
Comment 5 Teemu Mannermaa (:wicked) 2011-04-03 16:47:35 PDT
(In reply to comment #4)
>   I've only seen one report of Mouse not being installed, and that was from
> Wurblzap. Any::Moose explicitly lists Mouse as a dependency, so there's no

There have been many more on IRC. Both Mouse and MRO::Compat have been problematic for 4-5 people I've helped there.

There's one there at this very moment!

> standard way that somebody could install Math::Random::Secure without
> installing Mouse, and install-module.pl certainly doesn't do that in any of my

Is CPAN/install-module.pl a "standard way" to install MRS? Since that can leave it broken if Mouse fails to install but MRS install correctly. I can see this happening if Mouse requires (or one of it's requirement) a compiler tool chain to install and the system doesn't have it.

Whatever the cause, it's happening to people. Looks like CentOS 5 could be a common platform for the people seeking support on this issue. It does have Perl 5.8, so maybe the prototype error and missing MRO::Compat are unique problems to it.

> > There's also the scary "Prototype mismatch: sub
> > Mouse::Util::get_linear_isa ($;$) vs none" error coming from Mouse. It 
>   Okay. Well, I'm sure that that is a bug that can be fixed. Perhaps we need > to require a newer version of Mouse?

No idea. I doubt solution is a newer version since CPAN only hosts the very latest. Please, can you find out what's the problem and fix it?

> the most-frequently-reported installation issue is the failure of the DateTime
> dependencies to install properly.

Oh, okay. I haven't seen any lately and maybe I wasn't around when that was hot issue.
 
> > Can CRS be replaced or made truly optional?
>   Perhaps, but why replace it? There's no demonstration of it causing any
> problems whatsoever.

It's requiring Mouse, which is causing problems for many people coming to IRC  looking for help. Replacing that would remove our requirement on Mouse so that would help get things under control.

Hmm, according to your list it's MRS that actually requires Mouse? Can you remove that requirement? Why is it required? That would certainly solve a lot of problems.

IMO, Moose/Mouse should be banned from Bugzilla. This based on what their authors say about their own creation.

> out of all of us, I spend the most time monitoring the support list, and I 
> spent over 100 hours researching cryptographically secure random number
> generators and writing/fixing Math::Random::Secure and its dependencies. I 

Have you been over IRC or read over its logs? Why not do a search on irand() to read about the problems related to that. These errors are what I'm talking about and I've seen be the problem reported every time I go on IRC.

> a pretty broad view on what really is happening with our users, what any 
> problems are, and what some good solutions could be.

So, what would you do fix these problems and get our support volume down on this issue?
Comment 6 Max Kanat-Alexander 2011-04-03 22:17:35 PDT
(In reply to comment #5)
> There have been many more on IRC. Both Mouse and MRO::Compat have been
> problematic for 4-5 people I've helped there.

  Okay. We should determine why that problem is happening and work to correct it, just like we have for other problematic modules in the past (for example, DateTime).

> Is CPAN/install-module.pl a "standard way" to install MRS? Since that can leave
> it broken if Mouse fails to install but MRS install correctly.

  That's certainly a problem--MRS should not install if Mouse fails to install.

> I can see this
> happening if Mouse requires (or one of it's requirement) a compiler tool chain
> to install and the system doesn't have it.

  Mouse doesn't *require* a compiler (AFAIK) but it does have an XS extension that makes it faster if it's available.

> Whatever the cause, it's happening to people. Looks like CentOS 5 could be a
> common platform for the people seeking support on this issue. It does have Perl
> 5.8, so maybe the prototype error and missing MRO::Compat are unique problems
> to it.

  Hmm, could be. We should figure out specifically what's going on.

> No idea. I doubt solution is a newer version since CPAN only hosts the very
> latest. Please, can you find out what's the problem and fix it?

  Sure, I'd love to, but I need to be in direct contact with a user who's actually having the problem.

> It's requiring Mouse,

  So is MRS itself.

> IMO, Moose/Mouse should be banned from Bugzilla. This based on what their
> authors say about their own creation.

  I hate to say this, but I think you are making a judgment there based on insufficient information and perhaps not a broad enough experience or knowledge of the world of Perl software development today. I'd suggest you become more involved with the Perl community before making judgments about the best way to develop modern Perl software. (I'm not saying there's anything wrong with you, just that there's a tremendous amount of information and knowledge out there in the Perl community that we can be a little isolated from sometimes, in the Bugzilla Project, but is very worthwhile.)

> So, what would you do fix these problems and get our support volume down on
> this issue?

  The same as we do for every other issue that leads to support volume. Do a sensible investigation of the actual root causes of the issue until we fully understand it, and then design a sensible solution for those issue or issues.
Comment 7 Frédéric Buclin 2011-04-13 12:36:58 PDT
(In reply to comment #2)
> However, there are no user reports of
> Crypt::Random::Source being a source of problems in any way.

That's not true. And one user is reporting problems to install it on IRC right now (Params::Util refuses to install correctly). I still think we don't need Crypt::Random::Source at all. Relevant code (without all its dependencies) could be integrated into Math::Random::Secure, which would also avoids all these problems.
Comment 8 Max Kanat-Alexander 2011-04-13 16:30:10 PDT
  Okay, we should investigate what's up with Params::Util. I don't want any decisions made about this until we understand what the actual root cause of these problems is.
Comment 9 Frédéric Buclin 2011-04-14 16:23:47 PDT
Another user came on IRC today, and his error is again "Can't locate Params/Util.pm in @INC" when trying to access Crypt::Random::Source, as in comment 7. So either Crypt::Random::Source is broken, or Params::Util itself is.
Comment 10 Frédéric Buclin 2011-04-14 16:27:00 PDT
In comment 9, the user is using Centos 5.4 with Perl 5.8.8.
Comment 11 Frédéric Buclin 2011-04-14 16:38:46 PDT
Hum, okay, the user was having the error about Params::Util because I told him to use "perl -I lib ..." instead of "perl -Mlib=lib ...". Now he gets:

"Can't locate Moose.pm in @INC". Probably the same is true for the other user yesterday (as I also told him to use -I lib instead of -Mlib=lib). So maybe we should rather focus on Moose/Mouse and leave Params::Util alone, as install-module.pl says Params::Util is already installed.
Comment 12 Max Kanat-Alexander 2011-04-14 17:00:47 PDT
Okay. So it sounds like the problem is that for some reason, Any::Moose can be installed even when Mouse fails to install. This should *not* be happening, but it is probably because we do "notest" when we install modules. Probably the simplest solution here for now is to do a normal install for Math::Random::Secure instead, so it will fail properly.
Comment 13 Frédéric Buclin 2011-04-14 17:53:45 PDT
The summary should rather focus on Any::Moose / Mouse as the dependency loop with ISAAC has been fixed by its author, and I don't remember any other problem reported against other dependencies. Most of us agree that it should be:

 "Remove Any::Mouse / Mouse / Moose from our dependency chain"
Comment 14 Frédéric Buclin 2011-04-14 18:09:41 PDT
For 4.0.1, we should exclude Math::Random::Secure from install-module.pl --all as it triggers problems for a lot of people. That's the minimum we should do. My previous comment still stands, though.
Comment 15 Byron Jones ‹:glob› 2011-04-14 18:14:55 PDT
while enabling the tests is very sane, it won't resolve the issue we're having; it'll only result in a different error message (abet a much clearer one).  it's clear some distros have packaging issues (most notably rpm based distros).

it seems to me it would be easier to retool Math::Random::Secure::RNG to not use moose's syntax sugar, or drop it and use Crypt::Random::Source directly.
Comment 16 Max Kanat-Alexander 2011-04-14 21:17:45 PDT
(In reply to comment #15)
> while enabling the tests is very sane, it won't resolve the issue we're having;

  No, it will. The tests of Any::Moose will fail without Mouse installed, so it will not install, and we won't see this problem.
Comment 17 Byron Jones ‹:glob› 2011-04-14 22:06:12 PDT
(In reply to comment #16)
>   No, it will. The tests of Any::Moose will fail without Mouse installed, so it
> will not install, and we won't see this problem.

you won't see this error message, however the crux of the problem is mouse not being installed when it should be, not the error message.
Comment 18 Max Kanat-Alexander 2011-04-14 22:13:17 PDT
(In reply to comment #17)
> you won't see this error message, however the crux of the problem is mouse not
> being installed when it should be, not the error message.

  Well, sort of. The crux of the problem is that Bugzilla (checksetup.pl) suddenly dies and falls over and users have no idea what's going on. If MRS is going to fail to install, it should do so cleanly. Once it does that, the "tell users to install a compiler" bug will solve the rest of the problem.
Comment 19 Byron Jones ‹:glob› 2011-04-14 22:15:49 PDT
bear in mind any changes we make to install-module.pl won't help users who have to install modules from an rpm.
Comment 20 Dave Miller [:justdave] (justdave@bugzilla.org) 2011-04-15 00:34:07 PDT
Also for Mozilla's installation....  I've got webheads and staging servers in multiple architectures.  Whatever goes into bugzilla/lib is going to get distributed to all of the webheads.  If we have to locally install something with install-module.pl that has to be architecture-specific compiled that's going to make things really complicated, because I'm going to need to make sure there's both i386 and x86_64 versions in there somehow.  That's going to basically mean I'm going to avoid using install-module.pl to install anything that needs an arch-specific build and require RPMs for those instead.
Comment 21 Max Kanat-Alexander 2011-04-15 12:28:33 PDT
(In reply to comment #19)
> bear in mind any changes we make to install-module.pl won't help users who have
> to install modules from an rpm.

  True, but they won't be hitting this particular bug.

(In reply to comment #20)
> That's going to
> basically mean I'm going to avoid using install-module.pl to install anything
> that needs an arch-specific build and require RPMs for those instead.

  Sure, that sounds like a sensible practice. I will keep pushing on the rpmforge guys to actually release the package they checked in months ago.
Comment 22 Frédéric Buclin 2011-04-15 15:08:09 PDT
Another user on IRC just got: http://pastie.org/private/shvjtfjngyxfa07kq9j6w

 NUFFIN/Crypt-Random-Source-0.07.tar.gz
 /usr/bin/make -- OK
Running make test
t/proc.t ................ 1/? Can't call method "read" on an undefined value at /private/var/root/.cpan/build/Crypt-Random-Source-0.07-jvbCBL/blib/lib/Crypt/Random/Source/Base/Handle.pm line 38.
Result: FAIL
Comment 23 Max Kanat-Alexander 2011-04-16 16:09:43 PDT
  Okay, it took me only about 1/2 hour today to discover that the problem was a bug in Param::Util's Makefile.PL, which I have filed and patched here:

  https://rt.cpan.org/Ticket/Display.html?id=67522

  I will also look into more immediate workarounds in install-module.
Comment 24 Max Kanat-Alexander 2011-04-16 16:15:53 PDT
  For the 3.4 and 3.2 branches, this problem is considerably worse--install-module.pl cannot install MRS at all, because it doesn't require a new-enough CPAN for older Perl releases (namely, 5.8.x). We only made install-module require a new CPAN starting with 3.6.

  So for 3.4 and 3.2, I'm going to totally remove the optional MRS requirement; there's no realistic way to make it work, and that fact is a part of Bugzilla's installation infrastructure that we need to fix.
Comment 25 Max Kanat-Alexander 2011-04-16 16:17:09 PDT
Created attachment 526534 [details] [diff] [review]
Remove MRS (3.2 and 3.4)
Comment 26 Max Kanat-Alexander 2011-04-16 16:17:55 PDT
Comment on attachment 526534 [details] [diff] [review]
Remove MRS (3.2 and 3.4)

This patch also applies to the 3.4 branch without changes.
Comment 27 Max Kanat-Alexander 2011-04-16 16:24:02 PDT
Created attachment 526535 [details] [diff] [review]
Remove MRS (3.4 and 3.2) v2

Here's a fixed version (the previous patch forgot to update mod_perl.pl).
Comment 28 Max Kanat-Alexander 2011-04-16 16:49:15 PDT
I have also filed a bug against CPAN.pm:

  https://rt.cpan.org/Ticket/Display.html?id=67523
Comment 29 Max Kanat-Alexander 2011-04-16 17:06:10 PDT
Created attachment 526538 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6)

This makes install-module.pl properly fail to install Math::Random::Secure if it can't be installed (for example, if there's no compiler).
Comment 30 Max Kanat-Alexander 2011-04-16 17:07:40 PDT
Comment on attachment 526538 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6)

This applies properly on trunk, 4.0, and 3.6.
Comment 31 Frédéric Buclin 2011-04-17 02:26:51 PDT
Bugzilla 3.2 reached EOL already. Let's block 3.4.11 and 3.6.5 on it, though, as it's causing too much trouble.
Comment 32 Frédéric Buclin 2011-04-17 02:42:56 PDT
Comment on attachment 526535 [details] [diff] [review]
Remove MRS (3.4 and 3.2) v2

r=LpSolit for 3.4.10.
Comment 33 Frédéric Buclin 2011-04-17 02:45:58 PDT
Comment on attachment 526538 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6)

Why does install-module.pl ignore tests by default? Isn't that going to trigger unexpected behaviors in Bugzilla when a module has problems?
Comment 34 Teemu Mannermaa (:wicked) 2011-04-17 07:47:45 PDT
(In reply to comment #33)
> Why does install-module.pl ignore tests by default? Isn't that going to trigger

Hmm, is that really only ignoring tests?

It seems to use a "force install" which means "The force pragma may precede another command (currently: get, make, test, or install) to execute the command from scratch and attempt to continue past certain errors." Not sure what these "certain errors" include, though.

There's also a separate "notest install", which means "The notest pragma skips the test part in the build process." which sounds more like something we should be doing if we just want to ignore tests.

However, like LpSolit asks, why are we ignoring tests? If the module fails to test, it's broken and we shouldn't install it just to end up with odd errors that people blame on Bugzilla. This is also aggravated when the "--all" (which people simply try to use as it's one command instead of "millions" to install our requirements one by one) tries also to install all optional modules (increasing chances of something breaking).

This is evident especially by MRS, which seems to trigger a Mouse related warning (no idea if that is a real error or not) during checksetup.pl on Centos and Perl 5.8. :(
Comment 35 Max Kanat-Alexander 2011-04-17 12:49:25 PDT
Tests are for developers. They prove (to the developer) that the module works properly. Developers already get this information from CPAN via CPANTS. Running tests on every user install doesn't add any value. In this case, all it's doing is working around a bug in CPAN.pm. Also, install-module.pl takes about 10x longer if you run tests than if you don't.

Also, most platform-specific test failures are spurious, and in the past it's been far more valuable to skip tests than to run them. (Note that this is the first time it's ever been a problem, and it's not because the "tests fail", it's because dependencies didn't get installed properly and CPAN.pm is buggy about that.)
Comment 36 Max Kanat-Alexander 2011-04-17 12:53:59 PDT
Comment on attachment 526538 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6)

This needs one more fix in order to work on perl 5.8.8.
Comment 37 Max Kanat-Alexander 2011-04-17 13:15:18 PDT
Created attachment 526621 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6) v2

Okay, this one works.
Comment 38 Max Kanat-Alexander 2011-04-17 13:50:07 PDT
  I also just submitted a patch to fix the "t/proc.t" error on Perl 5.8 for Crypt::Random::Source:

  https://github.com/rafl/crypt-random-source/pull/2

  So basically, this bug was confusing because it was three or four different bugs in different areas.
Comment 39 Byron Jones ‹:glob› 2011-04-20 20:05:34 PDT
max, are you able to answer wicked's question, why are we using "force" instead of "notest" ?
Comment 40 Byron Jones ‹:glob› 2011-04-21 01:52:32 PDT
Comment on attachment 526621 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6) v2

the fix should be to change CPAN::Shell->force('install', $name) to CPAN::Shell->notest('install', $name)

then the script should fail correctly if dependencies failed, and still skip tests.
Comment 41 Max Kanat-Alexander 2011-04-21 11:56:47 PDT
Comment on attachment 526621 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6) v2

Hey Byron. No, that is not the fix. I'm sorry, I explained this to wicked on IRC and apparently did not explain it here. Probably you should read the callers of install_module and see when $test is set, and probably you should read the full code of Bugzilla::Install::CPAN and not just this patch.

To simplify that process, I'll just tell you: $test is set only when installing the CPAN module during check_cpan_requirements (or whatever it's called). Older versions of the CPAN module do not have the "notest" command, so it cannot be used while upgrading CPAN or installing install-module's pre-reqs. Otherwise "notest" is always used.

Running "notest" would *not* solve this bug--we're already using notest for every module in checksetup's requirements!
Comment 42 Max Kanat-Alexander 2011-04-21 11:57:46 PDT
(In reply to comment #40)
> then the script should fail correctly if dependencies failed, and still skip
> tests.

  Also, you'd think so, right? But no! Read the bug that I filed against CPAN.pm. (In fact, you may want to read all the RT links I've pasted in this bug.)

  In other news, this bug is highly critical, so only extremely critical issues should block this review.
Comment 43 Frédéric Buclin 2011-04-21 12:05:23 PDT
(In reply to comment #42)
>   In other news, this bug is highly critical, so only extremely critical issues
> should block this review.

This bug is not more critical than any other issue. If there is a reason to deny review and ask for more information or some use case to be fixed, then the reviewer should feel free to do so, instead of putting pressure on his shoulders to get r+ at all costs.
Comment 44 Max Kanat-Alexander 2011-04-21 13:00:55 PDT
(In reply to comment #43) 
> This bug is not more critical than any other issue.

  It is WAY more critical than any other issue. It is destroying our userbsae. People are not installing Bugzilla. It is majorly damaging a launch of one of our most major releases in history. Look at the active users graph:

  http://landfill.bugzilla.org/active/
Comment 45 Marc Schumann [:Wurblzap] 2011-04-22 03:22:30 PDT
Do we have some place off-bug where we can put an analysis of this userbase erosion? I find it very interesting that a bug in a newer version kills an installation. I'd've expected people to stay with their current version until the bug is resolved, and I'd like to see why this apparently doesn't happen.
Comment 46 Frédéric Buclin 2011-04-22 07:39:43 PDT
(In reply to comment #23)
> bug in Param::Util's Makefile.PL, which I have filed and patched here:
> 
>   https://rt.cpan.org/Ticket/Display.html?id=67522

Params::Util 1.04, which fixes this problem, has been released two days ago.
Comment 47 Teemu Mannermaa (:wicked) 2011-04-23 05:32:53 PDT
Comment on attachment 526621 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6) v2

FWIW, I tested this patch on my test install on landfill (I was updating one of my installs for another review). This patch indeed makes us update ExtUtils::MakeMaker to latest, which fixes most install errors for MRS modules. These errors happened because Makefile.PL failes to compile due to having only ExtUtils::MakeMaker v6.30, here's a log from that for reference:

--!--
  CPAN.pm: Going to build N/NU/NUFFIN/Crypt-Random-Source-0.07.tar.gz

ExtUtils::MakeMaker version 6.31 required--this is only version 6.30 at Makefile.PL line 7.
BEGIN failed--compilation aborted at Makefile.PL line 7.
Warning: No success on command[/usr/bin/perl Makefile.PL  ...]
  NUFFIN/Crypt-Random-Source-0.07.tar.gz
  /usr/bin/perl Makefile.PL  ... -- NOT OK
Skipping test because of notest pragma
Running make install
  Make had some problems, won't install
--!--

Since requirements come from this compilation, CPAN have no way knowing it should install ExtUtils::MakeMaker and we have a problem. :(

Patch also makes us not to install MRS when some of it requirements fail. This way user can fix the errors (or report the correct error) and retry MRS installation, which is nice.

I'm now facing the same error reported in comment 22.

BTW, now I know why I remember seeing tests during install-module.pl. The CPAN/YAML phase, which is run with force, does indeed run tests. It just ignores any errors (and there is at least one failing during CPAN install on landfill, which is from t/distribution.t failing badly).
Comment 48 Frédéric Buclin 2011-04-26 16:55:52 PDT
(In reply to comment #14)
> For 4.0.1, we should exclude Math::Random::Secure from install-module.pl --all
> as it triggers problems for a lot of people.

Could we add this to the patch, please?
Comment 49 Max Kanat-Alexander 2011-04-27 00:19:56 PDT
(In reply to comment #48)
> Could we add this to the patch, please?

  That's unnecessary with the patch.
Comment 50 Byron Jones ‹:glob› 2011-04-27 00:22:10 PDT
Comment on attachment 526621 [details] [diff] [review]
Make CPAN Fail Properly for M::R::S (trunk, 4.0, 3.6) v2

ok, r=glob as this is an improvement to the current state.
Comment 51 Max Kanat-Alexander 2011-04-27 15:06:40 PDT
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.4/                         
modified mod_perl.pl
modified Bugzilla/Util.pm
modified Bugzilla/Install/Requirements.pm
Committed revision 6799.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified Bugzilla/Install/CPAN.pm
Committed revision 7244.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified Bugzilla/Install/CPAN.pm
Committed revision 7581.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Install/CPAN.pm
Committed revision 7795.

Note You need to log in before you can comment on or make changes to this bug.