780562 - Remove unnecessary content manifest for global-platform

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Mike Hommey [:glandium]]

There are no content files shipped for global-platform and global-region, only locale files.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Remove unnecessary content manifest for global-platform and global-region

Comment 2

[Benjamin Smedberg]

Comment on attachment 649206 [details] [diff] [review]
Remove unnecessary content manifest for global-platform and global-region

It used to be that you had to register a content package before you could register a locale package. You've run this through tryserver?

Comment 3

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #2)
> Comment on attachment 649206 [details] [diff] [review]
> Remove unnecessary content manifest for global-platform and global-region
> 
> It used to be that you had to register a content package before you could
> register a locale package. You've run this through tryserver?

I only tested locally, but I sure can push to try.

Comment 4

[Mike Hommey [:glandium]]

So, in fact, the problem is that the "platform" keyword is only recognized for content. So while global-region is okay without a content package, global-platform is a different story. OTOH, now that there is the os flag, is there a reason to keep "platform" (especially considering global-platform is apparently the last place to use it)?

Comment 5

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #4)
> OTOH, now that there is the os flag,
> is there a reason to keep "platform" (especially considering global-platform
> is apparently the last place to use it)?

Mmmm one issue is that it's not possible to do AND rules with flags...

Comment 6

[Mike Hommey [:glandium]]

global-region was removed in bug 1277905.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

Review commit: https://reviewboard.mozilla.org/r/130458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130458/

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.

Review commit: https://reviewboard.mozilla.org/r/130460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130460/

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 780562 - Remove support for the chrome "platform" flag.

Review commit: https://reviewboard.mozilla.org/r/130462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130462/

Comment 10

[Benjamin Smedberg]

Comment on attachment 8858493 [details]
Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

https://reviewboard.mozilla.org/r/130458/#review134038

Bumping review to Axel. The serious problem that caused us not to do this in the past is that locale packs are not platform-dependent, and this effectively changes that. So I suspect that this is not ok, but the new world of l10n may have changed that.

Comment 11

[Mike Hommey [:glandium]]

AFAIK, the lang packs are now per platform.

Comment 12

[Mike Hommey [:glandium]]

Mmmm maybe not... https://addons.mozilla.org/en-US/firefox/language-tools/

I guess I got confused by http://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central-l10n/*/xpi/

Comment 13

[Mike Hommey [:glandium]]

If that doesn't work, we can still replace "platform" with the relevant "os" tests.

i.e.

locale global-platform @AB_CD@ %locale/@AB_CD@/global-platform/unix/
locale global-platform @AB_CD@ %locale/@AB_CD@/global-platform/mac/ os=Darwin
locale global-platform @AB_CD@ %locale/@AB_CD@/global-platform/win/ os=WINNT

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 8858493 [details]
Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

https://reviewboard.mozilla.org/r/130458/#review134238

Yeah, we don't support platform dependent language packs. We introduced bugs in that already with conditional packaging of files, I know, but we shouldn't break this further. Looking at the allowed-dupes changes, this might already be fixed, didn't look hard.

At some point we'll want to get rid of the chrome registry for localization files alltogether, so we'll want to solve this in some way.

One way to do it would be to do the platform detection at the callsites, and reference platform specific strings, by file or by id. That's what we do for Preferences vs Settings, for example.
The other way would be to fast-path l20n with platform selectors for this on desktop, but I don't know what we need to do on the callsites to get this going. CCing gandalf, maybe he can spend some thoughts on this.

r- on the patch and the follow-ups, we'll need to solve this differently such that we don't introduce platform dependencies in the langpack distributable.

Comment 15

[Mike Hommey [:glandium]]

My immediate goal is to get rid of the "platform" flag for chrome, such that bug 1158018 can finally be fixed without working around it.  Comment 13 would work just fine for that goal and wouldn't change the status quo wrt chrome registration in langpacks. Would you be okay with that for now? (also, you didn't CC gandalf)

Comment 16

[Axel Hecht [:Pike]]

Maybe if you add a platform-flagged chrome override? (AFAICT, comment 13 would still lead to different chrome paths at runtime?)

