797745 - Move l10n-merge/relativesrcdir logic from config.mk into JarMaker.py, allow jar.mn to override

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Axel Hecht [:Pike]]

Let's move the handling of l10n-merge (mergedir, l10ndir, en-US dir) and relativesrcdir from config.mk into JarMaker.py.

Public post is at https://groups.google.com/forum/#!topic/mozilla.dev.platform/F-2J5viSCbc, too.

In a first patch, I'll try to rip out some unused code like old perl options and the option to use JarMaker with more than one jar file, in a second one adds new functionality.

Comment 1

[Mike Hommey [:glandium]]

> use JarMaker with more than one jar file

Be careful that there are two places relying on that:
browser/locales/jar.mn
xulrunner/examples/simple/jar.mn

The former was done in bug 762864. See there for reasons why it was done this way.

Comment 2

[Axel Hecht [:Pike]]

err, I mean more than one manifest. jar.mn file. sorry for the confusion.

I want to unsupport JarMaker.py browser/locales/jar.mn toolkit/locales/jar.mn ...

Comment 3

[Axel Hecht [:Pike]]

Pushed to try, https://tbpl.mozilla.org/?tree=Try&rev=5460b8e89a8d.

Oh, can I claim that I was young and needed the money? "over-engineered" is one way to put it.

Comment 4

[Axel Hecht [:Pike]]

Attachment: remove multiple manifests code, old perl options, make non-optional arguments non-optional

This is cleaning up with ideas I had 5 years ago.
I'm removing the feature to process several jar.mn files in one invocation. This doesn't touch generating multiple jars from one jar.mn, though.
I'm also removing all the options of the old perl impl that we never implemented, as they weren't used even at the time.

This and the next patch passed a try-run on https://tbpl.mozilla.org/?tree=Try&rev=c11bebac13be with 
try: -b o -p linux64,macosx64,win32,android -u mochitest-1 -t none

Comment 5

[Axel Hecht [:Pike]]

Attachment: add --l10n-base, --relativesrcdir, --locale-mergedir, and migrate merge code out of config.mk

This is part 2 in a series of three.

This adds three options to pass LOCALE_MERGEDIR, relativesrcdir and L10N_BASE to JarMaker, and implement the l10n-merge inside JarMaker instead of hardcoding the order in config.mk. It also creates the localesrcdir in a method out of the three, so that in step three, I can regenerated those dirs when I hit a relativesrcdir override in a jar.mn

Comment 6

[Axel Hecht [:Pike]]

I think I'm going to use, without leading space, relativesrc foo colon, like

@AB_CD@.jar:
% locale b2g @AB_CD@ %locale/@AB_CD@/b2g/

relativesrcdir dom/locale:
 locale/@AB_CD@/b2g/appstrings.properties (%chrome/appstrings.properties)
% override chrome://global/locale/appstrings.properties chrome://b2g/locale/appstrings.properties

etc.

Figured the trailing colon has the best matching semantics.

Comment 7

[Axel Hecht [:Pike]]

Attachment: oh, found a way to test this, too

I actually realized that I can test the relativesrcdir magic, here's a patch. Created those tests in prep for the next one, I'd be happy to fold this into the previous one.

An variant of this test would be to actually pass in fake commandline args. But then I'd probably want to factor the code in main into a method on JarMaker. Not sure if that's worth it, given that we're already relying on internals in the final assertEquals.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 669501 [details] [diff] [review]
oh, found a way to test this, too

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

This test fails on windows, as it's creating a splendid mix of / and \. Will need to use os.path.join at the right point to make these tests pass on windows.

Otherwise, my latest patch queue passed https://tbpl.mozilla.org/?tree=Try&rev=0bfbeaa1f90c

::: config/tests/unit-JarMaker.py
@@ +267,5 @@
> +        jm.makeJar(self.fake_empty_file, '/NO_OUTPUT_REQUIRED')
> +        self.assertEquals(jm.localedirs,
> +                          ['/L10N_MERGE/browser',
> +                           '/L10N_BASE/browser',
> +                           '/TOPSOURCEDIR/browser/locales/en-US'])

These will need to use os.path.join.

Comment 9

[Axel Hecht [:Pike]]

Attachment: updated tests, work on windows now, too

This is the tests for attachment 668412 [details] [diff] [review], now also working on windows. the rest of comment 7 still applies.

This and the next patch passed on try: https://tbpl.mozilla.org/?tree=Try&rev=bf2144643c3e

Comment 10

[Axel Hecht [:Pike]]

Attachment: relativesrcdir dom/locales: logic

This adds a new syntax variant to jar.mn,

relativesrcdir dom/locales:
 entry line

I also added tests that it's creating the right localesrcdirs.

I'm wondering, do variants of these patches need to land on comm-central, too?

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 668410 [details] [diff] [review]
remove multiple manifests code, old perl options, make non-optional arguments non-optional

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

Always like code removal!

Comment 12

[Axel Hecht [:Pike]]

I'll need a check-in buddy for this, basically on PTO now.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I'm assuming that this should be waiting on all 4 patches getting r+ before checkin?

Comment 14

[Axel Hecht [:Pike]]

the first can land in one go, 2 and 3 likely want to land together, 4 can land separately.

Comment 15

[Axel Hecht [:Pike]]

FYI, attachment 670792 [details] [diff] [review] in bug 792077 shows how relativesrcdir would be used.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 668410 [details] [diff] [review]
remove multiple manifests code, old perl options, make non-optional arguments non-optional

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc7b1fbc8af

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fc7b1fbc8af

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Gah, can't believe I forgot to add [leave open] to the whiteboard :-\

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 668412 [details] [diff] [review]
add --l10n-base, --relativesrcdir, --locale-mergedir, and migrate merge code out of config.mk

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

Sorry for the delay. This looks fine, but feels like it could probably use a unit test.

Comment 20

[(not currently active) Ted Mielczarek]

Oh duh, tests are in the other patch.

Comment 21

[Axel Hecht [:Pike]]

I've folded the second and third patch (relsrcdir and test), and pushed the set of two to try again, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=621a01b74178

Then landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/75f27b51f66a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5950e455f4c3

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75f27b51f66a
https://hg.mozilla.org/mozilla-central/rev/5950e455f4c3