208604 - Make data/template dir locations configurable

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Bradley Baetz (:bbaetz)]

mod_perl needs to use absolute paths for various reasons, mainly because the
startup.pl stuff doesn't run from the cgi dir, and under mp2 thers no chdir at all.

I've got a patch which does this. Note that the paths defined in
Bugzilla::Config, not localconfig. Under mod_perl we can grab this from
.htaccess configuration directives. with a normal cgi, we could grab these from
localconfig, but there are ordering issues esp with checksetup, so I'd rather
leave them in the .pm for the moment. $localconfig will obviously never be
configurable via a conf file...

webdot is special because it needs to be in the webtree. The script should
generate the graphs directly to stdout like report.cgi does, and then we
wouldn't have to worry about this unless webdot was being used.

The graph/ dir has the same issue, but I didn't move it because gerv's graphing
rewrite gets rid of it, IIRC.

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: v1

OK, here we go. The other change is that the templates are now compiled into
data/template, rather than data/. This means that we'll get
data/template/template/en/.... I needed this so that we could get
data/template/usr/share/bugzilla/template/en/... and not have to worry about
namespace collisions. (TT handles the absolute vs relative template paths
without problems)

Other changes:
 - checksetup.pl now does its module testing in a BEGIN block. We can now feel
free to |use| other modules below without any problems. I needed to add a $^C
check so that tests passed, else perl -c checksetup.pl produced output and
failed tests.
 - Bugzilla/DB renames some vars to now be $:: - under mod_perl localconfig
vars are $Bugzilla::Config::foo and don't have the alias into $::main which we
have as a cgi. This also got in the way of something I was going to do here,
but decided to leave for another patch.
  - Removed reference to $contenttypes as a localconfig variable in
Bugzilla::Config, since it got moved into Bugzilla::Constants as part of
another patch a while back.

The contrib stuff compiles, and thats all the testing I did. I used BEGIN so
that I could use Bugzilla::Config rather than having to require + import + use
vars the stuff I wanted.

Comment 2

[Rob Browning]

In a somewhat related effort, I'm working on a trial Debian package for 2.17.4.
Debian has some requirements for the placement of files (inherited from
FHS and Debian Policy requirements), and so I thought I'd mention some of those
requirements here in case you might also want to consider them upstream.

I've been speaking with the current maintainer of the Debian package and he
mentioned several things:

  - Debian doesn't use the htaccess support (it's disabled) in checksetup.pl
    because Debian places the files that shouldn't be accessable outside the
    web tree.

  - partially as a result of the above, Debian has to deal with multiple
    file locations
      * http://hostname/bugzilla/{css/*,*.{html,js,gif,jpeg,dtd,txt}}
      * http://hostname/cgi-bin/bugzilla/*.cgi

  - Debian places processmail (and other bugzilla specific binaries) in
    /usr/share/bugzilla/lib, so all the invocations have to be edited
    accordingly.

