Bug 147776: Make checksetup.pl re-write localconfig with the current comments and remove old variables automatically

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> (module owner) a=mkanat

2 files changed, 77 insertions, 42 deletions

diff --git a/Bugzilla/Install/Filesystem.pm b/Bugzilla/Install/Filesystem.pm
index 8afaa3384..bf5ef8e61 100644
--- a/Bugzilla/Install/Filesystem.pm
+++ b/Bugzilla/Install/Filesystem.pm
@@ -64,6 +64,7 @@ sub FILESYSTEM {
     my $libdir        = bz_locations()->{'libpath'};
     my $extlib        = bz_locations()->{'ext_libpath'};
     my $skinsdir      = bz_locations()->{'skinsdir'};
+    my $localconfig   = bz_locations()->{'localconfig'};
 
     my $ws_group      = Bugzilla->localconfig->{'webservergroup'};
 
@@ -117,6 +118,8 @@ sub FILESYSTEM {
         'jobqueue.pl'     => { perms => $owner_executable },
         'install-module.pl' => { perms => $owner_executable },
 
+        "$localconfig.old" => { perms => $owner_readable },
+
         'docs/makedocs.pl'   => { perms => $owner_executable },
         'docs/style.css'       => { perms => $ws_readable },
         'docs/*/rel_notes.txt' => { perms => $ws_readable },
diff --git a/Bugzilla/Install/Localconfig.pm b/Bugzilla/Install/Localconfig.pm
index 7df9e0736..45005f032 100644
--- a/Bugzilla/Install/Localconfig.pm
+++ b/Bugzilla/Install/Localconfig.pm
@@ -187,13 +187,6 @@ EOT
     },
 );
 
-use constant OLD_LOCALCONFIG_VARS => qw(
-    mysqlpath
-    contenttypes
-    pages
-    severities platforms opsys priorities
-);
-
 sub read_localconfig {
     my ($include_deprecated) = @_;
     my $filename = bz_locations()->{'localconfig'};
@@ -221,9 +214,27 @@ Please fix the error in your 'localconfig' file. Alternately, rename your
 EOT
         }
 
-        my @vars = map($_->{name}, LOCALCONFIG_VARS);
-        push(@vars, OLD_LOCALCONFIG_VARS) if $include_deprecated;
-        foreach my $var (@vars) {
+        my @read_symbols;
+        if ($include_deprecated) {
+            # First we have to get the whole symbol table
+            my $safe_root = $s->root;
+            my %safe_package;
+            { no strict 'refs'; %safe_package = %{$safe_root . "::"}; }
+            # And now we read the contents of every var in the symbol table.
+            # However:
+            # * We only include symbols that start with an alphanumeric
+            #   character. This excludes symbols like "_<./localconfig"
+            #   that show up in some perls.
+            # * We ignore the INC symbol, which exists in every package.
+            # * Perl 5.10 imports a lot of random symbols that all
+            #   contain "::", and we want to ignore those.
+            @read_symbols = grep { /^[A-Za-z0-1]/ and !/^INC$/ and !/::/ }
+                                 (keys %safe_package);
+        }
+        else {
+            @read_symbols = map($_->{name}, LOCALCONFIG_VARS);
+        }
+        foreach my $var (@read_symbols) {
             my $glob = $s->varglob($var);
             # We can't get the type of a variable out of a Safe automatically.
             # We can only get the glob itself. So we figure out its type this
@@ -290,11 +301,6 @@ sub update_localconfig {
         }
     }
 
-    my @old_vars;
-    foreach my $name (OLD_LOCALCONFIG_VARS) {
-        push(@old_vars, $name) if defined $localconfig->{$name};
-    }
-
     if (!$localconfig->{'interdiffbin'} && $output) {
         print <<EOT
 
@@ -307,30 +313,41 @@ as well), you should install patchutils from:
 EOT
     }
 
