Last Comment Bug 723135 - Language packs should work for all releases of a branch, maxVersion should use * for compatibility ranges
: Language packs should work for all releases of a branch, maxVersion should us...
Status: RESOLVED FIXED
[qa+]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: Mark Banner (:standard8)
:
Mentors:
http://releases.mozilla.org/pub/mozil...
: 728567 (view as bug list)
Depends on:
Blocks: 723138
  Show dependency treegraph
 
Reported: 2012-02-01 08:41 PST by Sebastian H. [:aryx][:archaeopteryx]
Modified: 2012-06-05 02:23 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
13+
verified


Attachments
Proposed fix (3.15 KB, patch)
2012-02-27 12:42 PST, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Proposed fix v2 (3.20 KB, patch)
2012-03-06 12:05 PST, Mark Banner (:standard8)
l10n: review+
ted: review+
bugspam.Callek: feedback+
Details | Diff | Splinter Review
Proposed fix v3 (4.15 KB, patch)
2012-04-13 07:34 PDT, Mark Banner (:standard8)
ted: review+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Sebastian H. [:aryx][:archaeopteryx] 2012-02-01 08:41:59 PST
The language packs provided in the release folder on the ftp should work for all releases of a branch and not only a single release. With Enterprise Support Releases (ESR), this should provide more benefits (but also for normal users). At the moment, the language pack have the same minVersion and maxVersion, but because the strings are frozen, compatibility ranges should look like 10.0 - 10.0.* (This is being done for Firefox 3.6.x)
Comment 1 Matthias Versen [:Matti] 2012-02-18 09:03:03 PST
*** Bug 728567 has been marked as a duplicate of this bug. ***
Comment 2 Mark Banner (:standard8) 2012-02-27 12:42:11 PST
Created attachment 601019 [details] [diff] [review]
Proposed fix

I need this macro for some other things, but wanted to fix langpacks whilst I'm here :-)

The basic idea is to define a macro that looks for 'a' (i.e. a1 or a2) and if it isn't present, then we add '.*' onto the end of the version string.
Comment 3 Mark Banner (:standard8) 2012-02-27 13:05:01 PST
Comment on attachment 601019 [details] [diff] [review]
Proposed fix

So I forgot to account for the chemspill case :-(

Will have to rethink the macro a little, in the meantime feedback (and fixes!) welcome.
Comment 4 Mark Banner (:standard8) 2012-03-06 12:05:33 PST
Created attachment 603397 [details] [diff] [review]
Proposed fix v2

Better fix, this takes account of 10.0.1 style numbers. I'm not too keen on the shell, but I couldn't find a better way of fixing it.
Comment 5 Justin Wood (:Callek) 2012-03-10 00:47:59 PST
Comment on attachment 603397 [details] [diff] [review]
Proposed fix v2

This does address my previous concerns as well!
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-11 15:44:44 PDT
I'm waiting for Axel's review before looking at this.
Comment 7 Axel Hecht [:Pike] 2012-03-11 16:03:45 PDT
Comment on attachment 603397 [details] [diff] [review]
Proposed fix v2

Review of attachment 603397 [details] [diff] [review]:
-----------------------------------------------------------------

In principle, this is the right thing to do, but I don't grok the regexp foo, and if that corresponds/implements our stable release version numbers.

I'm marking an r+ pending on someone else making a strong review on that mapping. Kyle, if that's you, that'd be great, otherwise, would you know someone?
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-11 16:07:17 PDT
I can do it.  I was kind of hoping you would ;-)
Comment 9 Justin Wood (:Callek) 2012-03-11 17:10:45 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> I can do it.  I was kind of hoping you would ;-)

For what its worth, I did as both a c-c Build System Peer, and the L10n Driver for SeaMonkey. Though I'm not a m-c Build System Peer so I only marked it f+. But could certainly benefit by another set of eyes.
Comment 10 Mark Banner (:standard8) 2012-03-12 13:03:31 PDT
Comment on attachment 603397 [details] [diff] [review]
Proposed fix v2

