476733 - WinCE Dynamic CAB INF File Production Needed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[John Wolfe (:wolfe)]

Right now, in the mobile-browser repository, there are two hard-coded files - a "make-cab-package" script and a "fennec.inf" INF file.

These two files have within them a list of the files that are placed into the mobile dist installation source directory.

These two files should be dynamically generated from a pre-created directory structure.  This will require a python script to walk the directory collecting names and looking for name conflicts.  When conflicts arise, one of the conflicting files needs to be renamed in the make-cab-package script.  The newly-renamed filename needs to be used within the INF file in a special way (search for the string "_2" within the exisiting hard-coded INF file for examples).

Comment 1

[John Wolfe (:wolfe)]

Attachment: v1.0 patch for mozilla-central

This is a follow-up bug for creating a WinCE Installer.

This patch changes the calling sequence for the mobile-browser CAB file creation.

Together with the patch for mobile-browser, a WinMobile INF file is now dynamically created from a directory which is passed to a (perhaps-very-ugly) python script which steps through the directory, picking out and renaming/copying duplicate files, outputting a CAB file, running the CABWIZ.exe program to produce a compressed CAB file, renaming the produced CAB file to its long and slightly cryptic final name, then deleting all the copied files.

Comment 2

[John Wolfe (:wolfe)]

Attachment: v1.0 patch for mobile-browser repository

This is a follow-up bug for creating a WinCE Installer.

This patch works together with a patch for mozilla-central which changes the calling sequence for the mobile-browser CAB file creation script.

This patch for mobile-browser does the following: A WinMobile INF file is now dynamically created from a directory which is passed to a (perhaps-very-ugly) python script. 

The python script steps through a passed-in directory, picking out and renaming/copying duplicate files, outputting a CAB file, running the CABWIZ.exe program to produce a compressed CAB file, renaming the produced CAB file to its long and slightly cryptic final name, then deleting all the copied files.

This script is dynamic INF file creation, and relies on the application directory name being the same base name as the executable inside the application directory.

For instance, mobile-browser builds a fennec directory in OBJDIR/dist that contains a fennec.exe executable.  This executable becomes the target of the installation CAB's created shortcut.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 363635 [details] [diff] [review]
v1.0 patch for mozilla-central

+#MAKE_PACKAGE = $(MOZ_PKG_CAB_SCRIPT) "$(VSINSTALLDIR)" "$(topsrcdir)" "$(MOZ_PKG_DIR)" "$(MOZ_PKG_CAB_INF)" "$(MOZ_APP_NAME)" "$(PACKAGE)"

Please don't leave commented-out lines laying around.

Also, I was expecting that the python script would live in mozilla-central, and it would read some plain-text file as input (or scan the objdir, whatever works). This does not appear to be the case?

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 363636 [details] [diff] [review]
v1.0 patch for mobile-browser repository

r-, but mostly cause I expect this script to live in m-c.

Generally with Python scripts in our tree we like to have everything inside a main() method, like so:

def main():
  # do all your stuff here

if __name__ == "__main__":
    main()

The theory here is that you could import the script as a module and use it that way also. You can see an example at:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py

+# EXAMPLE OF COMMAND LINE:
+#   python make_wince_inf.py fennec Fennec /c/Program\ Files/Microsoft\ Visual\ Studio\ 9.0/ fennec_testing.cab
+#
+#   python make_wince_inf.py /c/hg/mozilla-central/objdir-wm6-dbg/mobile/dist fennec Fennec /c/Program\ Files/Microsoft\ Visual\ Studio\ 9.0/ fennec-0.11.en-US.wince-arm.cab


Your example commandlines don't seem to jibe with the variables you set below. Can you make them match?

I don't understand why your script is crawling dist/. Surely you want to crawl dist/fennec/, since that's the staged directory you're going to package?

+def read_exceptions(filename):
+    if (os.path.exists(filename)):
+        f = file(filename)
+        exceptions={}
...
+        return exceptions
+    else:
+        return {}

I'd write this as:
def read_exceptions(filename):
    exceptions = {}
    if os.path.exists(filename):
      ...
    return exceptions

Also note that parentheses aren't required with if expressions, you can just say |if foo:|

+    copy_command = "cp %s %s" % (old_relative_filename, new_relative_filename)
+    #print >> sys.stdout, "os.system(%s)" % copy_command
+    os.system(copy_command)

Use shutil.copyfile:
http://docs.python.org/library/shutil.html

+    print >> f, "; Here are a list of the currently duplicated filenames as of 20-Feb-09:\r"

Uh, this seems unnecessary. Also, I'd prefer f.write("string\r\n")

Note that you can also do multi-line strings in Python with triple quotes:
print """A
Multi
Line
String"""

+    print >> sys.stdout, "CABWIZ_PATH=[%s]" % cabwiz_path

print "foo" goes to stdout by default.

+    os.system(make_cab_command)

Just use subprocess.call here, it can take an array of [program, arg, arg], like:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#607

+    rename_cab_command = "rename %s.CAB %s" % (program_name, cab_final_name)

Use shutil.move.

+            delete_command = "rm -f %s" % new_relative_filename

os.unlink:
http://www.python.org/doc/2.5.2/lib/os-file-dir.html

+sys.exit(0)
+

Extraneous, Python will exit with a zero status if you don't sys.exit() with a different value. (Or have an uncaught exception.)

Comment 5

[John Wolfe (:wolfe)]

