Closed Bug 505713 Opened 11 years ago Closed 10 years ago

Factor out multiple jar-file handling in


(Toolkit Graveyard :: Build Config, defect)

1.9.1 Branch
Not set


(status1.9.2 beta2-fixed)

Tracking Status
status1.9.2 --- beta2-fixed


(Reporter: Pike, Assigned: Pike)



(2 files, 1 obsolete file) supports handling multiple jar.mns at once, but only via the commandline. I'd like to actually use that feature in a script, and thus factor that code into a method on the JarMaker class.
This is basically just moving code from the __main__ code into a method, up to one difference:

I added support to have l10nbase directories as callables. They're called with the deduced relativesrcdirs, and enable the caller to pass in lambdas like

lambda rel: os.path.join(topsrc, rel, 'locales', 'en-US')

to pick up files from en-US as part of l10n merge. In the regular builds, we do this on a per-directory level and just pass l10nsrcdirs for merge, l10n, en-US. When I actually put a few jar.mns into one call, I need to tweak the relativesrcdir into the path.

I went for callables so that the directory structure and logic of l10n merge can stay within the build foo, and doesn't need hard-coding inside

Use case is That script creates a language pack without build dir, directly from sources.

Tested with a full Firefox build on os/x, too.
Attachment #390466 - Flags: review?(ted.mielczarek)
Comment on attachment 390466 [details] [diff] [review]
factor the mutliple-file code into method, add support for callables as l10nbase

Trying Ted for this review, too.
Attachment #390466 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 390466 [details] [diff] [review]
factor the mutliple-file code into method, add support for callables as l10nbase

+        l10ndirs += map(lambda s: os.path.normpath(os.path.abspath(s)),
+                        localedirs)
+      srcdirs = map(lambda s: os.path.normpath(os.path.abspath(s)),
+                    sourcedirs) + \

Any reason not to use list comprehensions here? I think they're a bit more readable in Python. (I realize you're just refactoring here.)

Also, it probably wouldn't hurt to write a Python unittest for these methods, although I wouldn't block your checkin on it, since you already have the tests.
blocking1.9.1: needed → ---
Attached patch adding tests for (obsolete) — Splinter Review
I had landed the actual patch to a bit ago,

Here some tests for, one for a really simple, and the other for the multi-relpath case.

I didn't go into doing an extensive test suite for, we could do that in a separate bug, too.
Attachment #407316 - Flags: review?(ted.mielczarek)
Windows puked at me,

Gotta dig into that on a VM tomorrow. I suspected line endings for a minute, but I'm not sure where I would create different ones.
Comment on attachment 407316 [details] [diff] [review]
adding tests for

New patch working on the try server, gonna attach that tomorrow.

The unzip code had newline problems, I had to open then file with 'wb' there. And the manifest files are written out with unix line endings on windows, too, so I had to 'wb' the ref file for that as well.
Attachment #407316 - Attachment is obsolete: true
Attachment #407316 - Flags: review?(ted.mielczarek)
This time, with love.

I created, which has a testrunner and testresult that together create output as our unit test frameworks likes it for stats. For example,

TEST-PASS | tests/ | test_a_simple_jar
TEST-PASS | tests/ | test_k_multi_relative_jar

Tested on try, and it did bump the number of checks.

I also did the whitespace fixes and some more fixes to the reference that I hadn't caught before I tried to reproduce all error messages.

I'm not using for anything but yet, if it flies we can migrate the other unit tests over in different bugs.
Attachment #407613 - Flags: review?(ted.mielczarek)
Comment on attachment 407613 [details] [diff] [review]
tests, now working, and with to report nicely

+  \
+  \
+           $(NULL)

Can you just move all the test names down to separate lines with two-space indent, while you're here?

+"TEST-UNEXPECTED-FAIL | %s:%d | %s: %s" % 
+                            (_f, _ln, _t, value.message))

I think I'd just make this TEST-UNEXPECTED-FAIL | filename | blah blah at line %d

This is good stuff, thanks!
Attachment #407613 - Flags: review?(ted.mielczarek) → review+, FIXED on central.

We should take both the factorization and the tests on 1.9.2, that's gonna make it a whole lot easier to create language packs locally for testing.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #390466 - Flags: approval1.9.2?
Comment on attachment 390466 [details] [diff] [review]
factor the mutliple-file code into method, add support for callables as l10nbase

a=me to land this as long as it lands with the tests on 1.9.2.
Attachment #390466 - Flags: approval1.9.2? → approval1.9.2+, landed on 1.9.2 with tests.
Target Milestone: mozilla1.9.3a1 → mozilla1.9.2
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.