+    my @old_vars;
+    foreach my $var (keys %$localconfig) {
+        push(@old_vars, $var) if !grep($_->{name} eq $var, LOCALCONFIG_VARS);
+    }
+
     my $filename = bz_locations->{'localconfig'};
 
+    # Move any custom or old variables into a separate file.
     if (scalar @old_vars) {
+        my $filename_old = "$filename.old";
+        open(my $old_file, ">>$filename_old") || die "$filename_old: $!";
+        local $Data::Dumper::Purity = 1;
+        foreach my $var (@old_vars) {
+            print $old_file Data::Dumper->Dump([$localconfig->{$var}], 
+                                               ["*$var"]) . "\n\n";
+        }
+        close $old_file;
         my $oldstuff = join(', ', @old_vars);
         print <<EOT
 
 The following variables are no longer used in $filename, and
-should be removed: $oldstuff
+have been moved to $filename_old: $oldstuff
 
 EOT
     }
 
-    if (scalar @new_vars) {
-        my $filename = bz_locations->{'localconfig'};
-        my $fh = new IO::File($filename, '>>') || die "$filename: $!";
-        $fh->seek(0, SEEK_END);
-        foreach my $var (LOCALCONFIG_VARS) {
-            if (grep($_ eq $var->{name}, @new_vars)) {
-                print $fh "\n", $var->{desc},
-                      Data::Dumper->Dump([$localconfig->{$var->{name}}], 
-                                         ["*$var->{name}"]);
-            }
-        }
+    # Re-write localconfig
+    open(my $fh, ">$filename") || die "$filename: $!";
+    foreach my $var (LOCALCONFIG_VARS) {
+        print $fh "\n", $var->{desc},
+                  Data::Dumper->Dump([$localconfig->{$var->{name}}],
+                                     ["*$var->{name}"]);
+   }
 
+    if (@new_vars) {
         my $newstuff = join(', ', @new_vars);
         print <<EOT;
 
@@ -405,31 +422,46 @@ variables defined in localconfig, it will print out a warning.
 
 =over
 
-=item C<read_localconfig($include_deprecated)>
+=item C<read_localconfig>
 
-Description: Reads the localconfig file and returns all valid
-             values in a hashref.
+=over
+
+=item B<Description>
+
+Reads the localconfig file and returns all valid values in a hashref.
+
+=item B<Params>
+
+=over
+
+=item C<$include_deprecated> 
 
-Params:      C<$include_deprecated> - C<true> if you want the returned
-                 hashref to also include variables listed in 
-                 C<OLD_LOCALCONFIG_VARS>, if they exist. Generally
-                 this is only for use by C<update_localconfig>.
+C<true> if you want the returned hashref to include *any* variable
+currently defined in localconfig, even if it doesn't exist in 
+C<LOCALCONFIG_VARS>. Generally this is is only for use 
+by L</update_localconfig>.
+
+=back
+
+=item B<Returns>
+
+A hashref of the localconfig variables. If an array is defined in
+localconfig, it will be an arrayref in the returned hash. If a
+hash is defined, it will be a hashref in the returned hash.
+Only includes variables specified in C<LOCALCONFIG_VARS>, unless
+C<$include_deprecated> is true.
+
+=back
 
-Returns:     A hashref of the localconfig variables. If an array
-             is defined, it will be an arrayref in the returned hash. If a
-             hash is defined, it will be a hashref in the returned hash.
-             Only includes variables specified in C<LOCALCONFIG_VARS>
-             (and C<OLD_LOCALCONFIG_VARS> if C<$include_deprecated> is
-             specified).
 
-=item C<update_localconfig({ output =E<gt> 1 })>
+=item C<update_localconfig>
 
 Description: Adds any new variables to localconfig that aren't
              currently defined there. Also optionally prints out
              a message about vars that *should* be there and aren't.
              Exits the program if it adds any new vars.
 
-Params:      C<output> - C<true> if the function should display informational
+Params:      C<$output> - C<true> if the function should display informational
                  output and warnings. It will always display errors or
                  any message which would cause program execution to halt.