(In reply to comment #4)
> (From update of attachment 363636 [details] [diff] [review])
> r-, but mostly cause I expect this script to live in m-c.

Moved python script for dynamically generating INF file from a pre-built directory structure.


> Generally with Python scripts in our tree we like to have everything inside a
> main() method, like so:
> 
> def main():
>   # do all your stuff here
> 
> if __name__ == "__main__":
>     main()
> 
> The theory here is that you could import the script as a module and use it that
> way also. You can see an example at:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py

DONE


> +# EXAMPLE OF COMMAND LINE:
> +#   python make_wince_inf.py fennec Fennec /c/Program\ Files/Microsoft\
> Visual\ Studio\ 9.0/ fennec_testing.cab
> +#
> +#   python make_wince_inf.py /c/hg/mozilla-central/objdir-wm6-dbg/mobile/dist
> fennec Fennec /c/Program\ Files/Microsoft\ Visual\ Studio\ 9.0/
> fennec-0.11.en-US.wince-arm.cab
> 
> 
> Your example commandlines don't seem to jibe with the variables you set below.
> Can you make them match?

Yup - the comments did not match any incoming command line.  DONE. 


> I don't understand why your script is crawling dist/. Surely you want to crawl
> dist/fennec/, since that's the staged directory you're going to package?

The script is actually crawling dist/fennec - via the incoming source_dir argument (set to "fennec" for the WinMobile mobile-browser build.


> +def read_exceptions(filename):
> +    if (os.path.exists(filename)):
> +        f = file(filename)
> +        exceptions={}
> ...
> +        return exceptions
> +    else:
> +        return {}
> 
> I'd write this as:
> def read_exceptions(filename):
>     exceptions = {}
>     if os.path.exists(filename):
>       ...
>     return exceptions
> 
> Also note that parentheses aren't required with if expressions, you can just
> say |if foo:|

DONE.  That's what I get for reusing other people's code.  This function was left over from the original python script upon which I modeled this script, js/src/config/check-sync-dirs.py


> +    copy_command = "cp %s %s" % (old_relative_filename, new_relative_filename)
> +    #print >> sys.stdout, "os.system(%s)" % copy_command
> +    os.system(copy_command)
> 
> Use shutil.copyfile:
> http://docs.python.org/library/shutil.html

DONE - much better, thank you.


> +    print >> f, "; Here are a list of the currently duplicated filenames as of
> 20-Feb-09:\r"
> 
> Uh, this seems unnecessary. Also, I'd prefer f.write("string\r\n")

DONE


> Note that you can also do multi-line strings in Python with triple quotes:
> print """A
> Multi
> Line
> String"""

Multi-line strings and argument substitution do not seem to like each other.

I tried multi-line strings, and for the little file-size it saves, the confusion over what is actually written out was too big.  I switched back to using one f.write() per line.

> +    print >> sys.stdout, "CABWIZ_PATH=[%s]" % cabwiz_path
> 
> print "foo" goes to stdout by default.

Cleaned up all STDOUT print statements.


> +    os.system(make_cab_command)
> 
> Just use subprocess.call here, it can take an array of [program, arg, arg],
> like:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#607

DONE.


> +    rename_cab_command = "rename %s.CAB %s" % (program_name, cab_final_name)
> 
> Use shutil.move.

DONE.


> +            delete_command = "rm -f %s" % new_relative_filename
> 
> os.unlink:
> http://www.python.org/doc/2.5.2/lib/os-file-dir.html

Ended up using os.remove() function - since remove() is much closer to the Windows way of doing things than unlink() - since both unlink() and remove() are aliases for the same code.


> +sys.exit(0)
> +
> 
> Extraneous, Python will exit with a zero status if you don't sys.exit() with a
> different value. (Or have an uncaught exception.)

REMOVED.


I have two new patches:

* one for mobile-browser that only removes the unneeded installer/wince directory items and removes the additions into the installer/Makefile.in file.

* One for mozilla-central which adds the new make_wince_cab.py into a new subdirectory build/package/wince, as well as cleans up the toolkit/mozapps/installer/packager.mk file, removing all references to MOZ_PKG_CAB_SCRIPT and hard-codes the build/package/wince/make_wince_cab.py python script.

Comment 6

[John Wolfe (:wolfe)]

Attachment: v2.0 of mozilla-central patch

See previous (long) comment #5 for explanation of this patch, which incorporates Ted's comments and requests.

Comment 7

[John Wolfe (:wolfe)]

Attachment: v2.0 patch for mobile-browser repository

See previous (long) comment #5 for explanation of this patch, which incorporates Ted's comments and requests.

Comment 8

[John Wolfe (:wolfe)]

Attachment: v3.0 of mozilla-central patch

Creates ZIP file on |make package| command, while creating a new |make installer| command which produces a CAB file for WINCE builds.

Very small changes from v2.0 patch.

Comment 9

[John Wolfe (:wolfe)]

Attachment: v3.0 patch for mobile-browser repository

Creates ZIP file on |make package| command, while creating a new |make installer| command which produces a CAB file for WINCE builds.

Very small changes from v2.0 patch.

Comment 10

[John Wolfe (:wolfe)]

Attachment: v4.0 of mozilla-central patch

Very similar to previous patches, except establishes ZIP as default package format for WINCE, and removes support for CAB format packaging.

Goes together with associated patch for mobile-browser which creates a top-level OBJDIR |make installer| command that creates a CAB file for WINCE builds, and a DEB file for Linux builds.

Comment 11

[John Wolfe (:wolfe)]

Attachment: v4.0 patch for mobile-browser repository

Very similar to previous patches, except creates a top-level OBJDIR |make installer| command that creates a CAB file for WINCE builds, and a DEB file for Linux builds.  

Also creates |make installer| command inside installer directory which does the actual work of producing CAB file for WINCE builds and DEB file for Linux builds.

Goes together with associated patch for mozilla-central which establishes ZIP as default package format for WINCE, and removes support for CAB format packaging.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 364534 [details] [diff] [review]
v4.0 patch for mobile-browser repository

+deb: installer
 endif
+
+ifeq ($(OS_TARGET),WINCE)
+cab: installer
+endif

Are people using these targets that you need to keep them? I'd prefer if you just forced people to use "make installer", since that will do the right thing on any platform.

+	cd $(DIST) && $(PYTHON) $(topsrcdir)/build/package/wince/make_wince_cab.py "$(VSINSTALLDIR)" "$(MOZ_PKG_DIR)" "$(MOZ_APP_NAME)" "$(PKG_PATH)$(PKG_BASENAME).CAB"

I think I'd like you to pass the full pathname to cabwiz.exe to the python script here, so the Python script doesn't have to care about it.

In addition, just to guard against anyone screwing up by running this outside of Mozillabuild, can you add (in the makefile, above this somewhere):
VSINSTALLDIR ?= $(error VSINSTALLDIR not set, must be set to the Visual Studio install directory)

That way if that variable isn't set the makefile will error explicitly rather than having the script error in a confusing way.

More comments on the other patch, but this one looks fine otherwise.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 364533 [details] [diff] [review]
v4.0 of mozilla-central patch

+# Usage: python make-wince-inf.py ACTION_DIR DIST_DIR SOURCE_SUBDIR PROGRAM_NAME VS_INSTALL_DIR CAB_FILE_NAME

As I mentioned in the previous comment, I'd like you to pass the full path to cabwiz.exe into this script instead of the VSINSTALL dir.

+if len(sys.argv) != 5:
+    print >> sys.stderr, "Usage: %s VS_INSTALL_DIR SOURCE_DIR PROGRAM_NAME CAB_FINAL_NAME" % sys.argv[0]
+    print >> sys.stderr, "Example: %s /c/Program\ Files/Microsoft\ Visual\ Studio\ 9.0/ fennec Fennec fennec-0.11.en-US.wince-arm.cab" % sys.argv[0]
+    sys.exit(1)
+
+vs_install_dir = sys.argv[1]
+source_dir = sys.argv[2]
+program_name = sys.argv[3]
+app_name = "%s.exe" % source_dir
+cab_final_name = sys.argv[4]

These bits belong in main(). I'm not wild about the use of globals, only because they're kind of clumsy in Python, but whatever floats your boat.

+# Return the contents of FILENAME, a 'install-exceptions' file, as
+# a dictionary whose keys are exactly the list of filenames, along

+    exceptions={}

I think you should use a set here, as that's clearer.
exceptions = set()
exceptions.add(filename)

Then you can use it like a dict, more or less:
if foo in exceptions:
 ...
http://docs.python.org/library/stdtypes.html#set


+        return exceptions

This needs to be outdented by one level. Currently the function returns nothing if the file doesn't exist.

+def fnmatch_any(filename, patterns):

You can ditch this function and just write the call to it like:
any(fnmatch.fnmatch(filename, p) for p in patterns)

That'd be a combination of any:
http://docs.python.org/library/functions.html#any
with a generator expression:
http://www.python.org/dev/peps/pep-0289/

+def sort_dircount_then_filename(item1, item2):

This could use a comment or something, it's not exactly clear what it does (despite the verbose function name).

+    if item1[1] != item2[1]:
+        if item1[1] > item2[1]:
+            return 1
+        else:
+            return -1

Write this like:

if item1[1] != item2[1]:
  return cmp(item1[1], item2[1])
cmp returns exactly what you need here (it's made for use in sorting).

+def add_new_entry(dirpath, dircount, filename, filecount):
+    global file_entries
+    global file_entry_count
+    actual_filename = handle_duplicate_filename(dirpath, filename, filecount)
+    file_entries.append( [dirpath, dircount, filename, filecount, actual_filename] )

I think you'd be better off with a class or at least a dict for these file entries. Using a list and numeric indices makes it really hard to read. You could make a dead simple class like:
class FileEntry:
  def __init__(self, dirpath, dircount, filename, filecount, actual_filename):
    self.dirpath = dirpath.
    self.dircount = dircount
    self.filename = filename
    self.filecount = filecount
    self.actual_filename = actual_filename

then do:
  file_entries.append(FileEntry(dirpath ...))

then when you have an entry object you can just refer to its properties like .dirpath etc.

+    file_entry_count = file_entry_count + 1

Please use file_entry_count += 1 wherever you have code like this.

+        directories[dirpath] = dircount
+        files_in_directory.append( len(filenames) )

It feels like you're maintaining two parallel but intertwined data structures here. Is that right? |directories| is a dict of directory names that maps to an index, and files_in_directory is indexed by that index. Do you really need the indices, and do they need to be consistent? If you don't really need it (i.e. you can make one up later), then just do directories[dirpath] = len(filenames). If you really do, then I might suggest:
directories=[]
directories.append((dirpath, len(filenames))
and then you can just use the index into directories as the index.

+        if len(filenames) < 1:
+            print "NO FILES IN DIRECTORY [%s]" % dirpath

Do you need this output? If so, print it to stdout, and preface it with WARNING: or something.

+            if fnames.has_key( filename ):

if filename in fnames:

+def output_inf_file():

This whole function seems really long, and I have trouble reading it with all the literal strings interspersed in it. I wonder if it wouldn't read more cleanly if it were split up into some smaller helper functions? Maybe my multi-line string suggestion (below) will help.

+    f = open(inf_name, 'wb')
+    if not f:
+        return False

open will throw an exception if it fails, no need to check the result. Also, if you don't open in binary mode, and just use \n, Python writes out CRLF line endings on Windows anyway (at least in my testing).

+    f.write("; Duplicated Filenames ==> One of the two files\r\n")
+    f.write(";   needs renaming before packaging up the CAB\r\n")

I'd prefer multiline strings to be written with triple quoted strings:
f.write("""; Duplicated Filenames ==> One of the two files
;   needs renaming before packaging up the CAB
..."""

+    dirs = directories.keys()
+    dirs.sort()

dirs = sorted(directories)

+        if firstrun:
+            firstrun = False

I don't see that you actually use this anywhere.

+def make_cab_file():
+    cabwiz_path = os.path.join(vs_install_dir, "SmartDevices/SDK/SDKTools/cabwiz.exe")
+    print "CABWIZ_PATH=[%s]" % cabwiz_path
+    make_cab_command = "\"%s\" %s.inf /compress" % (cabwiz_path, program_name)
+    print "EXECUTING COMMAND TO MAKE %s CAB FILE (only works on BASH)" % program_name

Ditch the extra prints.

r- just due to the volume of commentary here. I didn't quite finish, but that's most of the way, I'll try to comment on the last few bits tomorrow.

Comment 14

[(not currently active) Ted Mielczarek]

+def handle_duplicate_filename(dirpath, filename, filecount):
+    shutil.copyfile(old_relative_filename, new_relative_filename)

Why are you copying files here? This is already in the package staging area, so it should be ok to just move them, right? Re-packaging would involve blowing away the staging dir and re-staging anwyay. If you just move, you could then get rid of purge_copied_files as well.

Despite my copious comments, overall I think the script is much improved and should get an r+ next time around if you clean that all up.

Comment 15

[John Wolfe (:wolfe)]

Attachment: v5.0 of mozilla-central patch

Fixes as per ted's comments.  Uber-comment coming after both m-c and m-b attachments are in place.

Comment 16

[John Wolfe (:wolfe)]

Attachment: v5.0 patch for mobile-browser repository

Fixes as per ted's comments.  Uber-comment coming after both m-c and m-b
attachments are in place.

Comment 17

[John Wolfe (:wolfe)]

(In reply to comment #12)
> (From update of attachment 364534 [details] [diff] [review])
> +deb: installer
>  endif
> +
> +ifeq ($(OS_TARGET),WINCE)
> +cab: installer
> +endif
> 
> Are people using these targets that you need to keep them? I'd prefer if you
> just forced people to use "make installer", since that will do the right thing
> on any platform.


"make installer" still works correctly.  Until now, for Linux builds, 
the command was "make deb" - and I wanted to keep that functionality 
in place.  There was not a "make installer" for nokia builds until 
this patch.  

I also created a "make cab" command in place as an alias for 
"make installer" in order to match the Linux build pattern within 
WinCE/WinMobile builds.  

The "make deb" and "make cab" commands are redundant, and if anyone 
objects strongly enough, I will remove these two alias commands.


> +    cd $(DIST) && $(PYTHON) $(topsrcdir)/build/package/wince/make_wince_cab.py
> "$(VSINSTALLDIR)" "$(MOZ_PKG_DIR)" "$(MOZ_APP_NAME)"
> "$(PKG_PATH)$(PKG_BASENAME).CAB"
> 
> I think I'd like you to pass the full pathname to cabwiz.exe to the python
> script here, so the Python script doesn't have to care about it.

DONE.


> In addition, just to guard against anyone screwing up by running this outside
> of Mozillabuild, can you add (in the makefile, above this somewhere):
> VSINSTALLDIR ?= $(error VSINSTALLDIR not set, must be set to the Visual Studio
> install directory)
> 
> That way if that variable isn't set the makefile will error explicitly rather
> than having the script error in a confusing way.

DONE.  I also added a check into the make_wince_cab.py PYTHON script to
verify that the CABWIZ_PATH points to a valid file, and exits with a small
explanation if no valid file is found.

Comment 18

[John Wolfe (:wolfe)]

(In reply to comment #13)
> (From update of attachment 364533 [details] [diff] [review])
> +# Usage: python make-wince-inf.py ACTION_DIR DIST_DIR SOURCE_SUBDIR
> PROGRAM_NAME VS_INSTALL_DIR CAB_FILE_NAME
> 
> As I mentioned in the previous comment, I'd like you to pass the full path to
> cabwiz.exe into this script instead of the VSINSTALL dir.

DONE.


> +if len(sys.argv) != 5:
> +    print >> sys.stderr, "Usage: %s VS_INSTALL_DIR SOURCE_DIR PROGRAM_NAME
> CAB_FINAL_NAME" % sys.argv[0]
> +    print >> sys.stderr, "Example: %s /c/Program\ Files/Microsoft\ Visual\
> Studio\ 9.0/ fennec Fennec fennec-0.11.en-US.wince-arm.cab" % sys.argv[0]
> +    sys.exit(1)
> +
> +vs_install_dir = sys.argv[1]
> +source_dir = sys.argv[2]
> +program_name = sys.argv[3]
> +app_name = "%s.exe" % source_dir
> +cab_final_name = sys.argv[4]
> 
> These bits belong in main(). I'm not wild about the use of globals, only
> because they're kind of clumsy in Python, but whatever floats your boat.

DONE - moved into main() function, and passed to various subroutines as 
function arguments instead of globals.


> +# Return the contents of FILENAME, a 'install-exceptions' file, as
> +# a dictionary whose keys are exactly the list of filenames, along
> 
> +    exceptions={}
> 
> I think you should use a set here, as that's clearer.
> exceptions = set()
> exceptions.add(filename)
> 
> Then you can use it like a dict, more or less:
> if foo in exceptions:
>  ...
> http://docs.python.org/library/stdtypes.html#set

DONE.


> +        return exceptions
> 
> This needs to be outdented by one level. Currently the function returns nothing
> if the file doesn't exist.

DONE.


> +def fnmatch_any(filename, patterns):
> 
> You can ditch this function and just write the call to it like:
> any(fnmatch.fnmatch(filename, p) for p in patterns)
> 
> That'd be a combination of any:
> http://docs.python.org/library/functions.html#any
> with a generator expression:
> http://www.python.org/dev/peps/pep-0289/

SWEET, THOUGH QUITE A TONGUE TWISTER - DONE.


> +def sort_dircount_then_filename(item1, item2):
> 
> This could use a comment or something, it's not exactly clear what it does
> (despite the verbose function name).
> 
> +    if item1[1] != item2[1]:
> +        if item1[1] > item2[1]:
> +            return 1
> +        else:
> +            return -1
> 
> Write this like:
> 
> if item1[1] != item2[1]:
>   return cmp(item1[1], item2[1])
> cmp returns exactly what you need here (it's made for use in sorting).

DONE.


> +def add_new_entry(dirpath, dircount, filename, filecount):
> +    global file_entries
> +    global file_entry_count
> +    actual_filename = handle_duplicate_filename(dirpath, filename, filecount)
> +    file_entries.append( [dirpath, dircount, filename, filecount,
> actual_filename] )
> 
> I think you'd be better off with a class or at least a dict for these file
> entries. Using a list and numeric indices makes it really hard to read. You
> could make a dead simple class like:
> class FileEntry:
>   def __init__(self, dirpath, dircount, filename, filecount, actual_filename):
>     self.dirpath = dirpath.
>     self.dircount = dircount
>     self.filename = filename
>     self.filecount = filecount
>     self.actual_filename = actual_filename
> 
> then do:
>   file_entries.append(FileEntry(dirpath ...))
> 
> then when you have an entry object you can just refer to its properties like
> .dirpath etc.

DONE.  A lot nicer looking code, too - thanks.


> +    file_entry_count = file_entry_count + 1
> 
> Please use file_entry_count += 1 wherever you have code like this.

DONE.


> +        directories[dirpath] = dircount
> +        files_in_directory.append( len(filenames) )
> 
> It feels like you're maintaining two parallel but intertwined data structures
> here. Is that right? |directories| is a dict of directory names that maps to an
> index, and files_in_directory is indexed by that index. Do you really need the
> indices, and do they need to be consistent? If you don't really need it (i.e.
> you can make one up later), then just do directories[dirpath] = len(filenames).
> If you really do, then I might suggest:
> directories=[]
> directories.append((dirpath, len(filenames))
> and then you can just use the index into directories as the index.

You are absolutely correct - I just wanted to be able to find the 
number of files in a particular directory.  That way, if a directory 
had no files inside, then I can output some different lines to the INF file.

I reformatted how I was saving this information, so that it all stays 
together in another simple class object, inside a set whose keys are the 
directory names.


> +        if len(filenames) < 1:
> +            print "NO FILES IN DIRECTORY [%s]" % dirpath
> 
> Do you need this output? If so, print it to stdout, and preface it with
> WARNING: or something.

DONE - mostly as a safety check for the next CAB file creation after fennec.


> +            if fnames.has_key( filename ):
> 
> if filename in fnames:

DONE.


> +def output_inf_file():
> 
> This whole function seems really long, and I have trouble reading it with all
> the literal strings interspersed in it. I wonder if it wouldn't read more
> cleanly if it were split up into some smaller helper functions? Maybe my
> multi-line string suggestion (below) will help.

Broken up into smaller functions, approximately one function per INF file 
section.


> +    f = open(inf_name, 'wb')
> +    if not f:
> +        return False
> 
> open will throw an exception if it fails, no need to check the result. 

DONE - Removed "if not f:"


> Also, if you don't open in binary mode, and just use \n, Python writes 
> out CRLF line endings on Windows anyway (at least in my testing).
> 
> +    f.write("; Duplicated Filenames ==> One of the two files\r\n")
> +    f.write(";   needs renaming before packaging up the CAB\r\n")

Checked out opening in regular (non-binary) mode, and writing out only
"\n" where needed - and that works.  Thanks for the tip - it makes the 
code a bit easier to parse.  

Silly CABWIZ.EXE needs CR-LF at the end of each line, or you get a 
"DirIDs not defined" error message.  Really self-explanitory, eh?

Added a check for CABWIZ.EXE failure, and added a little "help"
text to assist those whose CAB files fail to build properly.



> I'd prefer multiline strings to be written with triple quoted strings:
> f.write("""; Duplicated Filenames ==> One of the two files
> ;   needs renaming before packaging up the CAB
> ..."""

Packaged with as many multiline strings as I could manage.

There seems to be a bug in the PYTHON version I am using (2.5).
Multiline strings which try to output "%" as well as doing
a string substitution - I could not get it to work properly
after a lot of trying.

So I went back to single line strings for the problem lines.

There are still a number of multiline strings for your 
reviewing pleasure.



> +    dirs = directories.keys()
> +    dirs.sort()
> 
> dirs = sorted(directories)

DONE.


> +        if firstrun:
> +            firstrun = False
> 
> I don't see that you actually use this anywhere.

firstrun is used right where the comparison is done, and is used
to output something slightly different for the first line than for
all other lines output in the newly-created function
output_defaultinstall_section().


> +def make_cab_file():
> +    cabwiz_path = os.path.join(vs_install_dir,
> "SmartDevices/SDK/SDKTools/cabwiz.exe")
> +    print "CABWIZ_PATH=[%s]" % cabwiz_path
> +    make_cab_command = "\"%s\" %s.inf /compress" % (cabwiz_path, program_name)
> +    print "EXECUTING COMMAND TO MAKE %s CAB FILE (only works on BASH)" %
> program_name
> 
> Ditch the extra prints.

I left in the print for what command line is being executed, just in 
case the CAB file creation does not work, and someone needs to debug
the reasons why.

Comment 19

[John Wolfe (:wolfe)]

(In reply to comment #14)
> +def handle_duplicate_filename(dirpath, filename, filecount):
> +    shutil.copyfile(old_relative_filename, new_relative_filename)
> 
> Why are you copying files here? This is already in the package staging area, so
> it should be ok to just move them, right? Re-packaging would involve blowing
> away the staging dir and re-staging anwyay. If you just move, you could then
> get rid of purge_copied_files as well.
> 
> Despite my copious comments, overall I think the script is much improved and
> should get an r+ next time around if you clean that all up.


I do the copying for one simple reason.  WinMobile / WinCE is about as 
nasty a development environment around.  The staging directory is the 
easiest place to get the whole installation directory contents in one 
place - available for simply copying onto a device and debugging with 
a simple command:

"xulrunner.exe ../application.ini"

If I rename instead of copy, and something goes wrong, the staging 
directory is no longer a good directory to just copy -- and there is 
no indication to debuggers (like me, for instance) that the directory 
is corrupted.

So, rather than waste a few hours chasing down a bug that is not really 
a bug, I thought I would copy, make the CAB file, then delete my copied 
files.  Extra work during CAB file production, but the whole build already 
takes close to an hour on a decent Windows machine.

Comment 20

[(not currently active) Ted Mielczarek]

Thanks for the detailed reply. I'll give this another once-over today.

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 369007 [details] [diff] [review]
v5.0 patch for mobile-browser repository

+ifeq ($(OS_TARGET),WINCE)
+cab: installer
+endif

Just ditch the cab target, I don't see a compelling reason to introduce it. Ideally we'd ditch the deb target too, but people are probably using that. Just tell people to use "make installer" when they want a cab.

r=me otherwise.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 369005 [details] [diff] [review]
v5.0 of mozilla-central patch

+    # Next compare filenames
+    next_result = cmp(item1.filename, item2.filename)
+    if ( next_result != 0 ):

Ditch the parentheses in the if here, they're unnecessary in Python. 

+    # Lastly, compare filecounts
+    next_result = cmp(item1.filecount, item2.filecount)
+    if ( next_result != 0 ):
+        return next_result
+
+    # Otherwise, these are the same records!
+    return 0

You can actually ditch this if statement and just return cmp(...) here, since you're returning 0 when it's 0 anyway.

+Signature   = \"$Windows NT$\"        ; required as-is

You don't need to backslash-escape a double quote character inside a triple-quoted string.

+    for dir in dirs:
+        f.write("%d = , \"%s\",,%s\n" % (directories[dir].dircount, dir, dir))

If you'd like, you can use a triple quoted string here or use single quotes (Python allows ' or " as string delimiters, they're treated equally, but must be matched). Either would make it so you don't have to backslash-escape the double quotes.

+    file_entries.sort(sort_dircount_then_filename)
+
+    for entry in file_entries:

I'd write this as:
for entry in sorted(file_entries, sort_dircount_then_filename):
but that's just preference. Your call.

+        if firstrun:
+            firstrun = False

Is this just to skip the first entry in the list? If so, you could make the loop like:
for dir in dirs[1:]:
To only loop over elements 1 and later.

+        else:
+            copyfileslist = "%s, " % copyfileslist
+        mod_dir_string = string.join( string.split(dir, '\\'), '.' )
+        copyfileslist = "%sFiles.%s" % (copyfileslist, mod_dir_string)

I'm not wild about this string-constructing stuff. Generally in Python if you're going to do something like this you'd build a list and then join it with a string or something at the end. Could you do this a different way?

+        mod_dir_string = string.join( string.split(dir, '\\'), '.' )

Also, this would normally be written in Python as:
mod_dir_string = '.'.join(dir.split('\\'))
(just calling the bound string methods on the strings themselves)

+        if len(dir_minus_top_level) > 0:
+            dir_minus_top_level = "\\%s" % dir_minus_top_level

You can just say:
if dir_minus_top_level:

+            for entry in file_entries:
+                if entry.dirpath != dir:
+                    continue
+                else:
+                    f.write("\"%s\",%s\n" % (entry.filename, entry.actual_filename))

I'd just write:
if entry.dirpath == dir:
  f.write(...)
and skip the extra block+continue.

+    # The following DOES NOT WORK...

Then remove it. :) Please don't leave commented code in the file.

+    if not os.path.isfile("%s.CAB" % program_name):
+        print "***************************************************************************"
+        print "ERROR: CAB FILE NOT CREATED."

Triple-quoted string here?

+    if not os.path.isfile(cabwiz_path):
+        print "***************************************************************************"
+        print "ERROR: CABWIZ_PATH is not a valid file!"
+        print "       Perhaps your VSINSTALLDIR is not properly set up?"
+        print "       EXITING..."
+        print "***************************************************************************"
+        return

And here. Also, I'd prefer sys.exit(1) to a bare return here.

r=me with those comments addressed.

Comment 23

[John Wolfe (:wolfe)]

Attachment: v5.1 patch for mobile-browser repository

(In reply to comment #21)
> (From update of attachment 369007 [details] [diff] [review])
> +ifeq ($(OS_TARGET),WINCE)
> +cab: installer
> +endif
> 
> Just ditch the cab target, I don't see a compelling reason to introduce it.
> Ideally we'd ditch the deb target too, but people are probably using that.
> Just tell people to use "make installer" when they want a cab.

DONE.

> r=me otherwise.

Comment 24

[John Wolfe (:wolfe)]

Attachment: v6.0 of mozilla-central patch

Ted, can you take one more look at the patch, just to be sure.  It works for me, but there were a lot of changes, including a reworked loop in the output_defaultinstall_section() function.


(In reply to comment #22)
> (From update of attachment 369005 [details] [diff] [review])
> +    # Next compare filenames
> +    next_result = cmp(item1.filename, item2.filename)
> +    if ( next_result != 0 ):
> 
> Ditch the parentheses in the if here, they're unnecessary in Python. 

DONE.


> +    # Lastly, compare filecounts
> +    next_result = cmp(item1.filecount, item2.filecount)
> +    if ( next_result != 0 ):
> +        return next_result
> +
> +    # Otherwise, these are the same records!
> +    return 0
> 
> You can actually ditch this if statement and just return cmp(...) here, since
> you're returning 0 when it's 0 anyway.

Good Point - DONE.


> +Signature   = \"$Windows NT$\"        ; required as-is
> 
> You don't need to backslash-escape a double quote character inside a
> triple-quoted string.

OK - cleaned up triple-quoted strings containing \" - DONE.


> +    for dir in dirs:
> +        f.write("%d = , \"%s\",,%s\n" % (directories[dir].dircount, dir, dir))
> 
> If you'd like, you can use a triple quoted string here or use single quotes
> (Python allows ' or " as string delimiters, they're treated equally, but must
> be matched). Either would make it so you don't have to backslash-escape the
> double quotes.

Used Triple-Quoted Strings - DONE.


> +    file_entries.sort(sort_dircount_then_filename)
> +
> +    for entry in file_entries:
> 
> I'd write this as:
> for entry in sorted(file_entries, sort_dircount_then_filename):
> but that's just preference. Your call.

You are the master - changed and DONE.


> +        if firstrun:
> +            firstrun = False
> 
> Is this just to skip the first entry in the list? If so, you could make the
> loop like:
> for dir in dirs[1:]:
> To only loop over elements 1 and later.

Cleaned up the whole loop.  You should like it better now.  
More comments below in the next section.


> +        else:
> +            copyfileslist = "%s, " % copyfileslist
> +        mod_dir_string = string.join( string.split(dir, '\\'), '.' )
> +        copyfileslist = "%sFiles.%s" % (copyfileslist, mod_dir_string)
> 
> I'm not wild about this string-constructing stuff. Generally in Python if
> you're going to do something like this you'd build a list and then join it with
> a string or something at the end. Could you do this a different way?

Built a list, then did string modification on the resulting list.
Moved split/join to work on just the current DIR variable as well.


> +        mod_dir_string = string.join( string.split(dir, '\\'), '.' )
> 
> Also, this would normally be written in Python as:
> mod_dir_string = '.'.join(dir.split('\\'))
> (just calling the bound string methods on the strings themselves)

Oh, this is good to know.


> +        if len(dir_minus_top_level) > 0:
> +            dir_minus_top_level = "\\%s" % dir_minus_top_level
> 
> You can just say:
> if dir_minus_top_level:

Can you tell I dream in 'C' ?  DONE.


> +            for entry in file_entries:
> +                if entry.dirpath != dir:
> +                    continue
> +                else:
> +                    f.write("\"%s\",%s\n" % (entry.filename,
> entry.actual_filename))
> 
> I'd just write:
> if entry.dirpath == dir:
>   f.write(...)
> and skip the extra block+continue.

Office of Redundancy Office was notified.  DONE.


> +    # The following DOES NOT WORK...
> 
> Then remove it. :) Please don't leave commented code in the file.

Was hoping you could explain why the triple-quoted string with string 
replacement AND '%' characters fails so miserably.  BUT, I will not 
lose any sleep over it.  Commented out code removed.  DONE.


> +    if not os.path.isfile("%s.CAB" % program_name):
> +        print
> "***************************************************************************"
> +        print "ERROR: CAB FILE NOT CREATED."
> 
> Triple-quoted string here?

Great Point! DONE here and as per the next comment.


> +    if not os.path.isfile(cabwiz_path):
> +        print
> "***************************************************************************"
> +        print "ERROR: CABWIZ_PATH is not a valid file!"
> +        print "       Perhaps your VSINSTALLDIR is not properly set up?"
> +        print "       EXITING..."
> +        print
> "***************************************************************************"
> +        return
> 
> And here. Also, I'd prefer sys.exit(1) to a bare return here.

DONE.  I now return sys.exit(2) here, since I already do a sys.exit(1) above.

> r=me with those comments addressed.

SWEET!

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 370266 [details] [diff] [review]
v6.0 of mozilla-central patch

+            if line != '' and line[0] != '#':
+                exceptions.add(line)

I'd do if line and line[0] != '#':, but that's just preference.

+    # Lastly, compare filecounts
+    next_result = cmp(item1.filecount, item2.filecount)
+    return next_result

I'd just return cmp(...) here instead of using the intermediate var.

+    copyfileslist = string.join(copyfilesrawlist, ',')

','.join(copyfilesrawlist)

+        dir_split = string.split(dir, '\\')

dir.split('\\')

+        mod_dir_string = string.join(dir_split, '.')

'.'.join(dir_split)

+            dir_minus_top_level = string.join(dir_split[1:], '\\')

'\\'.join(dir_split[1:])

We'll get you thinking in Python eventually! ;-)

Looks good, thanks for seeing this through!
r=me

Comment 26

[(not currently active) Ted Mielczarek]

Please attach a final version of your patch addressing my final review comments before setting checkin-needed!

Comment 27

[John Wolfe (:wolfe)]

Attachment: v6.1 of mozilla-central patch

Ted, I am sorry that I misunderstood your comment.

I had thought that you were pointing out style comments,
not that you wanted those items changed.

Here is a new attachment which addresses all the points
you raised.


(In reply to comment #25)
> (From update of attachment 370266 [details] [diff] [review])
> +            if line != '' and line[0] != '#':
> +                exceptions.add(line)
> 
> I'd do if line and line[0] != '#':, but that's just preference.

DONE.


> +    # Lastly, compare filecounts
> +    next_result = cmp(item1.filecount, item2.filecount)
> +    return next_result
> 
> I'd just return cmp(...) here instead of using the intermediate var.

DONE.

> +    copyfileslist = string.join(copyfilesrawlist, ',')
> 
> ','.join(copyfilesrawlist)

DONE. All string.join constructs were replaced with the char
dot join construct instead.

> +        dir_split = string.split(dir, '\\')
> 
> dir.split('\\')

DONE. All string.split constructs were replaced with the string
dot split construct instead.


> +        mod_dir_string = string.join(dir_split, '.')
> 
> '.'.join(dir_split)

DONE. 


> +            dir_minus_top_level = string.join(dir_split[1:], '\\')
> 
> '\\'.join(dir_split[1:])

DONE. 


> We'll get you thinking in Python eventually! ;-)
>
> Looks good, thanks for seeing this through!
> r=me

Thanks Ted!

Comment 28

[John Wolfe (:wolfe)]

Comment on attachment 370675 [details] [diff] [review]
v6.1 of mozilla-central patch

>diff --git a/build/package/wince/make_wince_cab.py b/build/package/wince/make_wince_cab.py
>new file mode 100644
>--- /dev/null
>+++ b/build/package/wince/make_wince_cab.py
>@@ -0,0 +1,368 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Mozilla build system.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2007
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#  John Wolfe <[e-mail]>
>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
>+################################################################
>+#
>+# make-wince-cab.py --- Given a directory, walk it and make an
>+#          installer based upon the contents of that directory
>+#
>+# Usage: python make-wince-inf.py CABWIZ_PATH SOURCE_DIR PROGRAM_NAME CAB_FINAL_NAME
>+#
>+# Walk through the relative directory SOURCE_DIR, parsing filenames
>+#   Checks for duplicate filenames and renames where needed
>+#   Then builds a WinMobile INF file based upon results
>+# 
>+# Each directory in SOURCE_DIR may have a file named
>+# 'install-exceptions', which lists files in SOURCE_DIR that
>+# need not be placed into an installation CAB file. The
>+# 'install-exceptions' file itself is always ignored.
>+# 
>+# Blank lines and '#' comments in the 'install-exceptions' file are
>+# ignored.
>+#
>+# EXAMPLE OF COMMAND LINE:
>+#   python make_wince_inf.py /c/Program\ Files/Microsoft\ Visual\ Studio\ 9.0/SmartDevices/SDK/SDKTools/cabwiz.exe dist/fennec Fennec fennec-0.11.en-US.wince-arm.cab
>+#
>+# ARGS:
>+#   cabiz_path - Path to CABWIZ.EXE executable inside Visual Studio
>+#
>+#   source_dir - sub-directory which contains the target program 
>+#                NOTE: It is assumed that the application name is SOURCE_DIR.exe
>+#                EXAMPLE: source_dir=fennec, there should be a fennec/fennec.exe application
>+#
>+#   program_name - Name of the program to place inside the INF file
>+#
>+#   cab_final_name - actual final name for the produced CAB file
>+#
>+# NOTE: In our example, "fennec" is the directory [source_name]
>+#                       "fennec.exe" is the application [$(source_name).exe], and
>+#                       "Fennec" is the shortcut name [program_name]
>+################################################################
>+
>+import sys
>+import os
>+from os.path import join
>+from subprocess import call, STDOUT
>+import fnmatch
>+import string
>+import shutil
>+
>+
>+class FileEntry:
>+    def __init__(self, dirpath, dircount, filename, filecount, actual_filename):
>+        self.dirpath = dirpath
>+        self.dircount = dircount
>+        self.filename = filename
>+        self.filecount = filecount
>+        self.actual_filename = actual_filename
>+
>+
>+class DirEntry:
>+    def __init__(self, dirpath, dircount, filecount):
>+        self.dirpath = dirpath
>+        self.dircount = dircount
>+        self.filecount = filecount
>+
>+# Ignore detritus left lying around by editing tools.
>+ignored_patterns = ['*~', '.#*', '#*#', '*.orig', '*.rej']
>+
>+file_entry_count = 0
>+file_entries = []
>+
>+fcount = 0
>+fnames = {}
>+
>+directories = {}
>+files_in_directory = []
>+
>+# Return the contents of FILENAME, a 'install-exceptions' file, as
>+# a dictionary whose keys are exactly the list of filenames, along
>+# with the basename of FILENAME itself.  If FILENAME does not exist,
>+# return the empty dictionary.
>+def read_exceptions(filename):
>+    exceptions = set()
>+    if os.path.exists(filename):
>+        f = file(filename)
>+        for line in f:
>+            line = line.strip()
>+            if line and line[0] != '#':
>+                exceptions.add(line)
>+        exceptions.add( os.path.basename(filename) )
>+        f.close()
>+    return exceptions
>+
>+
>+# Sorts two items based first upon directory count (index of
>+# directory in list of directories), then upon filename.  A way of
>+# getting a list sorted into directories, and then alphabetically by
>+# filename within each directory. 
>+def sort_dircount_then_filename(item1, item2):
>+    # First Compare dirpaths
>+    if item1.dircount != item2.dircount:
>+        return cmp(item1.dircount, item2.dircount)
>+
>+    # Next compare filenames
>+    next_result = cmp(item1.filename, item2.filename)
>+    if next_result != 0:
>+        return next_result
>+
>+    # Lastly, compare filecounts
>+    return cmp(item1.filecount, item2.filecount)
>+
>+    
>+def handle_duplicate_filename(dirpath, filename, filecount):
>+    if filecount == 1:
>+        return filename
>+    file_parts = os.path.splitext(filename)
>+    new_filename = "%s_%d%s" % (file_parts[0], filecount, file_parts[1])
>+    old_relative_filename = join(dirpath, filename)
>+    new_relative_filename = join(dirpath, new_filename)
>+    shutil.copyfile(old_relative_filename, new_relative_filename)
>+    return new_filename
>+
>+def add_new_entry(dirpath, dircount, filename, filecount):
>+    global file_entries
>+    global file_entry_count
>+    actual_filename = handle_duplicate_filename(dirpath, filename, filecount)
>+    file_entries.append( FileEntry(dirpath, dircount, filename, filecount, actual_filename) )
>+    file_entry_count += 1
>+
>+
>+def walk_tree(start_dir, ignore):
>+    global fcount
>+    global fnames
>+    global directories
>+    global files_in_directory
>+    
>+    dircount = 0;
>+    files_in_directory = [0]
>+
>+    for (dirpath, dirnames, filenames) in os.walk(start_dir):
>+        exceptions = read_exceptions(join(dirpath, 'install-exceptions'))
>+
>+        dircount += 1
>+        directories[dirpath] = DirEntry(dirpath, dircount, len(filenames))
>+
>+        if len(filenames) < 1:
>+            print "WARNING: No files in directory [%s]" % dirpath
>+
>+        for filename in filenames:
>+            if len(exceptions) > 0 and filename in exceptions:
>+                continue
>+            if any(fnmatch.fnmatch(filename, p) for p in ignore):
>+                continue
>+            filecount = 1
>+            if filename in fnames:
>+                filecount = fnames[filename] + 1
>+            fnames[filename] = filecount
>+
>+            add_new_entry(dirpath, dircount, filename, filecount)
>+
>+
>+def output_inf_file_header(f, program_name):
>+    f.write("""; Duplicated Filenames ==> One of the two files
>+;   needs renaming before packaging up the CAB
>+;
>+; Technique: Rename the second directory's file to XXXX_1 prior to starting the CABWIZ,
>+;   then take care of the name in the File.xxxxx section
>+
>+[Version]
>+Signature   = "$Windows NT$"        ; required as-is
>+Provider    = "Mozilla"             ; maximum of 30 characters, full app name will be \"<Provider> <AppName>\"
>+CESignature = "$Windows CE$"        ; required as-is
>+
>+[CEStrings]
>+AppName     = "%s"              ; maximum of 40 characters, full app name will be \"<Provider> <AppName>\"\n""" % program_name)
>+
>+    f.write("InstallDir  = %CE1%\\%AppName%       ; Program Files\Fennec\n\n")
>+
>+
>+def output_sourcedisksnames_section(f, dirs):
>+    f.write("[SourceDisksNames]                  ; directory that holds the application's files\n")
>+    for dir in dirs:
>+        f.write("""%d = , "%s",,%s\n""" % (directories[dir].dircount, dir, dir))
>+    f.write(" \n")
>+
>+
>+def output_sourcedisksfiles_section(f):
>+    f.write("[SourceDisksFiles]                  ; list of files to be included in .cab\n")
>+    for entry in sorted(file_entries, sort_dircount_then_filename):
>+        f.write("%s=%d\n" % (entry.actual_filename, entry.dircount))
>+    f.write("\n")
>+
>+
>+def output_defaultinstall_section(f, dirs):
>+    copyfileslist = ""
>+    copyfilesrawlist=[]
>+    for dir in dirs:
>+        if directories[dir].filecount < 1:
>+            continue
>+        copyfilesrawlist.append( "Files.%s" % '.'.join( dir.split('\\') ) )
>+        prefix = ", "
>+    copyfileslist = ','.join(copyfilesrawlist)
>+
>+    f.write("""[DefaultInstall]                    ; operations to be completed during install
>+CopyFiles   = %s
>+AddReg      = RegData
>+CEShortcuts = Shortcuts
>+\n""" % copyfileslist)
>+
>+
>+def output_destinationdirs_section(f, dirs):
>+    f.write("[DestinationDirs]                   ; default destination directories for each operation section\n")
>+    for dir in dirs:
>+        dir_split = dir.split('\\')
>+        mod_dir_string = '.'.join(dir_split)
>+        if len(dir_split) > 1:
>+            dir_minus_top_level = '\\'.join(dir_split[1:])
>+        else:
>+            dir_minus_top_level = ""
>+        if dir_minus_top_level:
>+            dir_minus_top_level = "\\%s" % dir_minus_top_level
>+        if directories[dir].filecount < 1:
>+            f.write(";Files.%s = 0, %%InstallDir%%%s          ; NO FILES IN THIS DIRECTORY\n" % (mod_dir_string, dir_minus_top_level))
>+        else:
>+            f.write("Files.%s = 0, %%InstallDir%%%s\n" % (mod_dir_string, dir_minus_top_level))
>+    f.write("Shortcuts   = 0, %CE11%             ; \Windows\Start Menu\Programs\n\n")
>+
>+
>+def output_directory_sections(f, dirs):
>+    for dir in dirs:
>+        if directories[dir].filecount < 1:
>+            f.write(";[Files.%s]\n;===NO FILES===\n" % '.'.join( dir.split('\\') ))
>+        else:
>+            f.write("[Files.%s]\n" % '.'.join( dir.split('\\') ))
>+            for entry in file_entries:
>+                if entry.dirpath == dir:
>+                    f.write("\"%s\",%s\n" % (entry.filename, entry.actual_filename))
>+        f.write("\n")
>+
>+
>+def output_registry_section(f):
>+    f.write("""[RegData]                           ; registry key list
>+HKCU,Software\%AppName%,MajorVersion,0x00010001,1
>+HKCU,Software\%AppName%,MinorVersion,0x00010001,0
>+\n""")
>+
>+
>+def output_shortcuts_section(f, app_name):
>+    f.write("[Shortcuts]                         ; Shortcut created in destination dir, %CE14%\n")
>+    f.write("%%AppName%%,0,%s\n" % app_name)
>+
>+
>+def output_inf_file(program_name, app_name):
>+    global files_in_directory
>+    inf_name = "%s.inf" % program_name
>+    f = open(inf_name, 'w')
>+    output_inf_file_header(f, program_name)
>+    dirs = sorted(directories)
>+    output_sourcedisksnames_section(f, dirs)
>+    output_sourcedisksfiles_section(f)
>+    output_defaultinstall_section(f, dirs)
>+    output_destinationdirs_section(f, dirs)
>+    output_directory_sections(f, dirs)
>+    output_registry_section(f)
>+    output_shortcuts_section(f, app_name)
>+    f.close()
>+
>+
>+
>+def make_cab_file(cabwiz_path, program_name, cab_final_name):
>+    make_cab_command = "\"%s\" %s.inf /compress" % (cabwiz_path, program_name)
>+    print "INFORMATION: Executing command to make %s CAB file (only works on BASH)" % program_name
>+    print "    [%s]" % make_cab_command
>+    sys.stdout.flush()
>+
>+    success = call([cabwiz_path, "%s.inf" % program_name, "/compress"],
>+                   stdout=open("NUL:","w"), stderr=STDOUT)
>+
>+    if not os.path.isfile("%s.CAB" % program_name):
>+        print """***************************************************************************
>+ERROR: CAB FILE NOT CREATED.
>+       You can try running the command by hand:
>+          %s" % make_cab_comman
>+ ---- 
>+ NOTE: If you see an error like this:
>+       Error: File XXXXXXXXXX.inf contains DirIDs, which are not supported
>+ -- 
>+ this may mean that your PYTHON is outputting Windows files WITHOUT CR-LF
>+ line endings.  Please verify that your INF file has CR-LF line endings.
>+***************************************************************************"""
>+        return
>+
>+    print "INFORMATION: Executing command to move %s.CAB to %s" % (program_name, cab_final_name)
>+    sys.stdout.flush()
>+    shutil.move("%s.CAB" % program_name, cab_final_name)
>+
>+
>+def purge_copied_files():
>+    for entry in file_entries:
>+        if entry.filename != entry.actual_filename:
>+            new_relative_filename = join(entry.dirpath, entry.actual_filename)
>+            os.remove(new_relative_filename)
>+
>+
>+def main():
>+    if len(sys.argv) != 5:
>+        print >> sys.stderr, "Usage: %s CABWIZ_PATH SOURCE_DIR PROGRAM_NAME CAB_FINAL_NAME" % sys.argv[0]
>+        print >> sys.stderr, "Example: %s /c/Program\ Files/Microsoft\ Visual\ Studio\ 9.0/ fennec Fennec fennec-0.11.en-US.wince-arm.cab" % sys.argv[0]
>+        sys.exit(1)
>+
>+    cabwiz_path = sys.argv[1]
>+    source_dir = sys.argv[2]
>+    program_name = sys.argv[3]
>+    app_name = "%s.exe" % source_dir
>+    cab_final_name = sys.argv[4]
>+
>+    if not os.path.isfile(cabwiz_path):
>+        print """***************************************************************************
>+ERROR: CABWIZ_PATH is not a valid file!
>+       Perhaps your VSINSTALLDIR is not properly set up?
>+       EXITING...
>+***************************************************************************"""
>+        sys.exit(2)
>+
>+    walk_tree(source_dir, ignored_patterns)
>+    sys.stdout.flush()
>+    output_inf_file(program_name, app_name)
>+    sys.stdout.flush()
>+    make_cab_file(cabwiz_path, program_name, cab_final_name)
>+    purge_copied_files()
>+
>+
>+# run main if run directly
>+if __name__ == "__main__":
>+    main()
>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -65,9 +65,7 @@
> MOZ_PKG_FORMAT  = BZ2
> else
> ifeq (,$(filter-out WINCE, $(OS_ARCH)))
>-MOZ_PKG_CAB_SCRIPT ?= $(error MOZ_PKG_CAB_SCRIPT not specified)
>-MOZ_PKG_CAB_INF ?= $(error MOZ_PKG_CAB_INF not specified)
>-MOZ_PKG_FORMAT  = CAB
>+MOZ_PKG_FORMAT  = ZIP
> else
> MOZ_PKG_FORMAT  = TGZ
> endif
>@@ -115,12 +113,6 @@
> UNMAKE_PACKAGE	= $(UNZIP) $(UNPACKAGE)
> MAKE_SDK = $(ZIP) -r9D $(SDK) $(MOZ_APP_NAME)-sdk
> endif
>-ifeq ($(MOZ_PKG_FORMAT),CAB)
>-PKG_SUFFIX	= .cab
>-MAKE_PACKAGE = $(MOZ_PKG_CAB_SCRIPT) "$(VSINSTALLDIR)" "$(topsrcdir)" "$(MOZ_PKG_DIR)" "$(MOZ_PKG_CAB_INF)" "$(MOZ_APP_NAME)" "$(PACKAGE)"
>-UNMAKE_PACKAGE	= $(UNZIP) $(UNPACKAGE)
>-MAKE_SDK = $(ZIP) -r9D $(SDK) $(MOZ_APP_NAME)-sdk
>-endif
> ifeq ($(MOZ_PKG_FORMAT),DMG)
> ifndef _APPNAME
> ifdef MOZ_DEBUG

Comment 29

[John Wolfe (:wolfe)]

Attachment: v6.2 of mozilla-central patch

A small change to nsIdleServiceWindow.cpp found its way into the v6.1 patch.

Removed from the v6.2 patch, and moved to bug 475361 where it belongs.

Comment 30

[John Wolfe (:wolfe)]

Mozilla-Central attachment (id=370679)
changeset:   27055:e770072e3c74
tag:         tip
user:        John Wolfe <[e-mail]>
date:        Tue Apr 07 23:44:03 2009 -0400
summary:     Bug 476733 - WinCE Dynamic CAB INF File Production Needed - r+ted


Mobile-Browser attachment (id=370263)
changeset:   468:df1125b50527
tag:         tip
user:        John Wolfe <[e-mail]>
date:        Tue Apr 07 23:29:11 2009 -0400
summary:     Bug 476733 - WinCE Dynamic CAB INF File Production Needed - r+ted

Comment 31

[John Wolfe (:wolfe)]

We forgot to modify the buildbot to expect to do a "make package" in the mobile top level objdir -- so the tinderbox for mobile-wince-arm-dep is burning!

Comment 32

[John Wolfe (:wolfe)]

Attachment: buildbot broken - dirty patch to quickly fix tinderbox builds

This patch changes the mobile-browser's "make package" command for WinCE / WinMobile builds to actually do a "make installer" command within the mobile/installer directory.

This patch also changes the "make installer" command within the mobile/installer directory to build a fennec ZIP file that the buildbot expects to see.

This patch covers for the fact that the buildbots need to change to issue one "make installer" command instead of a "make package" command from the top level of the mobile-browser object directory.

The assumption is that this patch will only last as long as it takes to fix the buildbots in the right way, and land the buildbot patches when someone can ensure that other buildbots are not broken.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 372173 [details] [diff] [review]
buildbot broken - dirty patch to quickly fix tinderbox builds

Promise this mess will get cleaned up in a new bug?

Comment 34

[John Wolfe (:wolfe)]

Checked in attachment 372173 [details] [diff] [review] to mobile-browser:

changeset:   474:65176fae511c
user:        John Wolfe <[e-mail]>
date:        Fri Apr 10 22:34:50 2009 -0400
summary:     Bug 476733 - WinCE Dynamic CAB INF File Production Needed - ugly patch to fix buildbot issues

We need to go back early next week and fix buildbot, then back out this changeset.

Comment 35

[John Wolfe (:wolfe)]

Bug 487896 - WinCE Buildbot Needs Updating - 

Bug filed with attachment 372180 [details] [diff] [review] for reverting nasty hack to m-b build.mk - ready for a buildbot patch to be attached by someone who knows buildbots.  

Attachment 372180 [details] [diff] leaves in change to m-b installer/makefile.in to build fennec CAB and fennec ZIP when doing a "make installer" build command from mobile and/or mobile/installer obj directories.

See bug 487896 for further information.

Comment 36

[John Wolfe (:wolfe)]

Attachment: v2.0 - buildbot broken - dirty patch to quickly fix tinderbox builds

The buildbot looks for both a .zip file and a .cab file -- but the previous incarnation of this patch just created a .zip and a .CAB file.

I did not know that the buildbot shell is case sensitive -- so this patch causes the production of a .cab file - what the buildbot is now seeking for a successful completion to the build process.

The build itself is fine, only the post-build processing is off.

Rather than trying to change the buildbots over a weekend, mfinkle and I thought that this would be a nasty, but limited patch for getting the build trees for WinCE to stop burning.

We anticipate fixing the buildbots as quickly as possible, and this code is only to suppress the burning trees until we can replace the buildbot code.

Comment 37

[John Wolfe (:wolfe)]

mobile-browser changeset:
changeset:   475:2f103ccd6961

user:        John Wolfe <[e-mail]>
date:        Sun Apr 12 20:26:39 2009 -0400
summary:     Bug 476733 - WinCE Dynamic CAB INF File Production Needed - another ugly patch to try to get the WinCE buildbots working again.

Comment 38

[Aki Sasaki (not active)]

Attachment: [checked in] update wince buildbot steps to run 'make installer'

Tested on try staging; verified that the zips + cab were generated when only running the 'make installer' step.

Comment 39

[[e-mail] (:bhearsum)]

Comment on attachment 372468 [details] [diff] [review]
[checked in] update wince buildbot steps to run 'make installer'

Seems fine

Comment 40

[Aki Sasaki (not active)]

Comment on attachment 372468 [details] [diff] [review]
[checked in] update wince buildbot steps to run 'make installer'

buildbotcustom rev 248|29c6b7dee93a
production master has been reconfig'ed to pick up this change.

Comment 41

[Brad Lassey [:blassey] (use needinfo?)]

I believe this has all landed, reopen if not