Maybe ted will?
Comment 11 Ted Mielczarek [:ted.mielczarek] 2012-03-15 07:30:10 PDT
Comment on attachment 603397 [details] [diff] [review]
Proposed fix v2

Review of attachment 603397 [details] [diff] [review]:
-----------------------------------------------------------------

The concept is fine. Might make more sense to do this in configure.in instead of config.mk, though.

::: config/config.mk
@@ +835,5 @@
>  EXPAND_MOZLIBNAME = $(foreach lib,$(1),$(DIST)/lib/$(LIB_PREFIX)$(lib).$(LIB_SUFFIX))
> +
> +# EXPAND_MAXVERSION - $(call EXPAND_MAXVERSION,version)
> +# Expands 10.0 to 10.0.* for ajdusting extension and xpi compatibility ranges
> +EXPAND_MAXVERSION = $(if $(findstring a,$(1)),$(1),$(shell echo $(1) | sed "s/\(^[0-9]*\.[0-9]*\).*/\1/").*)

This seems fine, but you probably want that first grouping to be [0-9]+, right? Also you could probably calculate this in configure.in instead of running this command every time we build.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-19 09:25:40 PDT
Comment on attachment 603397 [details] [diff] [review]
Proposed fix v2

Review of attachment 603397 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like Ted took a good look at this.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-06 14:48:50 PDT
[Triage Comment]
If this is ready to nominate for landing approval please go ahead and do so in order to get it landed and off our tracking list. Thanks.
Comment 14 Mark Banner (:standard8) 2012-04-13 06:53:06 PDT
(In reply to Ted Mielczarek [:ted] from comment #11)
> The concept is fine. Might make more sense to do this in configure.in
> instead of config.mk, though.

I need to get this landed and haven't got time to do this switch at the moment, so I'll file a follow-up bug for that.

> ::: config/config.mk
> @@ +835,5 @@
> >  EXPAND_MOZLIBNAME = $(foreach lib,$(1),$(DIST)/lib/$(LIB_PREFIX)$(lib).$(LIB_SUFFIX))
> > +
> > +# EXPAND_MAXVERSION - $(call EXPAND_MAXVERSION,version)
> > +# Expands 10.0 to 10.0.* for ajdusting extension and xpi compatibility ranges
> > +EXPAND_MAXVERSION = $(if $(findstring a,$(1)),$(1),$(shell echo $(1) | sed "s/\(^[0-9]*\.[0-9]*\).*/\1/").*)
> 
> This seems fine, but you probably want that first grouping to be [0-9]+,
> right?

Nope, because if we do, then (at least on my version) it converts 10.0.1 to 10.0.1.*, so I believe we need it to be more hungry.
Comment 15 Mark Banner (:standard8) 2012-04-13 07:34:50 PDT
Created attachment 614772 [details] [diff] [review]
Proposed fix v3

It turns out moving it to configure was easier that I thought.
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-19 13:27:44 PDT
This will have to wait and land on ESR after 4/24 merge day since we're now at code-freeze and going to build tomorrow. Updating tracking flag to reflect that.
Comment 17 Mark Banner (:standard8) 2012-04-23 06:11:24 PDT
Landed on birch: https://hg.mozilla.org/projects/birch/rev/b0417d14dac8
Comment 19 Mark Banner (:standard8) 2012-04-25 00:59:00 PDT
Comment on attachment 614772 [details] [diff] [review]
Proposed fix v3

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes the maximum version in language packs to be something like 10.0.*, this means a language pack can be used for all the versions on the branch. Given the chat on the Enterprise Working List about the issues with language packs around the 10.0.3 release, its seems to be of benefit to them to set this value automatically the first time around.
User impact if declined: Admins using ESR can't just pick up one version of a language pack and run with it.
Fix Landed on Version: 15
Risk to taking this patch (and alternatives if risky): Very low. Affects language packs only.
String changes made by this patch: None
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 13:01:36 PDT
Comment on attachment 614772 [details] [diff] [review]
Proposed fix v3

[Triage comment]
Low risk, fixes langpacks for ESR - please go ahead and land to ESR branch.
Comment 21 Mark Banner (:standard8) 2012-04-26 02:09:05 PDT
Checked into ESR without the b2g or mobile changes as they don't apply, and aren't needed there for the desktop version which is most affected:

https://hg.mozilla.org/releases/mozilla-esr10/rev/37fdf8dbe871
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-22 15:33:08 PDT
@aryx, can you verify this is fixed with latest-mozilla-esr10?
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-esr10/
Comment 23 Virgil Dicu [:virgil] [QA] 2012-05-31 02:37:16 PDT
Mozilla/5.0 (X11; Linux i686; rv:10.0.5esrpre) Gecko/20120525 Firefox/10.0.5esrpre

ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-esr10/

The language pack listed in the above link can be installed (is compatible) in Firefox 10.0.5 and can't be installed with F10.0.4 ESR.

Is this enough to set this to verified? Are there any other language packs which did not previously work with 10.0.4ESR?
Comment 24 Mark Banner (:standard8) 2012-05-31 03:00:38 PDT
(In reply to Virgil Dicu [:virgil] [QA] from comment #23)
> Mozilla/5.0 (X11; Linux i686; rv:10.0.5esrpre) Gecko/20120525
> Firefox/10.0.5esrpre
> 
> ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-esr10/
> 
> The language pack listed in the above link can be installed (is compatible)
> in Firefox 10.0.5 and can't be installed with F10.0.4 ESR.

In theory a langpack for ESR can be installed in any 10.0.* version. However, because of the way the nightly builds are done, its actually messed up some of the detection, so this will only work for released esr builds.
Comment 25 Virgil Dicu [:virgil] [QA] 2012-05-31 04:49:10 PDT
So I guess this could only be verified with official candidates. 

Are there known STRs that exhibited the issue and which can be used to verify against?
Comment 26 Axel Hecht [:Pike] 2012-05-31 15:36:52 PDT
Reading the patches, the minversion is never changed? So a 10.0.4 langpack will be compatible with 10.0.4 min, and 10.0.* max?
Comment 27 Mark Banner (:standard8) 2012-06-01 03:43:06 PDT
(In reply to Axel Hecht [:Pike] from comment #26)
> Reading the patches, the minversion is never changed? So a 10.0.4 langpack
> will be compatible with 10.0.4 min, and 10.0.* max?

That's true, I hadn't really thought about that - I think my main idea was getting it right from the start of the branch (although obviously this was too late for esr). If we want to fix the min version as well, we should do that in a different bug.
Comment 28 Kevin Brosnan [:kbrosnan] 2012-06-02 09:50:28 PDT
Checked the following directories
ftp://ftp.mozilla.org/pub/firefox/candidates/10.0.5esr-candidates/build1/linux-i686/xpi/
ftp://ftp.mozilla.org/pub/firefox/candidates/10.0.5esr-candidates/build1/linux-x86_64/xpi/
ftp://ftp.mozilla.org/pub/firefox/candidates/10.0.5esr-candidates/build1/mac/xpi/
ftp://ftp.mozilla.org/pub/firefox/candidates/10.0.5esr-candidates/build1/win32/xpi/

I spot checked a langpack from each. All had <em:maxVersion>10.0.*</em:maxVersion>.

I checked the 13 release candidate and the langpacks in ftp://ftp.mozilla.org/pub/firefox/candidates/13.0b7-candidates/build1/linux-i686/xpi/ do not have this fixed. From reading the bug comments this looks to be expected.
Comment 29 Mark Banner (:standard8) 2012-06-05 02:23:05 PDT
(In reply to Kevin Brosnan [:kbrosnan] from comment #28)
> I checked the 13 release candidate and the langpacks in
> ftp://ftp.mozilla.org/pub/firefox/candidates/13.0b7-candidates/build1/linux-
> i686/xpi/ do not have this fixed. From reading the bug comments this looks
> to be expected.

Yes, they won't be fixed until FF 15.

Note You need to log in before you can comment on or make changes to this bug.