l10n repacking should require a minimal set of tools

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
13 years ago
5 months ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Assignee)

Description

13 years ago
Currently, repackaging l10n requires a pretty complete build environment, which makes testing locales before submitting patches harder than absolutely necessary.

The list currently includes sh for configure, perl for makejars and preprocessor, and a compile environment to create nsinstall.

In addition, repackaging does a whole lot of things which are more time-consuming than necessary, i.e., doing a clobber build at least once, and I think twice, for each run. Improving that is hard with the separated toolchain we have right now.

I try to create a suite of python modules/tools that integrate nicely and only requires an installed python to run.

I'll need to at least recreate preprocessor.pl and make-jars.pl.

I already have some tests for preprocessor.pl which I tend to use to test for compatibility.
(Assignee)

Comment 1

13 years ago
Posted file Preprocessor.py (obsolete) —
This is the current state of the art for my python port of the Preprocessor.

I have a flat-chrome-ready port of make-jars.pl, too, and I tested this against browser/base so far.

The only difference I see are different formats of the line number directives in js files, the perl version inserts a ../../mozilla/browser or so, while my python code does not. And the order in browser.manifest is different.

I don't do installed-chrome.txt yet, do we still support that?

I still need to go through some code formatting for JarMaker.py, thus I'm going for a code review cycle on Preprocessor.py first, to reduce some cycles there.
Attachment #251193 - Flags: first-review?(benjamin)
(Assignee)

Comment 2

13 years ago
PS: I have some more test than attached in bug 277122 now, and the Preprocessor passes those, too. I incorporated the changed logic from bug 277122, too.

Comment 3

12 years ago
Comment on attachment 251193 [details]
Preprocessor.py

>"""
>Preprocess text files.
>"""

I think it would be nice to put the explanatory text that's currently in preprocessor.txt in this docstring.

>class State:
>  """States used for the StateMachine below"""

This confused me for a second... you should probably indicate that this is an enumeration type.

>class Token:
>  """Tokens to be used for lexing expressions"""
>  IDENT,              \
>         NUMBER,      \

nit: whitespace

>class Evaluator:
>  """
>  Abstract base class for all operations for evaluating expressions.
>  """
>  def eval(self):
>    raise RuntimeError('override this')
>class Value(Evaluator):
>  """
>  Simple value subclass.
>  """
>  def __init__(self, value):
>    self.value = value
>  def eval(self):
>    return self.value
>import operator

