Closed Bug 143586 Opened 22 years ago Closed 22 years ago

required modules tests should be sorted

Categories

(Bugzilla :: Installation & Upgrading, defect)

2.15
x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: bbaetz, Assigned: dkl)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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?
Attached patch v1 (obsolete) — Splinter Review
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 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...
Attachment #83154 - Flags: review-
Keywords: regression
Target Milestone: --- → Bugzilla 2.16
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.
Keywords: patch, review
Attached patch v2 (obsolete) — Splinter Review
Attachment #83154 - Attachment is obsolete: true
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.
Its currently DBI before DBD, though, at least in perl5.6.1 on linux
even with your patch?
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.
right, so if you sort them alphabetically it's backwards, since DBI needs to be
before DBD::mysql...
But I'm not regressing anything, since it was broken before we sort them ;) 
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.
so whats the solution for 2.16, then?
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 on attachment 84059 [details] [diff] [review]
Modules information in anonymous array for proper sorting (v1)

WFM. r=bbaetz
Attachment #84059 - Flags: review+
Comment on attachment 84059 [details] [diff] [review]
Modules information in anonymous array for proper sorting (v1)

r= justdave

works for me.
Attachment #84059 - Flags: review+
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?
Attachment #83176 - Attachment is obsolete: true
Attachment #84059 - Attachment is obsolete: true
Comment on attachment 84262 [details] [diff] [review]
Modules information in anonymous array for proper sorting (v2)

yes.
Attachment #84262 - Flags: review+
-> patch author
Assignee: bbaetz → 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
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: