143586 - required modules tests should be sorted

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Bradley Baetz (:bbaetz)]

Recently, the 'what modules do we have to install' test was moved into a hash.
As a result, the order is changed, and so the AppConfig test comes after the
Tempalte test. Since we adde the appconfig test there only because TT's CPAN
module stuff was wrong, this seems like a bad idea.

I'll attach a path which sorts this. Do we want this for 2.16, because of the
above issue?

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: v1

This also changes the indent before the version found string, to deal with
Text::Wrap's long version number. Without this, that modules's 'ok' text is off
to the side a bit.

Comment 2

[Dave Miller [:justdave]]

Comment on attachment 83154 [details] [diff] [review]
v1

put CGI::Carp back where it was or you're going to screw up any error output
from anything tested after CGI::Carp.  There's a reason it's by itself. :-)

The other option is to restore the die and warn handlers to the originals after
each version test...

Yes, I think this is necessary for 2.16.  AppConfig should definitely be in the
list before TT2.  Although if you follow that logic, DBI should probably be
tested before DBD::mysql, which this patch will reverse...

Comment 3

[Bradley Baetz (:bbaetz)]

Doh. Forgot about that. How about I just save and restore for each one?

The sort order is undefined, since it depends on perl's hash function, so I'm
not really reversing it.

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: v2

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 83176 [details] [diff] [review]
v2

yeah, but you're calling sort on it which puts them in alphabetical order.  DBD
comes before DBI in that order.  The code your patching is broken in that
regard, too, they used to be in a specific order.  Guess it doesn't really
matter, since I'm sure DBD's dependency on DBI works correctly in CPAN.

Comment 6

[Bradley Baetz (:bbaetz)]

Its currently DBI before DBD, though, at least in perl5.6.1 on linux

Comment 7

[Dave Miller [:justdave]]

even with your patch?

Comment 8

[Bradley Baetz (:bbaetz)]

Both with and without. With, D < I, so DBD is first. Without, thats just how the
has function is:

Checking perl modules ...
Checking for      Text::Wrap (v2001.0131) ok: found v2001.0131
Checking for     Date::Parse (any)     ok: found v2.22
Checking for      File::Spec (v0.82)   ok: found v0.82
Checking for        Template (v2.07)   ok: found v2.07
Checking for      DBD::mysql (v1.2209) ok: found v2.0419
Checking for       AppConfig (v1.52)   ok: found v1.52
Checking for    Data::Dumper (any)     ok: found v2.102
Checking for             DBI (v1.13)   ok: found v1.21
Checking for       CGI::Carp (any)     ok: found v1.20

is the current order for me. Relying on that is useless, though, since the order
isn't defined.

Comment 9

[Dave Miller [:justdave]]

right, so if you sort them alphabetically it's backwards, since DBI needs to be
before DBD::mysql...

Comment 10

[Bradley Baetz (:bbaetz)]

But I'm not regressing anything, since it was broken before we sort them ;) 

Comment 11

[Dave Miller [:justdave]]

But it's still a regression, just not caused by your patch.  The module version
tests were only changed to a hash less than a week ago.  They *were* in the
correct order before it was converted to a hash.

Comment 12

[Bradley Baetz (:bbaetz)]

so whats the solution for 2.16, then?

Comment 13

[David Lawrence [:dkl]]

You could keep the hash the way it is and have an array that has the module 
names in the order you want and just iterate through that. Only problem is you 
have two places to maintain then when adding new modules in the future. We 
could also do something like this: 
 
my $modules = [ 
	{ 
		name => 'Text::Wrap',  
		version => '2001.0131' 
	}, 
	{ 
		name => 'Date::Parse (any)', 
		version => '2.22' 
	}, 
		name => 'File::Spec', 
		version => '0.82' 
	}, 
	{	 
		name => 'Template', 
		version => '2.07' 
	}, 
	{ 
		name => 'DBD::mysql', 
		version => '2.0419' 
	}, 
	{ 
		name => 'AppConfig', 
		version => '1.52' 
	}, 
	{ 
		name => 'Data::Dumper', 
		version => '2.102' 
	}, 
	{ 
		name => 'DBI', 
		version => '1.21' 
	}, 
	{ 
		name => 'CGI::Carp', 
		version => '1.20' 
	} 
]; 
 
Of course they would need to be in the original order before the hash was 
added. I admit that I didnt consider order when making the change earlier but 
it shouldnt be too late to add this in. I will work up a patch and attach for 
consideration. 

Comment 14

[David Lawrence [:dkl]]

Attachment: Modules information in anonymous array for proper sorting (v1)

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 84059 [details] [diff] [review]
Modules information in anonymous array for proper sorting (v1)

WFM. r=bbaetz

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 84059 [details] [diff] [review]
Modules information in anonymous array for proper sorting (v1)

r= justdave

works for me.

Comment 17

[David Lawrence [:dkl]]

Attachment: Modules information in anonymous array for proper sorting (v2)

Fixed single letter typo error in v1 of this patch which was causing the HASH
address information to display instead of the minimum required version when a
required module is missing. Should I transfer the same reviews from the
previous patch to this one and go ahead and check it in?

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 84262 [details] [diff] [review]
Modules information in anonymous array for proper sorting (v2)

yes.

Comment 19

[Bradley Baetz (:bbaetz)]

-> patch author

Comment 20

[David Lawrence [:dkl]]

BUGZILLA-2_16-BRANCH
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.149.2.3; previous revision: 1.149.2.2
done

HEAD
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.152; previous revision: 1.151
done