nits: please add a newline between classes. Please move this import statement to the top (unless there's a reason it needs to be here).

>class Preprocessor:

>  def __init__(self):

I think it would be nice for the init method to take keyword arguments for lineEndings, marker, etc.

>  def handleCommandLine(self, args, defaultToStdin = False):

Why is this a class method? I think it's more idiomatic to put this logic down in main(), becuase it is peculiar to command-line usage of this tool

>    """
>    Parse a commandline into this parser.
>    Uses OptionParser internally, no args mean sys.args[1:].
>    """

you mean sys.argv

>    includes = []
>    def handleI(option, opt, value, parser):
>      includes.append(value)

Can't you just use action="append"?

>    def handleE(option, opt, value, parser):
>      for k,v in os.environ.iteritems():
>        self.engine.setVariable(k, v)

Perhaps there should be an engine.update() method?

It would be kinda nice if the Preprocessor class was itself a file-like object that you could write to, but perhaps that can wait for a new bug, since it's not necessary for this task.

With those changes, r=me... but I'd like a patch with nits fixed to be reviewed by a python guru... sayrer, bhearsum, rhelmer probably all qualify.
Attachment #251193 - Flags: first-review?(benjamin) → first-review+

Comment 4

12 years ago
>    if self.op(self.leftside.eval(), self.argument.eval()):
>      return 1
>    return 0

Could just use |return self.op(self.leftside.eval(), self.argument.eval())|

This would return a bool, which subclasses int with True==1, False==0.

>    if self.argument.eval():
>      return 0
>    return 1

|return not self.argument.eval()|

>           if not self.transitions.has_key(state):

If you replaced these |has_key|s with |key in dict| use |key not in dict| instead of |not key in dict|

>    self.write(reduce(lambda x, y: x+y, lst, ''))

This is |self.write(''.join(lst))|

>    filterNames = current.keys()
>    filterNames.sort()
>    self.filters = [(fn, current[fn]) for fn in filterNames]
>    return

This is |self.filters = sorted(current.items())|.

Also here and in other places the return is not needed.

>  def filter_slashslash(self, aLine):
>    [aLine, rest] = aLine.split('//', 1)

The [] around aLine, rest aren't necessary but this doesn't change anything.
(Assignee)

Comment 5

12 years ago
(In reply to comment #3)
> >class Preprocessor:
> 
> >  def __init__(self):
> 
> I think it would be nice for the init method to take keyword arguments for
> lineEndings, marker, etc.

I actually change this on the same preprocessor every now and then.

> >  def handleCommandLine(self, args, defaultToStdin = False):
> 
> Why is this a class method? I think it's more idiomatic to put this logic down
> in main(), becuase it is peculiar to command-line usage of this tool

Actually, this is factored already to be used by JarMaker.py, which takes a bunch of arguments for the preprocessor. It used to be in main before, but now I have a separate method for it.

As I use a single Preprocessor object to parse the commandline and handle includes and after that just clone that mother processor, having a mean of setting the marker makes sense. 

I'm working on the rest of the nits.

I'm also incorporating some comments by Marc, in particular, I already removed the has_key function calls, and I'll probably move the _v to a real public variables property on Engine, so that we can do a more python-ish 
if aVar in self.engine.variables:

Re Marc's comment 4, I did the key not in dict, too, thanks for the pointer.

I won't do sorted(), as that's new in 2.4, and we want to work on 2.3. The last comment didn't work for me on 2.4 either,

>>> aLine = 'foo//bar'
>>> [tmp] = aLine.split('//',1)
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
ValueError: too many values to unpack

I'm happy to see someone with some python skills look at this stuff, though, because that's one main reason why I started attaching one chunk first.

Comment 6

12 years ago
I meant to use:

  aLine, rest = aLine.split('//', 1)

>    filterNames = current.keys()
>    filterNames.sort()
>    self.filters = [(fn, current[fn]) for fn in filterNames]

To avoid sorted you could instead write:

  self.filters = list(current.items())
  self.filters.sort()
(Assignee)

Comment 7

12 years ago
Is it good-enough python to overload __contains__ to only return true if it a variable is defined, but to overload __getitem__ to return the key value without raising an IndexError?

http://docs.python.org/ref/sequence-types.html#l2h-223 says "should", and it'd make for some probably cute code, but maybe only in my eyes.

Comment 8

12 years ago
If you're trying to act like a dictionary then __getitem__ wouldn't raise an IndexError anyway. IndexError is only for sequences to show that there is no item at that index.

If you derive your class from a dict then all of these methods will be automagically defined for you.
(Assignee)

Comment 9

12 years ago
Oops, make that a KeyError.

>>> foo={'hi':'bar'}
>>> foo['ho']
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
KeyError: 'ho'

is what a regular dict does, but what the preprocessor needs is that

foo['ho'] yields 'ho'

Thus I'd overwrite __getitem__ to 

if not key in self:
  return key
return super(Variables, self).__getitem__(self, key);
(Assignee)

Comment 10

12 years ago
Posted file Expression.py (obsolete) —
I have been looking into parsers and stuff quite heavily in the past few days and I grew to dislike lexing and parsing in separate steps, and I'm not so sure that that statemachine will actually be easy to maintain.

I haven't done some testing, but this is expected to work with the slightly twisted context I layed out in the previous comments. Which I didn't code yet.

Anyway, I'm curious if this contextual grammar based approach would be less brain-hurting than the statemachinery I did in Preprocessor.py.
Attachment #252930 - Flags: first-review?(benjamin)

Comment 11

12 years ago
Comment on attachment 252930 [details]
Expression.py

I don't understand this very well, but I'm ok with that. But I do want unit tests at the bottom of this file in the standard "if __name__ == '__main__'" way.
Attachment #252930 - Flags: first-review?(benjamin) → first-review-
(Assignee)

Comment 12

12 years ago
Posted file test Expression.py (obsolete) —
I'd prefer to put the tests into a separate file, as I experienced some oddness when trying to use Expression as a module compared to using it internally.

How does this test look?

I'll attach the updated Expression.py that includes the Context object in a second.

And we still need a place where this should end up in the tree.
(Assignee)

Comment 13

12 years ago
Posted file Expression.py with Context (obsolete) —
Attachment #252930 - Attachment is obsolete: true
(Assignee)

Comment 14

12 years ago
Looking at a (faked negative) run of a port to unittest:

$ python unit-Expression.py && echo 'hi'
.....F..
======================================================================
FAIL: test the != operator
----------------------------------------------------------------------
Traceback (most recent call last):
  File "unit-Expression.py", line 48, in test_notequals
    self.assert_(Expression('FAIL == 1').evaluate(self.c))
AssertionError

----------------------------------------------------------------------
Ran 8 tests in 0.003s

FAILED (failures=1)


I'd interpret that output to satisfy our requirements on unit tests layed out on http://developer.mozilla.org/en/docs/How_to_add_a_build-time_test.

Would you agree? Then I'd base my tests on python's unittest module (and add that unittest is OK to the MDC page)
I agree, Pike. unittest.py is a very decent implementation of xUnit for Python and should be adequate for testing the l10n content before repack.

Updated

12 years ago
Depends on: 372400
In Preprocessor.py you're using the 'pie-decorator" syntax with @classmethod for the Token.get(...) method. That syntax was introduced in Python 2.4 (PEP 318), so I would think that if 2.3 is an included target, you'd need to change it and move the transformation to after the function body.

That is, removing the |@classmethod| line and adding |get = classmethod(get)| immidiately after the function body (on the same indentation level as the method def's).

Anyway, just something I noticed...
(Assignee)

Comment 17

12 years ago
Posted file testing string value (obsolete) —
Fredrik, that decorator code is gone already, but thanks for looking.

Benjamin, I found an inconsistency in preprocessor.pl with cpp, which is biting me right now. In cpp, #if takes a constant integer expression, that is, string values are false. In preprocessor.pl, they're true. I'd like to change that, as it will make a systematic implementation of 
#if FOO
much easier, and is likely going to be less surprising when we extend the grammar of constant expressions. I just attached a testcase for this.

If I change that, should I make a separate  patch to preprocessor.pl to land first?

This is the current state of my tree and the migration plan:

Replace all uses of preprocessor.pl for building a Firefox. I'm not touching mail, suite, nor repackaging of l10n yet. Nor do I touch jar packaging, as that's tied to perl in make-jars.pl, wich I'd rather not change to begin with.

Once that patch sticks to the tree, I'd attack jar generation next, or mail. I'd leave suite up for the SeaMonkey folks.

Does that sound like a plan?

I need to investigate some line ending issues with MozillaBuilds python, too. all.js has one 0a in the source, and ends up with 0d0d0d in the output. Didn't look in detail yet.
(Assignee)

Comment 18

12 years ago
To paste Benjamins reply here, I'm not going to fix preprocessor.pl, and just have to make sure that what I end up with works.

Right now I'm testing trunk builds, unpatched vs patched, by diffing the output. More bugs in preprocessor.pl to be found, #expand doesn't emit js line number comments. That's the only substantial difference I found so far, I'm still going through the diff, which is mostly filled with slightly different paths in js files.
(Assignee)

Comment 19

12 years ago
Here goes my first real patch for this bug.

I did an lxr search for preprocessor.pl, and changed all the usecases for unofficial Minefields. I didn't touch make-jars.pl yet nor browser/locales, browser/installer, mail, extensions, xpfe, calendar, xulrunner, themes.

I added two unit test sets, one for the Expression.py class, which handles the evaluation of expressions, and one for Preprocessor.py, which does the actual processing.

I made a debug build on windows with mozillabuild, python 2.5 and diffed dist/bin, same thing on the mac with python 2.3, and did some basic testing on the mac.

First review goes over to Benjamin for the general setup and build foo, all watchers with python skills are invited to comment, as always.
Attachment #251193 - Attachment is obsolete: true
Attachment #253624 - Attachment is obsolete: true
Attachment #253625 - Attachment is obsolete: true
Attachment #260462 - Attachment is obsolete: true
Attachment #260709 - Flags: first-review?(benjamin)
Comment on attachment 260709 [details] [diff] [review]
first pass, simple usecases for Preprocessor.py, itself, tests

>Index: config/tests/unit-Preprocessor.py

>+    f = NamedIO("conditional_if_0.in", """#if 0
>+FAIL
>+#else
>+PASS
>+#endif
>+""")

Having the first line of the test string horizontally separated makes reading it hard.  The following style is equivalent but much more readable:

>+    f = NamedIO("conditional_if_0.in", """\
>+#if 0
>+FAIL
>+#else
>+PASS
>+#endif
>+""")

Comment 21

12 years ago
Comment on attachment 260709 [details] [diff] [review]
first pass, simple usecases for Preprocessor.py, itself, tests

http://lxr.mozilla.org/mozilla/source/intl/uconv/src/nsUNIXCharset.cpp#244 uses OSARCH and expects it to be quoted.

Other than that, this appears fine.
Attachment #260709 - Flags: first-review?(benjamin) → first-review-
(Assignee)

Comment 22

12 years ago
I fixed nsUNIXCharset.cpp by using NS_STRINGIFY from nscore.h. I actually changed Append to AppendLiteral, too, as I changed that line. Going through the preprocessor on my mac, the outcome is

      propertyFile.AssignLiteral("unixcharset.");
      propertyFile.AppendLiteral("Darwin");
      propertyFile.AppendLiteral(".properties");

which should be fine. I verified with Benjamin that we do use cygwin wrapper for PYTHON, too, so windows on cygwin should be fine, too.
Attachment #260709 - Attachment is obsolete: true
Attachment #261262 - Flags: first-review?(benjamin)

Updated

12 years ago
Attachment #261262 - Flags: first-review?(benjamin) → first-review+
(Assignee)

Comment 23

12 years ago
This is a bustage fix patch, we should use cygwin wrapper for python only for non-cygwin pythons, still need to find a good way to check that.

Updated

12 years ago
Attachment #261275 - Flags: first-review+

Comment 24

12 years ago
Attachment #261282 - Flags: first-review?(l10n)
(Assignee)

Updated

12 years ago
Attachment #261282 - Flags: first-review?(l10n) → first-review+

Comment 25

12 years ago
I don't know if it is related, but, after a successful build process, when I enter make -C browser/installer, I have :

"/usr/bin/python ../../config/Preprocessor.py -Fsubstitution -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DBUILD_ID=2007041207 -DAB_CD=en-US -DMOZ_APP_NAME=firefox -DDLL_PREFIX=lib -DDLL_SUFFIX=.so -DHAVE_64BIT_OS=1 -DMOZILLA_VERSION=\"1.9a4pre\" -DMOZILLA_VERSION_U=1.9a4pre -DD_INO=d_ino -DSTDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_INT16_T=1 -DHAVE_INT32_T=1 -DHAVE_INT64_T=1 -DHAVE_UINT=1 -DHAVE_UNAME_DOMAINNAME_FIELD=1 -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_ATTRIBUTE=1 -DHAVE_DIRENT_H=1 -DHAVE_GETOPT_H=1 -DHAVE_SYS_BITYPES_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_GNU_LIBC_VERSION_H=1 -DHAVE_NL_TYPES_H=1 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATFS_H=1 -DHAVE_MMINTRIN_H=1 -DHAVE_SYS_CDEFS_H=1 -DHAVE_LIBM=1 -DHAVE_LIBDL=1 -DFUNCPROTO=15 -DHAVE_XSHM=1 -D_REENTRANT=1 -DHAVE_RANDOM=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_STAT64=1 -DHAVE_LSTAT64=1 -DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -DHAVE_STRTOK_R=1 -DHAVE_RES_NINIT=1 -DHAVE_GNU_GET_LIBC_VERSION=1 -DHAVE_LANGINFO_CODESET=1 -DVA_COPY=va_copy -DHAVE_VA_COPY=1 -DHAVE_VA_LIST_AS_ARRAY=1 -DHAVE_I18N_LC_MESSAGES=1 -DMOZ_EMBEDDING_LEVEL_DEFAULT=1 -DMOZ_EMBEDDING_LEVEL_BASIC=1 -DMOZ_EMBEDDING_LEVEL_MINIMAL=1 -DMOZ_PHOENIX=1 -DMOZ_BUILD_APP=browser -DMOZ_XUL_APP=1 -DMOZ_DEFAULT_TOOLKIT=\"cairo-gtk2\" -DMOZ_WIDGET_GTK2=1 -DMOZ_ENABLE_XREMOTE=1 -DMOZ_THEBES=1 -DMOZ_CAIRO_GFX=1 -DMOZ_X11=1 -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DMOZ_ENABLE_XFT=1 -DMOZ_ENABLE_PANGO=1 -DMOZ_ENABLE_COREXFONTS=1 -DMOZ_EXTRA_X11CONVERTERS=1 -DOJI=1 -DMOZ_ENABLE_XINERAMA=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_NO_XPCOM_OBSOLETE=1 -DMOZ_XTF=1 -DMOZ_MATHML=1 -DMOZ_ENABLE_CANVAS=1 -DMOZ_SVG=1 -DMOZ_SVG_FOREIGNOBJECT=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_PLACES=1 -DMOZ_FEEDS=1 -DMOZ_STORAGE=1 -DMOZ_SAFE_BROWSING=1 -DMOZ_URL_CLASSIFIER=1 -DMOZ_LOGGING=1 -DHAVE___CXA_DEMANGLE=1 -DMOZ_USER_DIR=\".mozilla\" -DMOZ_STATIC_BUILD=1 -DHAVE_STDINT_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_UINT64_T=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_RDF=1 -DMOZ_MORKREADER=1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNIX=1 -DUNIX_ASYNC_DNS=1 -DJS_THREADSAFE=1 -DMOZ_ACCESSIBILITY_ATK=1 -DATK_MAJOR_VERSION=1 -DATK_MINOR_VERSION=18 -DATK_REV_VERSION=0 -DMOZILLA_LOCALE_VERSION=\"1.9a1\" -DMOZILLA_REGION_VERSION=\"1.9a1\" -DMOZILLA_SKIN_VERSION=\"1.8\"  ./removed-files.in > removed-files
Usage: Preprocessor.py [options]

Preprocessor.py: error: no such option: -F
make[1]: *** [removed-files] Error 2"

And I have no created .tar.bz2 file :(

Comment 26

12 years ago
Sorry to spam this bug, but replacing :

$(PYTHON) $(topsrcdir)/config/Preprocessor.py -Fsubstitution $(DEFINES) $(AC
DEFINES) $(MOZ_PKG_REMOVALS) > $(MOZ_PKG_REMOVALS_GEN)

by

$(PYTHON) $(topsrcdir)/config/Preprocessor.py -Fsubstitution $(DEFINES) $(AC
DEFINES) $(MOZ_PKG_REMOVALS) > $(MOZ_PKG_REMOVALS_GEN)

in mozilla/toolkit/mozapps/installer/packager.mk could fix this bug ?!

Comment 27

12 years ago
removing "-Fsubstitution" in line 249 of mozilla/toolkit/mozapps/installer/packager.mk unbreak packaging command under linux.
(In reply to comment #27)
> removing "-Fsubstitution" in line 249 of
> mozilla/toolkit/mozapps/installer/packager.mk unbreak packaging command under
> linux.
> 

The packager.mk change has caused various red & orange on tinderbox builds now they have started doing nightly releases (i.e. packaging it):

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1176376680.24930.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1176374460.27620.gz

As no one was around on irc to help, I wasn't sure if the suggested fix is all that is required or not, so as preprocessor.pl is still present I backed out the change to mozilla/toolkit/mozapps/installer/packager.mk to try and fix the trees.

Comment 29

12 years ago
(In reply to comment #1)
> I don't do installed-chrome.txt yet, do we still support that?

We can drop that for all apps that have MOZ_XUL_APP=1 set (that unfortunately means that currently SeaMonkey needs it still, but as long as the .pl is still available until we switch the suite to the toolkit-based variant, we should be fine, I hope).
(Assignee)

Comment 30

12 years ago
Thanks to Mark for backing out, I need to actually add that command line argument.

Sorry for not triggering this in my tests.

The fix is trivial though. Once I get the review, I'd land the change to packager.mk back.
Attachment #261375 - Flags: first-review?(benjamin)
Blocks: 377308

Updated

12 years ago
Attachment #261375 - Flags: first-review?(benjamin) → first-review+

Comment 31

11 years ago
Comment on attachment 261375 [details] [diff] [review]
duh, man, add commandline argument

>+    p.add_option('-F', action='callback', callback=handleF, type="string",
>+                 metavar="FILTER", help='Enabble the specified filter')
                                               ^^ You got one too many "b"s there
(Assignee)

Comment 32

11 years ago
Ted, Benjamin, I have a mq at http://hg.mozilla.org/users/axel_mozilla.com/index.cgi/build-patches/file/tip with patches, and I wonder how to proceed on those.

The queue is currently 
http://hg.mozilla.org/users/axel_mozilla.com/index.cgi/build-patches/file/370020c92726/MozZipFile.py.patch, extending the standard zipfile by code to overwrite existing entries. It has a pretty extensive test suite generator, though I don't know if it's complete. I don't test things like mixed compression, on purpose though.

On top of that is a mostly naive rewrite of make-jars.pl in python. http://hg.mozilla.org/users/axel_mozilla.com/index.cgi/build-patches/file/370020c92726/JarMaker.py.patch is the patch. There is some limited testing outside of this repo, in the end, there's nothing better than to run two builds and compare. There are a few things that I'd like to do on top, one of which is to factor the code better, the other is to add fallback for multiple code locations, at least for locales so that we can hook up l10n merge and fork properly, and pick up files from en-US as last resort, too. Good refactoring should enable processing more than one jar.mn at once, too.

The current code is about twice as fast as make-jars.pl when doing a make -C browser chrome.

My question would be, how do we want to sheppard this in, one big patch here, dependent bugs per patch, including refactoring or doing that later?

Also, are all hystoric features of make-jars.pl worth keeping? force platform and x11? "symlink" and "both" output?

Note to self, foreignPlatform is missing, we probably should keep that.
(In reply to comment #32)
> The current code is about twice as fast as make-jars.pl when doing a make -C
> browser chrome.

This is awesome!

> 
> My question would be, how do we want to sheppard this in, one big patch here,
> dependent bugs per patch, including refactoring or doing that later?
> 

I think I'd like to do:
1) Your make-jars replacement + the zip lib it depends on as one patch
2) The build-system hookup as a second patch
3) Any refactoring separately (before or after, as you see fit)


> Also, are all hystoric features of make-jars.pl worth keeping? force platform
> and x11? "symlink" and "both" output?

I think anything not currently used in the tree can be unsupported, as long as you update any relevant docs as well. I don't think you should bear the burden of re-implementing unused features.

As far as testing, as long as you have a comprehensive test suite (by your definition) to prevent regressions, and you can diff the results against the existing make-jars to prove initial correctness, that's enough for me.

Comment 34

11 years ago
I think symlink output is important. I never really understood what "both" was for.
(Assignee)

Comment 35

11 years ago
Ha, of course symlink is the hardest one to resurrect :-(, I'll give that a whirl.

Comment 36

11 years ago
(In reply to comment #34)
> I never really understood what "both" was for.

FYI, I'm using it for having jar-packaged chrome usually but being able to fast-test changes by hacking the plain files and only adjusting the manifest line(s) for the affected components.
(Assignee)

Comment 37

11 years ago
Posted patch add a simple jar test first (obsolete) — Splinter Review
Before going into the details of the actual replacements, I thought we should just have tests.

This tests the following of jar.mn processing:

Ouput formats jar, flat, and symlink (not ensuring the symlinks, though)

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 #324634 - Flags: review?(ted.mielczarek)
(In reply to comment #37)
> 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.

I don't see this in your patch, did it get left out? I like the rest of this patch though, get me a full one and we'll get it in.


Also, I think maybe you should split this out into a separate bug. You can do one bug just for your whole make-jars replacement, but this bug is kind of cluttered.
(Assignee)

Updated

11 years ago
Attachment #324634 - Attachment is obsolete: true
Attachment #324634 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

11 years ago
Depends on: 439050
(Assignee)

Comment 40

11 years ago
Filed bug 439050 for make-jars.pl.

Updated

10 years ago
Depends on: 483432
(Assignee)

Comment 41

6 years ago
most of this is done, marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 months ago
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.