de-xpfe and revive make-jars.pl to python

RESOLVED FIXED

Status

defect
RESOLVED FIXED
11 years ago
9 months ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

xpfe is dead, there's some cumbersome stuff in make-jars.pl that's not needed anymore. On the other hand, I'd like to make it do some tricks:

- not spawn processes for everything
- run multiple jars at once
- work on multiple source dirs.

The first two should help performance, the latter should help l10n merge and fork.

Forked this bug out of bug 361583, which remains to cover my hidden agenda to just use one script to create a language pack for l10n.

Tests for jar.mn coming up first. I'm not testing contents.rdf on purpose, we're just going to keep make-jars.pl around for folks that need that.

This tests the following of jar.mn processing:

Ouput formats jar, flat, and symlink (not ensuring the symlinks, though); not testing "both", OK to deprecate that?

Regular files, preprocessed files, preprocessed css files (for %), topsrcdir
files and l10n files.

Extending an existing manifest, not adding duplicate entries.

After calling into the regular realchrome target, I use diff to compare to a
reference. In the case of jar, I use a regular unzip to extract the jar again,
and then diff. The manifests need to be sorted before diffing, though.

I modified the hooks in config/Makefile.in a bit, so that we can run the
preprocessor tests independently from the jar.mn tests, if wanted, by default,
make check runs both.
Attachment #324950 - Flags: review?(ted.mielczarek)
Comment on attachment 324950 [details] [diff] [review]
tests for jar.mn, srcdir, topsrcdir,l10n,preprocessed, flat,jar,symlink

This looks good. You might need to stick tests/src-simple/Makefile in allmakefiles.sh, since you don't have it in DIRS or anything.
Attachment #324950 - Flags: review?(ted.mielczarek) → review+
New patch with the Makefile creation carried over, I had my own call into make-makefile, which would be sad if that changed.

Carrying over the review, per irc.
Attachment #324950 - Attachment is obsolete: true
Attachment #324965 - Flags: review+
Attachment #324988 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 324965 [details] [diff] [review]
moved tests/src-simple Makefile generation from config to allmakefiles.sh (landed)

Tests landed and stuck to the tree. Keeping open for the actual work.

