98658 - Add version headers to all templates in preparation for later admin notification of template changes.

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P1)

Description

[Matthew Tuck [:CodeMachine]]

If an administrator has a customised template in the custom directory, and the
template in the default directory is modified by the Bugzilla team, we should
notify the administrator.

The changes might be small, or they might cause incompatibility.  It might be
required, possibly required or beneficial for the admin to update their custom
template.

One implementation is for checksetup.pl to maintain a list of CRCs for all the
templates and regenerate and check them each run.  This however would make it
impossible to make changes that are "below-the-radar" such as license/comment
changes, unless we added a preprocessing step to strip these out.

An alternative option is to store version numbers, but any system where these
needed to be manually updated would be asking for trouble.

Comment 1

[Dave Miller [:justdave]]

-> Installation

Comment 2

[Dave Miller [:justdave]]

As long as there's a way to mask it so the template system doesn't trip out on
it, you can place an $Id$ line anywhere in the text of a file, and the cvs
system will automatically replace it with the filename, cvs revision number, who
last changed it, and the date&time of the checkin whenever anyone checks the
file out.

Comment 3

[Matthew Tuck [:CodeMachine]]

There would need a way to avoid updating $Id in specific circumstances (but not
by default), or the id would have no use.

Comment 4

[Jacob Steenhagen]

Matty, $Id$ can't be "not updated" on checkin as it tells who made a checkin,
what the current revision is (which always changes) and the date/time of the
checkin.  If that makes it unusable in this context, then so does any other use
of the revision number.

That being said, we should be able to add the following line to all the templates:
<!-- $Id$ -->

That, of course, assumes that Template Toolkit recognizes <!-- --> as a comment.

Comment 5

[Matthew Tuck [:CodeMachine]]

My logic was CRC would be easier, but that might not be the case if this stuff
is automatic.

CRC would also work regardless of CVS, but I'm not sure how the default
templates would be edited outside that environment.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

