592574 - removed-files is not L10n-aware

Status: RESOLVED DUPLICATE of bug 642765

(Firefox Build System :: General, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

Localised builds updating to 4.0b5 with a complete (eg 4.0b4 or older, failed partials) don't remove the localized chrome. mwu says no manifest is referencing the file so it's not loaded, and hence can't cause any issues and this bug is about cleaning up after ourselves. There's an entry in removed-files.in but we aren't regenerating removed-files for each locale, just once for en-US.

partial updates from 4.0b4 to b5 do remove the jar file, since the partial generator notices the file went away.

Comment 1

[Michael Wu [:mwu]]

Bug 579178 caused this by renaming @AB_CD@.jar to localized.jar or something like that. (though omnijar would've later if bug 579178 didn't)

Comment 2

[Benjamin Smedberg]

Unless we were to add every localized JAR to removed-files (which I guess is an option), this is probably WONTFIX.

Comment 3

[Robert Kaiser]

(In reply to comment #2)
> Unless we were to add every localized JAR to removed-files (which I guess is an
> option), this is probably WONTFIX.

I don't think so, we just need to teach the repack process to regenerate removed-files - and I have a patch for that (coming up in a minute)!

Comment 4

[Robert Kaiser]

Attachment: v1: make the a repack regenerate and replace removed-files

This patch regenerates and replaces removed-files when doing a repack. Nicely, when MOZ_PKG_REMOVALS is not set in locales/Makefile, we just ignore this and do things like before, so this doesn't break SeaMonkey (which I also have a patch for), Thunderbird or others when this lands.

Comment 5

[Robert Kaiser]

Hmm, I see this has been "fixed" the hacky way for Firefox in bug 590953, but using this patch we could revert the locale file changes done here. Due to the changes done there this currently has no actual effect on Firefox, but it enables us to use @AB_CD@ in removed-files.in - SeaMonkey really wants to be able to do that, and I think it would be a good idea to go back to this for Firefox as well.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 520483 [details] [diff] [review]
v1: make the a repack regenerate and replace removed-files

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

Sorry for the lag, this patch looks good in principle, but bitrotted. Thus r-, will need a new patch.

Comment 7

[Robert Kaiser]

Attachment: v1.1: unbitrotted

Here's an unbitrotted version.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 534230 [details] [diff] [review]
v1.1: unbitrotted

I'm sorry, one would think this patch wouldn't bitrot, but it does. FWIW, I'd suggest rs as a reviewer more than me.

Comment 9

[Robert Kaiser]

(In reply to Axel Hecht [:Pike] from comment #8)
> Comment on attachment 534230 [details] [diff] [review] [diff] [details] [review]
> v1.1: unbitrotted
> 
> I'm sorry, one would think this patch wouldn't bitrot, but it does. FWIW,
> I'd suggest rs as a reviewer more than me.

The patch didn't bitrot, only the context did slightly. And I thought you would be the one for L10n hacks, but given that I've mostly given up on ever getting reviews for any of my mozilla-central patches, I'll follow any advice (it just takes a lot of time to follow any step because the motivation is way higher to do things that have a realistic chance of success within a reasonable time). It's only been almost 5 months since I first posted this patch for review.

Comment 10

[Robert Kaiser]

Attachment: v1.2: more context unbitrotting

OK, here's another context-unbitrotted patch, now trying Rob for review.
This would give us a cleaner way for cleaning up locale files on updates.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 551285 [details] [diff] [review]
v1.2: more context unbitrotting

I verified that it does the right thing during repackaging. I'd like Nick to review the patch to make sure it does the right thing when generating nightly and release mars.

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 551285 [details] [diff] [review]
v1.2: more context unbitrotting

Does this require that we use the same configure arguments for l10n as for en-US ? Not all of the #ifdef & #ifndef in the m-c removed-files.in are platform tests.

Comment 13

[Robert Kaiser]

(In reply to Nick Thomas [:nthomas] from comment #12)
> Does this require that we use the same configure arguments for l10n as for
> en-US ? Not all of the #ifdef & #ifndef in the m-c removed-files.in are
> platform tests.

Hmm, I guess it probably does, yes. Thanks for thinking that far. IMHO, it would be nice anyhow if we didn't need different mozconfigs for en-US and L10n repacks anyhow.

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 551285 [details] [diff] [review]
v1.2: more context unbitrotting

minusing per nthomas's comment #12

Comment 15

[Robert Kaiser]

I'm giving up on this. It might still be nice but I don't want to care any more about doing patches that for some reason don't make it anyhow.

Comment 16

[Robert Strong (they/them - no direct email)]

When Bug 642765 landed it made it so on a complete update all files are staged for removal before adding new files and on partial update any files no longer present are added as remove instructions. This doesn't apply to the distribution directory since we aren't suppose to mess with that on update. Even if the code to change channels is removed the code to do the above will still remain.

I think this is fixed by that bug but I am leaving it open in case someone can come up with a use case the above doesn't cover.

Comment 17

[Robert Strong (they/them - no direct email)]

Resolving as duplicate of bug 642765. There may be cases where we want to do something like this but I can't think of any and the fix from Bug 642765 covers the case that caused this bug to be reported. If anyone thinks removed-files still needs to be made l10n aware please reopen.

Comment 18

[Axel Hecht [:Pike]]

The other usecase is removal of searchplugins we're not shipping anymore. Right now, we're adding them to all locales, which leads to the lengthy list of them in http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-files.in#287

Not sure if that's a big enough problem, though.

Looking through this, and in the light that complete updates now removes everything, I guess we can get rid of a ton of entries? Anything that we removed less than a version ago would go into a complete update and be removed anywho?

Comment 19

[Robert Strong (they/them - no direct email)]

(In reply to Axel Hecht [:Pike] from comment #18)
> The other usecase is removal of searchplugins we're not shipping anymore.
> Right now, we're adding them to all locales, which leads to the lengthy list
> of them in
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-
> files.in#287
> 
> Not sure if that's a big enough problem, though.
I'm not concerned about it but if you want to reopen please do so.

> Looking through this, and in the light that complete updates now removes
> everything, I guess we can get rid of a ton of entries? Anything that we
> removed less than a version ago would go into a complete update and be
> removed anywho?
Yes and I plan to do so... there are prerequisites (all client platforms need to update to a build with the functionality) and it will be cleaned up after there is such a requirement.