Closed
Bug 143586
Opened 22 years ago
Closed 22 years ago
required modules tests should be sorted
Categories
(Bugzilla :: Installation & Upgrading, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: bbaetz, Assigned: dkl)
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
1.76 KB,
patch
|
justdave
:
review+
justdave
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•22 years ago
|
||
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•22 years ago
|
||
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-
Updated•22 years ago
|
Keywords: regression
Target Milestone: --- → Bugzilla 2.16
Reporter | ||
Comment 3•22 years ago
|
||
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.
Reporter | ||
Updated•22 years ago
|
Reporter | ||
Comment 4•22 years ago
|
||
Attachment #83154 -
Attachment is obsolete: true
Comment 5•22 years ago
|
||
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.
Reporter | ||
Comment 6•22 years ago
|
||
Its currently DBI before DBD, though, at least in perl5.6.1 on linux
Comment 7•22 years ago
|
||
even with your patch?
Reporter | ||
Comment 8•22 years ago
|
||
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•22 years ago
|
||
right, so if you sort them alphabetically it's backwards, since DBI needs to be before DBD::mysql...
Reporter | ||
Comment 10•22 years ago
|
||
But I'm not regressing anything, since it was broken before we sort them ;)
Comment 11•22 years ago
|
||
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.
Reporter | ||
Comment 12•22 years ago
|
||
so whats the solution for 2.16, then?
Assignee | ||
Comment 13•22 years ago
|
||
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.
Assignee | ||
Comment 14•22 years ago
|
||
Reporter | ||
Comment 15•22 years ago
|
||
Comment on attachment 84059 [details] [diff] [review] Modules information in anonymous array for proper sorting (v1) WFM. r=bbaetz
Attachment #84059 -
Flags: review+
Comment 16•22 years ago
|
||
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+
Assignee | ||
Comment 17•22 years ago
|
||
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 18•22 years ago
|
||
Comment on attachment 84262 [details] [diff] [review] Modules information in anonymous array for proper sorting (v2) yes.
Attachment #84262 -
Flags: review+
Assignee | ||
Comment 20•22 years ago
|
||
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
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•