Closed
Bug 361583
Opened 18 years ago
Closed 11 years ago
l10n repacking should require a minimal set of tools
Categories
(Toolkit Graveyard :: Build Config, enhancement)
Toolkit Graveyard
Build Config
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Pike, Assigned: Pike)
References
Details
Attachments
(4 files, 7 obsolete files)
45.81 KB,
patch
|
benjamin
:
first-review+
|
Details | Diff | Splinter Review |
593 bytes,
patch
|
benjamin
:
first-review+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
Pike
:
first-review+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
benjamin
:
first-review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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•17 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•17 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•17 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•17 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•17 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•17 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•17 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•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
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•17 years ago
|
||
Attachment #252930 -
Attachment is obsolete: true
Assignee | ||
Comment 14•17 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)
Comment 15•17 years ago
|
||
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.
Comment 16•17 years ago
|
||
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•17 years ago
|
||
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•17 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•17 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 20•17 years ago
|
||
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•17 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•17 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•17 years ago
|
Attachment #261262 -
Flags: first-review?(benjamin) → first-review+
Assignee | ||
Comment 23•17 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•17 years ago
|
Attachment #261275 -
Flags: first-review+
Comment 24•17 years ago
|
||
Attachment #261282 -
Flags: first-review?(l10n)
Assignee | ||
Updated•17 years ago
|
Attachment #261282 -
Flags: first-review?(l10n) → first-review+
Comment 25•17 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•17 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•17 years ago
|
||
removing "-Fsubstitution" in line 249 of mozilla/toolkit/mozapps/installer/packager.mk unbreak packaging command under linux.
Comment 28•17 years ago
|
||
(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•17 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•17 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)
Updated•17 years ago
|
Attachment #261375 -
Flags: first-review?(benjamin) → first-review+
Comment 31•16 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•16 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.
Comment 33•16 years ago
|
||
(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•16 years ago
|
||
I think symlink output is important. I never really understood what "both" was for.
Assignee | ||
Comment 35•16 years ago
|
||
Ha, of course symlink is the hardest one to resurrect :-(, I'll give that a whirl.
Comment 36•16 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•16 years ago
|
||
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)
Comment 38•16 years ago
|
||
(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.
Comment 39•16 years ago
|
||
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•16 years ago
|
Attachment #324634 -
Attachment is obsolete: true
Attachment #324634 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 40•16 years ago
|
||
Filed bug 439050 for make-jars.pl.
Assignee | ||
Comment 41•11 years ago
|
||
most of this is done, marking FIXED.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•