As a result of Debian's requirements, the Debian diff has a whole bunch of
small changes, many just to edit relative paths, etc.  In case it's helpful,
here are some examples of common diffs from the current 2.16.3 Debian package:

  -use lib ".";
  +use lib '/usr/share/bugzilla/lib';

  -use lib qw(.);
  +use lib '/usr/share/bugzilla/lib/';

  -  open(PMAIL, "-|") or exec('./processmail', $id);
  +  open(PMAIL, "-|") or exec('/usr/share/bugzilla/lib/processmail', $id);

  -  if (open(COMMENTS, "<data/comments")) {
  +  if (open(COMMENTS, "</var/lib/bugzilla/data/comments")) {

  -  <link href="css/buglist.css" rel="stylesheet" type="text/css">
  +  <link href="/bugzilla/css/buglist.css" rel="stylesheet" type="text/css" >

Hope this helps, and thanks.

Comment 3

[Bradley Baetz (:bbaetz)]

This patch will help (and processmail doesn't exist any more), but its not quite
at that level yet.

Comment 4

[Rob Browning]

Is this patch likely to work very well against 2.17.4, or has CVS changed enough
to make that unlikely?  It looked like there were some rejects, but I don't
yet know how serious they are.

Comment 5

[Rob Browning]

I'm not sure you want to support this much flexibility upstream, but
to allow for the current Debian arrangement, we'd probably need
something like this (using an autoconf-style naming convention):

  our $sysconfdir = "/etc";
  our $localconfig = "${sysconfdir}/bugzilla/localconfig";

  # ideally, localconfig would be allowed to override any of these:
  our $libdir = '/usr/lib/bugzilla';
  our $datadir = "/usr/share/bugzilla";
  our $localstatedir = "/var/lib/bugzilla";
  our $templatedir = "${datadir}/template";
  our $custom_templatedir = "${localstatedir}/template";
  our $cachedir = "/var/cache/bugzilla"
  our $webdotdir = "${cachedir}/webdot";

Comment 6

[Bradley Baetz (:bbaetz)]

At some point we may (what do you put in /etc?), although we can't put $libdir
in a module, since we need that first. We're only going to support moving that
under mod_perl, where we can get it OOB. Maybe via SetEnv too, I guess.

Comment 7

[Rob Browning]

> At some point we may (what do you put in /etc?)

/etc/bugzilla/localconfig for one thing.  Debian requires that any file that
can/should be edited by the local admin be placed in /etc.

> although we can't put $libdir in a module, since we need that first.

I'm not sure I understand the situation in this respect well enough yet to
comment.

FWIW here's a synopsis of where the current 2.16.3 Debian package places
things:

/etc/bugzilla
...
/etc/bugzilla/localconfig
...
/etc/cron.daily/bugzilla
...
/var/lib
/var/lib/bugzilla
/var/lib/bugzilla/web
/var/lib/bugzilla/web/index.html
/var/lib/bugzilla/template
/var/lib/bugzilla/template/en
/var/lib/bugzilla/template/en/custom
/var/lib/bugzilla/shadow
/var/lib/bugzilla/data
/var/cache
/var/cache/bugzilla
/var/cache/bugzilla/template
/usr
/usr/share
/usr/share/bugzilla
/usr/share/bugzilla/web
/usr/share/bugzilla/web/1x1.gif
/usr/share/bugzilla/web/bugwritinghelp.html
/usr/share/bugzilla/web/bug_status.html
...
/usr/share/bugzilla/web/robots.txt
/usr/share/bugzilla/web/quicksearch.js
...
/usr/share/bugzilla/web/ant.jpg
/usr/share/bugzilla/web/bugzilla.dtd
/usr/share/bugzilla/web/css
/usr/share/bugzilla/web/css/buglist.css
...
/usr/share/bugzilla/lib
/usr/share/bugzilla/lib/CGI.pl
/usr/share/bugzilla/lib/Bug.pm
...
/usr/share/bugzilla/lib/bug_form.pl
...
/usr/share/bugzilla/template/en/default/account/email/change-new.txt.tmpl
/usr/share/bugzilla/template/en/default/account/email/change-old.txt.tmpl
/usr/share/bugzilla/template/en/default/account/email/confirm.html.tmpl
/usr/share/bugzilla/template/en/default/account/password
/usr/share/bugzilla/template/en/default/account/password/forgotten-password.txt.tmpl
...
/usr/share/bugzilla/template/en/default/account/prefs/account.html.tmpl
...
/usr/share/bugzilla/template/en/default/list/list-simple.html.tmpl
...
/usr/share/bugzilla/web/index.html
...
/usr/share/doc/bugzilla/README
/usr/share/doc/bugzilla/copyright
/usr/share/doc/bugzilla/changelog.gz
...
/usr/lib/cgi-bin/bugzilla/attachment.cgi
/usr/lib/cgi-bin/bugzilla/buglist.cgi
...
/var/www/bugzilla_graphs

Comment 8

[Bradley Baetz (:bbaetz)]

/etc/bugzilla/localconfig for one thing.  Debian requires that any file that
can/should be edited by the local admin be placed in /etc.


That doesn't let you have more than one install off the same source, though.

Comment 9

[Rob Browning]

True.  How does the current approach support multiple installs?

I'm not sure what kind of flexibility there is wrt to the bugzilla cgis, but if
it were possible, perhaps something like /etc/bugzilla/localhost/FOO or
/etc/bugzilla/localhost.FOO might work.

Note that wrt /etc and Debian, there can always be symlinks from other places to
the /etc files, it's just that the conf files themselves have to be in /etc.
i.e. /usr/lib/blarg -> /etc/foo/bar is OK.

Comment 10

[Dave Miller [:justdave]]

I wasn't intending for this to become a major discussion on the
multiple-directoy install system....  I just thought what was happening on this
bug would help with it.  The real discussion on the multiple-directory install
stuff is happening over on bug 44659.  Let's revive that and get this discussion
going over there.

Comment 11

[Bradley Baetz (:bbaetz)]

Attachment: v2

This just fixes cvs conflicts.

Comment 12

[Dave Miller [:justdave]]

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

> Index: checksetup.pl
> ===================================================================
> @@ -851,13 +857,13 @@
>  
> -unless (-d 'data/webdot') {
> +unless (-d "$datadir/webdot") {
>      # perms/ownership are fixed up later
> -    mkdir 'data/webdot', 0700;
> +    mkdir "$datadir/webdot", 0700;
>  }

$webdotdir?

> @@ -1361,7 +1371,7 @@
>    Bugzilla Guide in the doc directory and all parts of the MySQL
>    documentation.
>  * There is an subtle problem with Perl, DBI, DBD::mysql and MySQL. Make
> -  sure all settings in 'localconfig' are correct. If all else fails, set
> +  sure all settings in '$localconfig' are correct. If all else fails, set
>    '\$db_check' to zero.\n
>  EOF
>      }

nit: this is going to get output on the console, and there's a good chance
$localconfig is going to be pretty long.  Might want to consider moving
everything after it to the following line in the output.

> @@ -3120,20 +3130,20 @@
> +    print "The $datadir/comments file (used to store quips) has been copied into\n" .
> +      "the database, and the $datadir/comments file moved to $datadir/comments.bak - \n" .
>        "you can delete this fileonce you're satisfied the migration worked\n" .
>        "correctly.\n\n";

same here.  Actually, we already require Text::Wrap...	maybe we oughtta use it
here. :)

> Index: showdependencygraph.cgi
> ===================================================================
> @@ -189,7 +190,7 @@
>      my $dotfh;
>      my ($pngfh, $pngfilename) = File::Temp::tempfile("XXXXXXXXXX",
>                                                       SUFFIX => '.png',
> -                                                     DIR => 'data/webdot');
> +                                                     DIR => $webdotdir);
>      open (DOT, '-|') or exec ($webdotbase, "-Tpng", $filename);
>      print $pngfh $_ while <DOT>;
>      close DOT;

Hmmm, we're going to have a little problem here....   $pngfilename gets passed
to the template...  with the intention that it's a relative URL from the cgi
location.  Since it's more than likely going to be a full pathname now, we're
going to need knowledge of the webserver's document root or something to be
able to correctly produce the URL...

Comment 13

[Bradley Baetz (:bbaetz)]

> <checksetup.pl text>

Lets leave this now, and we can rewrite checksetup later. We can't use
Text::Wrap, since we need to detect it here.

> <webdot stuff>

See paragraph 3 in comment 0. Also see my comment in Bugzilla::Config:

+# $webdotdir must be in the webtree somewhere. Even if you use a local dot,
+# we output images to there. Also, if $webdot dir is not relative to the
+# bugzilla root directory, you'll need to change showdependancygraph.cgi to
+# set image_url to the correct location.
+# The script should really generate these graphs directly...

I only handled the webdot stuff here because its going to be one of the first
mod_perl'd things. I'll fix it to make it really work when that happens.

Comment 14

[Dave Miller [:justdave]]

OK, so noted.  So we just need the one $webdotdir thing fixed then.

Comment 15

[Bradley Baetz (:bbaetz)]

Attachment: v3

Fixed that, and added a more explicit comment in Bugzilla::Config

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 131423 [details] [diff] [review]
v3

The following jump out at me as instances that were probably missed...

>checksetup.pl:3744:        $product_file = "data/mining/$product_file";

>checksetup.pl:1002:            print "    template and placed in the 'template/en/custom' directory.\n\n";
>checksetup.pl:1076:               # => $datadir/template/`pwd`/template/{en, ...}/{custom,default}
>defparams.pl:160:       if(   ! -d 'template/'.trim($language).'/custom' 
>defparams.pl:161:          && ! -d 'template/'.trim($language).'/default') {
>page.cgi:26:# put them in the "pages" subdirectory of template/en/default, call them
>t/008filter.t:48:    $path =~ m|template/([^/]+)/([^/]+)|;

and this one (already mentioned above) :
>checksetup.pl:1076:               # => $datadir/template/`pwd`/template/{en, ...}/{custom,default}

what's with `pwd` ?  Isn't that likely to be different than the path
$templatedir is going to wind up pointing at when it searched for compiled
copies at runtime?

Otherwise, the rest of this works great.  We need a URI for the webdot
directory at some point to be published from Bugzilla::Config, because if
$webdotdir moves, showdependencygraph.cgi is going to break, because the
browser won't be able to find the image file.  But that can probably be another
patch.

Comment 17

[Dave Miller [:justdave]]

Attachment: v3 updated

for the record, I'm attaching the patch that I actually reviewed.  v3 had
bitrotted, so I updated it to the tip.	There's no changes to v3 other than
fixing the bitrot, so the previous review comments still apply.  (just thought
I'd save you some time if you haven't kept it up on your local copy)

Comment 18

[Bradley Baetz (:bbaetz)]

Attachment: v4

As I've mentioend previously showdependancygraph should display the graph
directly, just like the new graphing code does.

The `pwd` is in comments, and relates to the chdir bit of File::Find, I think
(its not too clear...)

Tests are only for use prior to installation - otherwise we'd have to do the
tab stuff within $libdir, for example, and that could in theory be the global
site_perl dir.

Comment 19

[Dave Miller [:justdave]]

Comment on attachment 135559 [details] [diff] [review]
v4

looks good.  r=justdave

Comment 20

[Bradley Baetz (:bbaetz)]

Fixed! (with two small CVS conflict fixes)