Closed Bug 505713 Opened 11 years ago Closed 10 years ago
Factor out multiple jar-file handling in Jar
3.83 KB, patch
|Details | Diff | Splinter Review|
9.81 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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.
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
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.
You need to log in before you can comment on or make changes to this bug.