http://hg.mozilla.org/mozilla-central/index.cgi/rev/459ccba9d923
Attachment #324965 - Attachment description: moved tests/src-simple Makefile generation from config to allmakefiles.sh → moved tests/src-simple Makefile generation from config to allmakefiles.sh (landed)
(In reply to comment #0)
> not testing "both", OK to deprecate that?
As I recall, "both" is a really simple combination of "zip" and "flat" - the difference being that "zip" temporarily copies the files to a temporary area while "both" zips up the "flat" copies. (Theoretically this should make for slightly faster depend builds although I prefer symlinks anyway.)

As for other suggestions, what about using hardlinks (either for flat chrome or for the temporary area used by zip chrome)?
The temporary stage for zips died, I'm directly updating the jar file without writing anything temporary to disk. Adding both would be a little awkward in the current code.

I really think that "both" is flawed, as the chrome manifest only supports one, so after you built, you have to manually edit the chrome manifest to actually pick up the content. I think that there are less awkward ways to achieve something similar, like just adding a flat for just one Makefile.in.

Re hardlinks, at least in python, hardlinks are not exposed on windows either, so we wouldn't have a platform win. I'm not sure where the win would be over symlinks on the unixish platforms.
In reply to m.d.platform post:
> Any wishes, suggestions?

It'd be great, if the new make-jars can be made as silent as possible. There is a patch with r+ from Benjamin in bug 433701 on this. It didn't make it into 3.0 release, but may still be of interest.
Adding a -q flag is a good idea. You probably want to get bug 433701 landed on the trunk aka mozilla-central before going in cvs, right?
(In reply to comment #8)
> Adding a -q flag is a good idea. You probably want to get bug 433701 landed on
> the trunk aka mozilla-central before going in cvs, right?

Sure, what should I do to achieve that?
Depends on: 433701
How is this to be used? 
http://mxr.mozilla.org/mozilla-central/source/config/tests/src-simple/

Is this part of generating installers that do not have missing entities?
It's a test for make-jars.pl and the rewrite. Nothing to do with installers or somesuch.
Posted patch add MozZipFile.py to config (obsolete) — Splinter Review
First patch of three. This patch is the actual zipwriter code that I use to write jars. The evolution is at http://hg.mozilla.org/users/axel_mozilla.com/index.cgi/build-patches/, which is the repo of my mq.

The major feature is that I can actually write the same file into an existing jar multiple times, which other libs don't support. I'm doing some optimizations, in particular when I'm storing a file that has the same size, I overwrite in place.

This comes with an extensive pseudo-fuzzing pseudo-unit test case, which expands to a total of 30k+ tests to be run. That's why I'm not adding it to the regular build. It's trying to write three files of different size in arbitrary order, close the zip, test it, and then write any of the three in any of three lengths to the same file, and test it. I already cut away some symmetries, but in the end, I didn't feel comfortable in reducing the complexity further, as there is a lot of potential for error that doesn't show up if you only write once, read once. Basically, it's trying to catch most situations that you'd see in depend builds.

This patch lays the ground for the following patch, JarMaker.py.patch.

Overall Style Question: I'm using intercaps for module names, I'm not sure if I should do so. Just tell me not to, and I'll change it, hg can do that :-).
Attachment #335892 - Flags: review?(ted.mielczarek)
Attachment #335892 - Flags: review?(dtownsend)
This patch adds the actual JarMaker.py. It includes changes to PreProcessor.py, as it's sharing the command line handling with that, and adds locks to MozZipFile.

Overall changes compared to make-jars.pl are:

- drop any support for installed-chrome.txt
- add support for writing both chrome.manifest and extension manifest files, though there is no commandline option for that yet
- less spaghetti code
- internalize PreProcessor
- share command line handling with PreProcessor

... which brings us to one of the more significant changes, the preprocessing is not done as a separate process anymore, but the JarMaker holds one PreProcessor that it clones for all child preprocessings. As the preprocessor is now integrated, it makes sense to actually make the two share one commandline, so -DFOO=bar is actually an argument to JarMaker.py, which is handled by the PreProcessor part of the OptionParser that is generated by PreProcessor and then extended by JarMaker.

I left all the unsupported options in so that we're not breaking on legacy build systems right from the start.
Attachment #335895 - Flags: review?(ted.mielczarek)
... and the easy part. Remove all options that are not needed anymore, and merge the two commandlines for preprocessor and jarmaker. I also don't pipe the output of the jar.mn preprocessing, but give it as an argument to jarmaker, which prepocesses the file internally.

The second-to-last incarnation of this code passed on the try server, I didn't push the latest argument removal and patch factorization. Tested the resulting builds from https://build.mozilla.org/tryserver-builds/2008-08-28_04:12-axel@mozilla.com-try-d14238c196b/ on windows and mac.
Attachment #335896 - Flags: review?(ted.mielczarek)
Comment on attachment 335892 [details] [diff] [review]
add MozZipFile.py to config

This looks good. There are a couple of things to fix and I want to see it again because I'm not convinced I have got my head round what cases you are testing.

>+      if ((zinfo.compress_type == zipfile.ZIP_STORED
>+           and zi.compress_size == len(bytes))
>+          or ((i + 1) == len(self.filelist)) and True):

Why |and True| ?

>+    zipfile.ZipFile.writestr(self, zinfo, bytes)
>+    self.filelist.sort(lambda l, r: cmp(l.header_offset, r.header_offset))

Seems like this is kinda inefficient. Presumably the new zipinfo is last in the filelist. And we know it should either be last or it should be at an earlier position which we worked out earlier, so can't we just move it there?

>+def mOutOfN(m, N, start=0):
>+  if m == 1:
>+    for k in xrange(start, N):
>+      yield [k]
>+    return
>+  for k in xrange(start, N - m + 1):
>+    for sub in mOutOfN(m - 1, N, start + 1):
>+      yield [k] + sub

This doesn't seem to ever be used.

>+  def tearDown(self):
>+    self.f = None
>+    self.ref = None

Should probably trash the self.stage directory here.

>+  def _verifyZip(self):
>+    zf = zipfile.ZipFile(self.f)
>+    badEntry = zf.testzip()
>+    self.failIf(badEntry, badEntry)
>+    zlist = zf.namelist()
>+    zlist.sort()
>+    vlist = self.ref.keys()
>+    vlist.sort()
>+    self.assertEqual(zlist, vlist)

Despite storing the expected content in self.ref and on disk you haven't verified that the zip contains the right data, just that it passes crc checks. I'd prefer to add the full data check unless there is a compelling reason not to.
Attachment #335892 - Flags: review?(dtownsend) → review-
(In reply to comment #15)
> (From update of attachment 335892 [details] [diff] [review])
> This looks good. There are a couple of things to fix and I want to see it again
> because I'm not convinced I have got my head round what cases you are testing.
> 
> >+      if ((zinfo.compress_type == zipfile.ZIP_STORED
> >+           and zi.compress_size == len(bytes))
> >+          or ((i + 1) == len(self.filelist)) and True):
> 
> Why |and True| ?

Removed.

> >+    zipfile.ZipFile.writestr(self, zinfo, bytes)
> >+    self.filelist.sort(lambda l, r: cmp(l.header_offset, r.header_offset))
> 
> Seems like this is kinda inefficient. Presumably the new zipinfo is last in the
> filelist. And we know it should either be last or it should be at an earlier
> position which we worked out earlier, so can't we just move it there?

I remember that I had something less brute-force initially, but it wasn't clever enough, so I started to run into problems on three or four writes. The list is mostly sorted, so it really shouldn't be that much of a hassle, so I'd rather like to keep this.

> >+def mOutOfN(m, N, start=0):
> >+  if m == 1:
> >+    for k in xrange(start, N):
> >+      yield [k]
> >+    return
> >+  for k in xrange(start, N - m + 1):
> >+    for sub in mOutOfN(m - 1, N, start + 1):
> >+      yield [k] + sub
> 
> This doesn't seem to ever be used.

Removed.

> >+  def tearDown(self):
> >+    self.f = None
> >+    self.ref = None
> 
> Should probably trash the self.stage directory here.

If I did, there'd be no way to debug on error, which is why I just keep it hanging around.

> >+  def _verifyZip(self):
> >+    zf = zipfile.ZipFile(self.f)
> >+    badEntry = zf.testzip()
> >+    self.failIf(badEntry, badEntry)
> >+    zlist = zf.namelist()
> >+    zlist.sort()
> >+    vlist = self.ref.keys()
> >+    vlist.sort()
> >+    self.assertEqual(zlist, vlist)
> 
> Despite storing the expected content in self.ref and on disk you haven't
> verified that the zip contains the right data, just that it passes crc checks.
> I'd prefer to add the full data check unless there is a compelling reason not
> to.

Fixed, didn't seem to impact the perf of the test run either.

New patch coming up.
Attachment #335892 - Attachment is obsolete: true
Attachment #336648 - Flags: review?(ted.mielczarek)
Attachment #336648 - Flags: review?(dtownsend)
Attachment #335892 - Flags: review?(ted.mielczarek)
Attachment #336648 - Flags: review?(dtownsend) → review+
Comment on attachment 335895 [details] [diff] [review]
The actual make-jars rewrite, JarMaker.py

+See the documentation for jar.mn on MDC for further details on the format.

Might make sense to include the actual URL to the documentation here.

+    # HACK, we need to unescape the string variables we get,
+    # the perl versions didn't grok strings right

Is this something you can fix in the future, once you've replaced the perl version entirely?

+    # backwards compat, not needed
+    p.add_option('-o', help="cross compile for auto-registration, ignored")
+    p.add_option('-z', help="backwards compat, ignored")
+    p.add_option('-p', help="backwards compat, ignored")

You have a couple of other options above that are ignored, can you move them all down here?

+  def finalize_jar(self, jarPath, chromebasepath, register,
+                   doZip=True):

Your method naming seems inconsistent. Can you standardize on camelCase?
 (looks like your argument names have the same problem, fwiw)

+    mode = {True: 'r+b', False: 'wb'}[os.access(manifestPath, os.F_OK)]

I think os.path.exists would be clearer here.

+    if mode == 'r+b':

I don't really like this that much (it doesn't read very clearly to me). Could you use a boolean with a more readable name instead?

Also, what happens here if someone removes an entry from a jar manifest? Does it stay until they clobber, since you read the existing content?

+      if os.access(jarfilepath, os.F_OK) and \

Again, os.path.exists.

From main():
+  print ' '.join(sys.argv)

This looks like debugging output. Just remove it?

I have a few more comments, but I need to reboot my system right now.
Attachment #336648 - Flags: review?(ted.mielczarek) → review+
Attachment #335895 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 335895 [details] [diff] [review]
The actual make-jars rewrite, JarMaker.py

>+++ b/config/JarMaker.py
>+  def processJarSection(self, jarfile, lines,
...
>+    while True:
>+      try:
>+        l = lines.next()
>+      except StopIteration:
>+        self.finalize_jar(jarfile, chromebasepath, register)
>+        if jf is not None:
>+          jf.close()
>+        return

This return feels sort of hidden. Could you just break out of the loop here instead?

Also, this method starts to feel very long here. Do you think you could refactor part of this into another method? Might improve the readability some.

>--- a/config/MozZipFile.py
>+++ b/config/MozZipFile.py
>@@ -47,11 +47,18 @@ class ZipFile(zipfile.ZipFile):
>   Subclassing zipfile.ZipFile to allow for overwriting of existing
>   entries, though only for writestr, not for write.
>   """
>-  def __init__(self, file, mode="r", compression=zipfile.ZIP_STORED):
>+  def __init__(self, file, mode="r", compression=zipfile.ZIP_STORED,
>+               lock = False):
>     zipfile.ZipFile.__init__(self, file, mode, compression)
>     self._remove = []
>     self.end = self.fp.tell()
>     self.debug = 0
>+    if lock:
>+      assert isinstance(file, basestring)
>+      from utils import lockFile
>+      self.lockfile = lockFile(file + '.lck')
>+    else:
>+      self.lockfile = None

Do you need to hide the import inside this if block? Why not just stick it at the top of the file unconditionally?

>+++ b/config/utils.py
...
>+class LockFile(object):

There's no builtin Python library to do this? I'm surprised!

...  
>+    # it's not been locked too long, wait a while and retry
>+    f.close()
>+    time.sleep(1)

Seems like you could check max_wait and maybe sleep a bit longer here. Also, your default is 600 seconds? That feels way too long. I would think more like 5 or 10 seconds would be plenty.

>+class pushback_iter(object):
>+  def next(self):
>+    try:
>+      return self.pushed_back.pop()
>+    except IndexError:
>+      return self.it.next()

Are exceptions that cheap in Python? Wouldn't it be faster to check len(self.pushed_back) > 0 instead?

Overall, looks good. Sorry, took me a while to get through this patch!
Attachment #335896 - Flags: review?(ted.mielczarek) → review+
Regarding the timeouts, I'd prefer to stay on the safer side here.

The hard part here is that we're not having a queue, that is, a single process can be blocked a few times by different other processes taking turns in locking and writing to that one file. So as long as we're not loading the system with the poll, I'd rather poll more often than not. And the timeout is really just to not get a system stuck forever on a lock file. Yeah, 10 minutes is really pessimistic, but given that this will fatally abort the build, I'm leaning towards trying long and often.
Comment on attachment 339310 [details] [diff] [review]
address most of Ted's comments

Carrying over ted's review.
Attachment #339310 - Flags: review+
Attachment #335895 - Attachment is obsolete: true
All trees are good apparently, resolving FIXED.

I have more things on my list, I'll file extra bugs on those.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
this broke the fennec builds:

make[6]: Entering directory `/home/dougt/builds/mozilla/mobilebase/xulrunner/netwerk/locales'
Traceback (most recent call last):
  File "/home/dougt/builds/mozilla/src/config/JarMaker.py", line 436, in ?
    main()
  File "/home/dougt/builds/mozilla/src/config/JarMaker.py", line 406, in main
    logging.basicConfig(level = noise, format = "%(message)s")
TypeError: basicConfig() takes no arguments (2 given)
make[6]: *** [libs] Error 1
make[6]: Leaving directory `/home/dougt/builds/mozilla/mobilebase/xulrunner/netwerk/locales'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/home/dougt/builds/mozilla/mobilebase/xulrunner/netwerk'
make[4]: *** [libs_tier_necko] Error 2
make[4]: Leaving directory `/home/dougt/builds/mozilla/mobilebase/xulrunner'
make[3]: *** [tier_necko] Error 2
make[3]: Leaving directory `/home/dougt/builds/mozilla/mobilebase/xulrunner'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/home/dougt/builds/mozilla/mobilebase/xulrunner'
make[1]: *** [build] Error 2
make[1]: Leaving directory `/home/dougt/builds/mozilla/src'
make: *** [build] Error 2
[sbox-CHINOOK-ARMEL-2007: ~/builds/mozilla/src] >
Is that bug 456134?
It's possible that this is a result of one of my patches, but I saw this exception (didn't stop the build) while testing something just now:
processing /Users/luser/build/mozilla-central/toolkit/content/jar.mn
Exception exceptions.AttributeError: "'NoneType' object has no attribute 'mode'" in <bound method ZipFile.__del__ of <MozZipFile.ZipFile instance at 0x3aa4e0>> ignored
Which version of python are you using? And which OS?
That's Python 2.5.2 on OS X 10.5.
I think I found it, though I'm not sure why, it's tough to reproduce this reliably. It seems that I close the jarfile twice in some sitation, and close wasn't handling that fine. python 2.3 backwards compat hack for bug 456134 is part of the patch, too.

Dirkjan, can you cross-check if I'm doing the version check right? basicConfig accepts keyword params only starting with version 2.4.
Attachment #339655 - Flags: review?(ted.mielczarek)
Attachment #339655 - Flags: review?(dirkjan)
Comment on attachment 339655 [details] [diff] [review]
close right, and python 2.3 compat (landed)

Looks sane to me.
Attachment #339655 - Flags: review?(dirkjan) → review+
Attachment #339655 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 339655 [details] [diff] [review]
close right, and python 2.3 compat (landed)

landed this follow-up, http://hg.mozilla.org/mozilla-central/rev/5b2f1103ce92
Attachment #339655 - Attachment description: close right, and python 2.3 compat → close right, and python 2.3 compat (landed)
Blocks: 456134
I'm currently working on switching the chatzilla build environment to use a python script in place of our current makexpi bash script.

However, chatzilla is still using the old mozilla CVS, is there any chance of having these changes pushed to CVS as well?

(The preprocessor python stuff is already on CVS, its just this stuff thats missing)
I doubt this will make it to CVS, but IMHO ChatZilla should rather go into its own hg.mozilla.org repository, like it was done for DOMi.
The Preprocessor.py stuff is already on CVS trunk, so what would be the issue with committing these updates to that and the make-jars replacement?

As for cZ being on Hg - thats quite a separate discussion. Its been discussed but I cant remember off-hand the reasons for not moving.
CVS trunk is the 1.9.0 stable branch. We will not be backporting this code to that branch.
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.