Closed Bug 505713 Opened 11 years ago Closed 10 years ago

Factor out multiple jar-file handling in JarMaker.py

Categories

(Toolkit Graveyard :: Build Config, defect)

1.9.1 Branch
defect
Not set

Tracking

(status1.9.2 beta2-fixed)

RESOLVED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: Pike, Assigned: Pike)

Details

Attachments

(2 files, 1 obsolete file)

JarMaker.py 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 JarMaker.py.

Use case is http://bitbucket.org/pike/langpacker/src/tip/build-langpack.py#cl-70. 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 jar.mn tests.
blocking1.9.1: needed → ---
Attached patch adding tests for JarMaker.py (obsolete) — Splinter Review
I had landed the actual patch to JarMaker.py a bit ago, http://hg.mozilla.org/mozilla-central/rev/6920d39fa27c.

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

I didn't go into doing an extensive test suite for JarMaker.py, we could do that in a separate bug, too.
Attachment #407316 - Flags: review?(ted.mielczarek)
Windows puked at me, http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256065171.1256073644.23417.gz&fulltext=1#err0.

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 JarMaker.py

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 mozunit.py, which has a testrunner and testresult that together create output as our unit test frameworks likes it for stats. For example,

TEST-PASS | tests/unit-JarMaker.py | test_a_simple_jar
TEST-PASS | tests/unit-JarMaker.py | 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 mozunit.py for anything but unit-JarMaker.py 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 mozunit.py to report nicely

 PYUNITS := unit-Expression.py unit-Preprocessor.py unit-nsinstall.py \
-           unit-printprereleasesuffix.py
+           unit-printprereleasesuffix.py \
+           unit-JarMaker.py \
+           $(NULL)

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

+        self.stream.writeln("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+
http://hg.mozilla.org/mozilla-central/rev/5cbe1e28ffb5, 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.
Status: NEW → RESOLVED
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+
http://hg.mozilla.org/releases/mozilla-1.9.2/pushloghtml?changeset=c399c316ae22, 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.