The Template Toolkit does not recognize <!-- --> as a comment, although it could
probably be configured to do so.  It does, however pass <!-- --> through to the
template output, so you can use that tag when you want to insert an HTML comment
into the generated output.  Otherwise TT recognizes [%# %] as a TT comment and
will not do anything with its contents.

Comment 7

[Dave Miller [:justdave]]

This is going to be a blocker for the 2.16 release

Comment 8

[Gervase Markham [:gerv]]

It seems to me that the majority of changes made to the default templates will
not be incompatible - HTML fixes, cosmetic changes, whatever. I can't see any
way that we can get some automated checker to detect which changes are relevant,
unless it's extremely intelligent.

It seems to me that the best fix for this is for each template to say, at the
top of it, what its interface is - which variables it expects to be passed in.
If someone changes that interface, they will also need to update the comment.
Even aside from this requirement, such documentation is desireable.

We can then enclose the comment in some detectable tags, and CRC the space
between them. Or, we can just look at the differences manually just before a
release.

Gerv

Comment 9

[Zach Lipton [:zach]]

Why not use a UUID-like system somewhat like XPCOM. Each template 
gets a UUID and that UUID changes when the template API changes. A 
custom template would have the same UUID as the default template it is 
based on, just with a different domain. checksetup.pl could then see if any 
custom templates are installed, and if so examine the UUID's to 
determine if things have been changed and print messages to the user 
telling them what to do.

UUID's would be in a comment and look like (for a default template):
<!-- [e-mail] -->

If redhat (for example) wanted to customize the template, it would be the 
same UUID, just with @redhat.com or @bugzilla.redhat.com at the end. 

We could also use version numbers, but people may make the mistake of 
bumping up the version number when they don't change the API. 
Generating the uuid is easy: mozbot uuid will get you one.

This is currently targeted at 2.16 as a blocker. We need a spec _now_ for 
how this will work if we want it in by the end of the month.

Zach

Comment 10

[Myk Melez [:myk] [@mykmelez]]

>We could also use version numbers, but people may make the mistake of 
>bumping up the version number when they don't change the API. 
>Generating the uuid is easy: mozbot uuid will get you one.

Making sure version increments are done correctly should become part of the
review process.

Comment 11

[Christian Reis]

I think a simple version number would probably work for bugzilla. Making things
easier for people to manage and read is good; otherwise we end up with things
like "what API was your template designed against, @#$&#@*($&!@#!@ or
@##&@#$(*#&?" instead of "what API version is your template, v1 or v3?" IMHO
simplicity would win out here.

So my proposal is: make a standard heading for templates that includes what API
version it expects, and make the .cgi file include a standard heading stating
the API version it exports and what variables it assumes.

Comment 12

[Matthew Tuck [:CodeMachine]]

This should be about bumping the version number up on any change the
administrator might be interested in, not just API changes.

Comment 13

[Zach Lipton [:zach]]

let's do the best of both worlds: a version number and a domain. The 
domain won't be used now, but could be used later on. 

Every template would begin with:
<html>
<head>
<!-- [e-mail] -->
etc...

If (and when) we do a skined thing for bugzilla so users can select skins, 
we could have: 


<!-- [e-mail] -->

All templates would start at 1.0 now and numbers would be bumped up 
as part of review. Someone needs to document this, barnboy? 

What do you think? Simplicity of version numbers and the expandibility of 
domains for later.

Comment 14

[Zach Lipton [:zach]]

Attachment: Non-working first crack at this.

This does not yet work, but if the logic is fixed in checksetup.pl it should be
ok. I'll get to this in a day or two unless someone beats me to it.

Comment 15

[Zach Lipton [:zach]]

Attachment: Patch v2, this should do the trick

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 61035 [details] [diff] [review]
Patch v2, this should do the trick

it's a minor nit-pick, but how about saying "out of sync" instead of "out of
date" since your giving the error if the version doesn't match, whether or not
it's greater or less...

Comment 17

[Zach Lipton [:zach]]

Attachment: v3

Comment 18

[Zach Lipton [:zach]]

Attachment: v3.1, without macbinary header

Comment 19

[Dave Miller [:justdave]]

Comment on attachment 61204 [details] [diff] [review]
v3.1, without macbinary header

Uncaught exception from user code:
	Uncaught exception from user code:
	Uncaught exception from user code:
	Uncaught exception from user code:
	Can't locate Support/Files.pm in @INC (@INC contains:
/sw/lib/perl5/darwin /sw/lib/perl5 /System/Library/Perl/darwin
/System/Library/Perl /Library/Perl/darwin /Library/Perl /Library/Perl
/Network/Library/Perl/darwin /Network/Library/Perl /Network/Library/Perl .) at
t/Support/Templates.pm line 25.
	Support::Templates::BEGIN() called at Support/Files.pm line 25
	require 0 called at Support/Files.pm line 25
	require t/Support/Templates.pm called at ./checksetup.pl line 245
	main::BEGIN() called at Support/Files.pm line 25
	require 0 called at Support/Files.pm line 25
BEGIN failed--compilation aborted at t/Support/Templates.pm line 25.
	require t/Support/Templates.pm called at ./checksetup.pl line 245
	main::BEGIN() called at t/Support/Templates.pm line 25
	require 0 called at t/Support/Templates.pm line 25
Compilation failed in require at t/Support/Templates.pm line 245.
	main::BEGIN() called at t/Support/Templates.pm line 245
	require 0 called at t/Support/Templates.pm line 245
BEGIN failed--compilation aborted at ./checksetup.pl line 245.

Comment 20

[David D. Kilzer (ddk)]

To fix attachment 61204 [details] [diff] [review] change this line:

use t::Support::Templates;

to this:

require "./t/Support/Templates.pm";

or these:

use lib "t";
use Support::Templates;

Comment 21

[Gervase Markham [:gerv]]

The error message should present the dodgy files as a list at the end of the
process, not print four lines for every template.

Also, presumably 
+my @templates = @Support::Templates::testitems;
doesn't, in fact, get a list of the custom templates a user has. Does it?

Should we be doing sanity checks on the domain name part? If not, why's it
there?

Also, the standard templates should have a domain. Thinking about it, why aren't
we using contract ID syntax? This seems designed for this sort of thing.

Gerv

Comment 22

[Gervase Markham [:gerv]]

zach: ping? :-)

Gerv

Comment 23

[Zach Lipton [:zach]]

gerv, I'm a little confused about your comments, when you have a sec can 
you get on irc?

Zach

Comment 24

[Dave Miller [:justdave]]

zach, gerv, ping...

have you two gotten together and figured this out yet?

Comment 25

[Zach Lipton [:zach]]

Attachment: v4

I didn't get a chance to talk to gerv, but I think a bunch of us figured it out
on irc.