AKA, I'm fine with chrome registry being runtime dependent, as long as the language pack xpi works as is on all platforms.

(I CCed gandalf before I submitted my review, knowing I couldn't do that through mozreview ;-) , probably w/out bugmail)

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm working on moving l10n resources away from chrome registry all together (bug 1333980).

It's one of the items on the path to the new l10n API which will allow us to get string variants per-platform, so one could write:

```
key = { PLATFORM() ->
  [win] Settings
  [mac] Peferences
 *[other] Options
}
```

On top of that, I start to suspect that intl.properties may have a better home in form of a .toml file instead of a .properties file combined into a complex pref.

So, there are some changes that I'll be working on over the next quarter.

If you want to fix it sooner, I'm with Pike. Do whatever you need to, but don't make us create per-platform langpacks.

I tried to read bug 1158018 but it feels like you guys make a lot of mental shortcuts when talking between each other and it's a bit hard to grasp the scope of the problem, at least in how it affects the platform specific l10n resources.
Could you give me a tl;dr and I'll try to help you with the solution? :)

Comment 18

[Mike Hommey [:glandium]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> Could you give me a tl;dr and I'll try to help you with the solution? :)

The tl;dr is that we want the packaging steps to just use the chrome manifests to determine what to package, and we make it error out when things referenced in chrome manifests are missing. Which the empty content entry for global-platform triggers. We could add a one-off exception for that, but we might as well get rid of the "platform" thing entirely, considering it's possible to do the same without it.

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8858493 [details]
Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130458/diff/1-2/

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8858494 [details]
Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130460/diff/1-2/

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8858495 [details]
Bug 780562 - Remove support for the chrome "platform" flag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130462/diff/1-2/

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8858493 [details]
Bug 780562 - Take locale chrome manifest flags into account when repacking l10n.

https://reviewboard.mozilla.org/r/130458/#review141358

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8858494 [details]
Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.

https://reviewboard.mozilla.org/r/130460/#review141362

::: commit-message-115ad:15
(Diff revision 2)
> +Unfortunately, for determinism reasons, the chrome manifest entries from
> +jar.mn are sorted (per bug 982075), so keeping global-platform/unix
> +would leave it appearing after /mac, and would override it on mac
> +because of the lack of "os" flag on the /unix entry (we can't put "os"
> +flags on that entry because we can't do something like os!=Darwin &&
> +os!=WINNT). So we move it to /gtk such that it always comes before /mac.

Do you feel this potential footgun requires an inline comment? Or even better, can the parser verify that flag-less entries appear before entries with flags?

(Or since this pattern is only used once, perhaps we'll just take a risk that the footgun never fires.)

Comment 24

[Benjamin Smedberg]

Comment on attachment 8858494 [details]
Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.

https://reviewboard.mozilla.org/r/130460/#review143084

Boy that's quite the footgun. Perhaps it would make sense to implement a specific manifest marker for all the unix-like OSes?

Comment 25

[Benjamin Smedberg]

Comment on attachment 8858495 [details]
Bug 780562 - Remove support for the chrome "platform" flag.

https://reviewboard.mozilla.org/r/130462/#review143094

Comment 26

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #24)
> Comment on attachment 8858494 [details]
> Bug 780562 - Stop relying on the "platform" chrome flag for global-platform.
> 
> https://reviewboard.mozilla.org/r/130460/#review143084
> 
> Boy that's quite the footgun. Perhaps it would make sense to implement a
> specific manifest marker for all the unix-like OSes?

os=Unix?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47661994bdb1
Take locale chrome manifest flags into account when repacking l10n. r=gps
https://hg.mozilla.org/integration/autoland/rev/c94e87a18096
Stop relying on the "platform" chrome flag for global-platform. r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/82edbfe72c47
Remove support for the chrome "platform" flag. r=bsmedberg

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47661994bdb1
https://hg.mozilla.org/mozilla-central/rev/c94e87a18096
https://hg.mozilla.org/mozilla-central/rev/82edbfe72c47