783108 - re-packed add-ons need their version numbers bumped.

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

For re-packing, we need to incrment the add-on version in the xpi's install.rdf file in order for Firefox to trigger an upgrade ask.

According to Mossop, the simplest and least invasive way to do this would be to add the string '.1' to the end of the existing version.

Implementation sketch:

 - once we've created an xpi based on the un-packed sources, we then extract and 'bump' the version number.

Pull request:

https://github.com/ochameau/jetpack-repacker/pull/1

Comment 1

[Dave Townsend [:mossop]]

To clarify this is based on what we decided we were going to do to version numbers when repacking in the way back when. The first repack would add ".1". If repacking a second time it would change the ".1" to ".2", and so on. If the developer releases a new version then we fall back to adding ".1" the next time.

So for this first pass adding ".1" is just fine. But if we have to do this again (which seems likely) then we'll need some more clever logic. It might be worth considering how we do it this time round so we can detect the ".1" was added by us so the next time round we'll be able to act accordingly.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Assuming Firefox's update logic works the same way with characters in the version number string, I think that all future bikesheds should end in ".rXX", where "r" stands for "repack", and "XX" is the version number. 

When the repack script goes through, it should split the version number field at the ".". If there is no ".", then the script should add a ".r1" to the end of the string.
Same if the final piece of the split does not contain the "r" (so 3.6.1 will become 3.6.1.r1). If the final piece of the split does contain the "r", increment the number after the "r" (so 3.6.1.r1 becomes 3.6.1.r2).


Alternatively don't have the repacker care if it repacked it, and just do the split on "." and if it doesn't exist (or the final piece of the split is not just an integer), append the ".1" to the end of it. If the final piece is an integer, increment that piece.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Wes Kocher (:KWierso) from comment #2)
> Assuming Firefox's update logic works the same way with characters in the
> version number string, I think that all future bikesheds should end in
> ".rXX", where "r" stands for "repack", and "XX" is the version number. 

Sadly they don't, .rXX would not be enough to make Firefox think a new version is available. See discussion in bug 691561 where we were originally using .sdk.X.X and it didn't work.

We could use .1r, that would work, but is ugly and we've also heard that authors don't like the text sometimes.

I don't think we have a big problem here, the XPI contains harness-options.json which should contain the original version. install.rdf contains the version presented to Firefox. If they match then you add .1 to install.rdf. If they don't then you increment the bit at the end of install.rdf. Making sure to leave the original version in harness-options.json all the time.

That should work for all cases except authors manually hacking things in their XPI.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Ah, I guess we do not to round-trip this, sorry.

It seems like the slightly more complicated logic would be something like:

 - if the versions in harness-options and install.rdf match, we append '.1'

 - if the rdf version contains a string '.[0-9]+' at the end *and* the version in harness-options does not contain the extra digit but they otherwise match, we increment the digit

 - if there is some other total mis-match between the two versions, we take the harness-options version and append a '.1'

The last cause seems unlikely unless the xpi was edited manually?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Jeff Griffiths (:canuckistani) from comment #4)
>  - if there is some other total mis-match between the two versions, we take
> the harness-options version and append a '.1'
> 
> The last cause seems unlikely unless the xpi was edited manually?

Yeah, but in that case we have to append a ".1" to the install.rdf version (to ensure we're getting something larger in Firefox's eyes). Maybe we should replace the harness-options version with what is in install.rdf at that point so it matches what we expect next time around?

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Dave Townsend (:Mossop) from comment #5)
> ... Maybe we
> should replace the harness-options version with what is in install.rdf at
> that point so it matches what we expect next time around?

Yeah, I'm not sure if the correct value to take is from harness options or install.rdf. I'm wondering if install.rdf's version should always be derived from harness-options.

So:

if (harness === rdf)
	rdf += '.1'
else if (new RegExp(harness+'\.[\d]+$').test(rdf) === true) // a digit is appended
	increment(rdf)
else
	rdf = harness +'.1'

...my reasoning is that we generate harness-options from package.json, that feels like it should take precedence over whatever is in install.rdf.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Jeff Griffiths (:canuckistani) from comment #6)
> (In reply to Dave Townsend (:Mossop) from comment #5)
> > ... Maybe we
> > should replace the harness-options version with what is in install.rdf at
> > that point so it matches what we expect next time around?
> 
> Yeah, I'm not sure if the correct value to take is from harness options or
> install.rdf. I'm wondering if install.rdf's version should always be derived
> from harness-options.

It /should/ be always the same, unless the developer has edited it.

> So:
> 
> if (harness === rdf)
> 	rdf += '.1'
> else if (new RegExp(harness+'\.[\d]+$').test(rdf) === true) // a digit is
> appended
> 	increment(rdf)
> else
> 	rdf = harness +'.1'
> 
> ...my reasoning is that we generate harness-options from package.json, that
> feels like it should take precedence over whatever is in install.rdf.

That would be great, except what is important here is what version Firefox sees for the old and new versions and those are the versions in install.rdf. With your logic:

harness = "1.0"
rdf = "2.0"

Your result would be to put "1.0.1" into the rdf, which would make it a lower version and so Firefox would ignore it as an update.

Comment 8

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Dave Townsend (:Mossop) from comment #7)
...
> With your logic:
> 
> harness = "1.0"
> rdf = "2.0"
> 
> Your result would be to put "1.0.1" into the rdf, which would make it a
> lower version and so Firefox would ignore it as an update.

Argh. I'm comfortable with throwing that corner case under the bus of progress if you are. I suppose there is some additional safety that could be added ( pseudo-code ):

# assuming both versions have int-ish first chars...
if first_char_as_int(harness) < first_char_as_int(rdf):
    new = rdf+'.1'

Comment 9

[Dave Townsend [:mossop]]

New idea, just refuse to repack if rdf and harness differ by more than we expect. It's probably not done anywhere in the real world, and if it is they can handle it themselves

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I took a look at the current crop of re-pack candidates and all of them had identical versions in install.rdf and harbess-options.json *except* a single odd case where there was no version field in the json file.

So, we're bike-shedding a bit - it looks like we have 3 scenarios:

1. the versions are identical and we append '.1'
2. the rdf version has already been touched and we increment what is there to the next int
3. the json value is missing, so we copy the original rdf version into the json, then slap '.1' onto it and copy *that* back into the rdf

Comment 11

[Dave Townsend [:mossop]]

I declare that winning

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Alex already implemented this separately in his script.