Comment 26

[Dave Miller [:justdave]]

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

1) cp template/default/global/header template/custom/global/header
2) ./checksetup.pl
(no mismatch found, version should match anyway)
3) change mozilla.org to syndicomm.com in the version header
4) ./checksetup.pl
no mismatch found, so far so good
5) change 1.0 to 1.1 in the version header
6) ./checksetup.pl
no mismatch found.  maybe ok, it's newer than the default one
7) change 1.1 to 0.9 in the version header
8) ./checksetup.pl
no mismatch found.  this is a problem

Comment 27

[Gervase Markham [:gerv]]

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

>+print "Checking custom template versions\n";
>+use lib "t";

Is this the testing library? Should we be requiring it for normal Bugzilla
operation?

>+use Support::Templates;
>+my @templates = @Support::Templates::testitems;

And what about this? Shouldn't we be using a recursive glob?

>+   if (-e "template/custom/".$file) { # if there is a custom version...

Nit: "template/custom/" . $file

>+      $currenttemplate =~ /\[\%\#<!--\s(.*?)\@.*?\s-->/; # get the version number in $1

Why do you require HTML comments around template version numbers? E.g.
>+[%#<!-- [e-mail] -->%]

What's wrong with:

>+[%# Version: [e-mail] %]

(We should be using bugzilla.org, probably.)

>+      if ($customversion ne $defaultversion) {

We should do something more sophisticated than this, IMO. It would be nice to
have "harmless" 
warnings which say "We've put extra cool stuff in this template - fix it if you
like, but it's not
vital", as opposed to "your template will break if you don't fix this."

This means the numbering scheme probably requires a bit more thought. But this
must be a problem 
that's been solved before.

In your next patch, only add headers to a couple of template files (for
testing) - you're chasing a moving target, and it's not worth it.

Gerv

Comment 28

[Zach Lipton [:zach]]

I don't mind chasing a moving target, most of the 2.16 templates are done 
and I can always add more later. Quick responces to your comments:

1. Testing library/recursive glob:
The testing library currently does some things that we don't want to do (it 
won't find included templates like header and footer plus we probably 
shouldn't use the tests anyway. I'll have this fixed in the next patch.

2. Why html comments?:
I guess there isn't much reason for this. I'll cut it out of the next version. 

3. More advanced system:
How about this:
An increase in the major version number (before the decimal point) 
indicates a major change that will break all old templates. An increase in 
the minor version number (after the decimal point) indicates a minor 
change (though typo fixing, etc shouldn't get an increase at all IMHO) that 
old templates will still work with. 

4. bugzilla.org for domains:
Good idea, that way mozilla.org can use custom templates. 

Let me know what you think about this. I'll wait before going forward with a 
new patch.

Zach

Comment 29

[Gervase Markham [:gerv]]

Your comment on 3) sounds fine. Make sure the comparison allows 10.1 to be > 9.3
and 10.12 to be greater than 10.9 . Should we pad the decimal part with a zero
(1.00, 2.06)?

Gerv

Comment 30

[Bradley Baetz (:bbaetz)]

What about between releases? Can we make a policy that templates version numbers
are only bumped once per stable release? Else we're going to end up at version
100 real easily. Not to mention the cvs conflicts that it would likely cause.

Comment 31

[Matthew Tuck [:CodeMachine]]

If we bump version numbers once, we are going to potentially break
administrators running off CVS.  I don't see any problem with bumping by 100
each release.

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Zach: ping, what's your status on this?

Comment 33

[Gervase Markham [:gerv]]

Zach?

Gerv

Comment 34

[Matthew Tuck [:CodeMachine]]

Bueler?

Comment 35

[Zach Lipton [:zach]]

I didn't die, working on this now

Comment 36

[Myk Melez [:myk] [@mykmelez]]

We don't need this for 2.16, since 2.16 is the first official release containing
templates and thus does not contain any templates that have been modified from
previously released versions.  Reducing severity accordingly.

Comment 37

[Andreas Franke (gone)]

Myk, I think there is one part of this that is indeed a blocker: If we don't put 
any kind of version information in the templates that are shipped with 2.16, how 
 can it be possible to solve this for 2.18? Well... maybe you are proposing to 
treat all versionless templates as always outdated? But then you can't treat 
missing version information as an error condition any more... Can you clarify?

Comment 38

[Matthew Tuck [:CodeMachine]]

Yes this was marked as a blocker for 2.16 because you need the information for
2.16 to know its outdated for 2.18.

Comment 39

[Zach Lipton [:zach]]

Why don't we do this in two stages. I'll fix the patch up to include only the 
version information (starting at 1.0) and get that in. The checksetup.pl 
logic part can get in if it's ready by RC, or we can hold off on it until after 
release.

Comment 40

[Myk Melez [:myk] [@mykmelez]]

How far away are you from getting the version info into the templates?

Comment 41

[Zach Lipton [:zach]]

Attachment: Add the version string to each template only and test to see that is is there

This patch will add the version string to every template and test each template
to ensure that it is present as part of the tests. I did the version string as
an html comment so that the user will be able to see what version is being sent
for debugging if need be.

Comment 42

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 77284 [details] [diff] [review]
Add the version string to each template only and test to see that is is there

The patch failed with a "malformed patch" error on line 93, which turned out to
be a typo on line 89 ("+1,3" instead of "+1,4").  With that typo fixed the
patch applies fine, the tests complete successfully, and templates work. r=myk

Comment 43

[Zach Lipton [:zach]]

The malformed patch error thing is my fault, I manually changed 
something in the patch and forgot I couldn't do that. Someone please 2x 
review this.

Comment 44

[Gervase Markham [:gerv]]

Comment on attachment 77284 [details] [diff] [review]
Add the version string to each template only and test to see that is is there

> if ($firstline =~ /<!-- \d\.\d+\@\w+\.\w+ -->/) {

This fails to match e.g.
[e-mail]. 
Or [e-mail], for that matter.
Does \w correspond exactly to the characters valid in a hostname?

Also, I'm sure we were going to use bugzilla.org rather than mozilla.org for
our templates...

Gerv

Comment 45

[Zach Lipton [:zach]]

New patch will be up by 4 this afternoon

Comment 46

[Myk Melez [:myk] [@mykmelez]]

This is still a blocker until the first half of the fix gets checked in.

Comment 47

[Zach Lipton [:zach]]

Attachment: Fix Gerv's points, change to bugzilla.org

Comment 48

[Dave Miller [:justdave]]

Comment on attachment 77337 [details] [diff] [review]
Fix Gerv's points, change to bugzilla.org

r= justdave

passes tests, takes all manner of wonky version strings I tried throwing at it.

But get rid of the tabs you added to 004template.t before you check it in :)

Comment 49

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 77337 [details] [diff] [review]
Fix Gerv's points, change to bugzilla.org

Works for me.  r=myk

Comment 50

[Zach Lipton [:zach]]

stage 1 commited, keeping this open to track the second part of this bug 
(adding the logic to checksetup)

Comment 51

[Myk Melez [:myk] [@mykmelez]]

Correcting the milestone now that part one has been committed.

Comment 52

[Marcus Pallinger]

Out of curiosity, wouldn't it be better to use the RCSid as the version numer,
as that's guranteed to change whenever a template gets updated. There appear to
be a few files that havn't had their version updated over several revisions in
CVS. This would make it extreemly hard for an admin to pickup changes in files.

Also, another advantage of using RCSids is that when I do an update, I can use
cvs diff to find out what's changed in a template since the last time I synced
it, without having to guess what has changed.
(our site tends to follow the CVS version of Bugzilla)

Comment 53

[Zach Lipton [:zach]]

We considered using the RCS version string, however, doing that means 
that the version number _always_ is increased, and there are times when 
we don't want to do that. Also, we want to be able to bump the major 
version string ( the 1 in 1.0) for big changes and the minor string (the .0 in 
1.0) for little quick changes. You can't do that with RCS versions.

Comment 54

[Bradley Baetz (:bbaetz)]

You can up the major verison, but its a pain, and I don't know if bonsai handles
handles it.

Comment 55

[Dave Miller [:justdave]]

This already has a checkin, I want it off the "ready to check in" list since
it's not and/or already has been.  Let's do a new bug to finish the job.

Comment 56

[Matthew Tuck [:CodeMachine]]

Where's the other half of this?

Comment 57

[GavinS]

For reference, the other part if this is bug#186866

Comment 58

[Frédéric Buclin]

Do you realize that this bug is the most useless "blocker/P1 ultra important" bug ever filed? :) 5 years later, all templates still mention [e-mail]. Back this out! And I'm not kidding.