775154 - packager.mk: update {PACK,UNPACK}_OMNIJAR zip call to use response files

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

toolkit/mozapps/installer/packager.mk

extract $(OMNIJAR_FILES) and $(NON_OMNIJAR_FILES) file lists and move them into config files so external programs can gain access to the file lists (re: bug 755724).

Update{PACK,UNPACK}_OMNIJAR $(ZIP) call to use response file syntax.


also replace 'make -C' calls within packager.mk with '$(MAKE) -C'

Comment 1

[Joey Armstrong [:joey]]

Attachment: update {pack,unpack}_omnijar zip call to use response files.

Comment 2

[Joey Armstrong [:joey]]

win32 problem to check on:

zip I/O error: No such file or directory
zip error: File not found or no read permission (/e/builds/moz2_slave/try-w32-dbg/build/toolkit/mozapps/installer/omnijar/sources)

Comment 3

[Joey Armstrong [:joey]]

(In reply to Joey Armstrong [:joey] from comment #2)
> win32 problem to check on:
> 
> zip I/O error: No such file or directory
> zip error: File not found or no read permission
> (/e/builds/moz2_slave/try-w32-dbg/build/toolkit/mozapps/installer/omnijar/
> sources)

win32/win64 may not appreciate response file syntax (-i@/fyl, -x@/fyl).
Need to add a small script that will probe zip version and functionality on try.  zip version bundled with mozbuild appears to function properly for local builds.

Comment 4

[(not currently active) Ted Mielczarek]

The zip shippped with MozillaBuild is the only supported zip on Windows, FWIW. (We don't support any non-standard build configurations on Windows.)

Comment 5

[Joey Armstrong [:joey]]

Attachment: update {pack,unpack}_omnijar zip call to use response files.

Comment 6

[Joey Armstrong [:joey]]

Comment on attachment 650108 [details] [diff] [review]
update {pack,unpack}_omnijar zip call to use response files.

omnijar zip calls updated to use response files allowing OMNIJAR_FILES and NON_OMNIJAR_FILES file lists could be relocated outside packager.mk.

Non-existent file problem on windows fixed by a call to normalizepath.  Map UNC paths to dos.

Try job passed but re-running again w/o debug and extra makefile content removed.

Comment 7

[Joey Armstrong [:joey]]

https://tbpl.mozilla.org/?tree=Try&rev=a9088f6a1894

win32 opt failure was due to a timeout while attempting to clone a repository.

Comment 8

[Joey Armstrong [:joey]]

ping on the code review

Comment 9

[Joey Armstrong [:joey]]

Comment on attachment 650108 [details] [diff] [review]
update {pack,unpack}_omnijar zip call to use response files.

Patch refresh.  Re-try job running.

move omnijar file lists outside packager.mk and update PACK/UNPACK zip calls to use response files.

Comment 10

[Joey Armstrong [:joey]]

https://tbpl.mozilla.org/?tree=Try&rev=e5016d75fa7c

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 650108 [details] [diff] [review]
update {pack,unpack}_omnijar zip call to use response files.

Review of attachment 650108 [details] [diff] [review]:
-----------------------------------------------------------------

I assumed that you've tested this, and zip supports these files on all the platforms we care about, especially Windows.

::: toolkit/mozapps/installer/omnijar/excludes
@@ +1,2 @@
> +chrome/icons/*
> +$(PREF_DIR)/channel-prefs.js

No point in putting this here, since $(PREF_DIR) doesn't get expanded.

::: toolkit/mozapps/installer/packager.mk
@@ +485,5 @@
>  
>  # defaults/pref/channel-prefs.js is handled separate from other prefs due to
>  # bug 756325
> +# todo: PREF_DIR/key=value expansion or explicit value list pending.
> +omnijarExcl =\

Nit: Put a space between = and \.

@@ +487,5 @@
>  # bug 756325
> +# todo: PREF_DIR/key=value expansion or explicit value list pending.
> +omnijarExcl =\
> +  -x@$(ojcfg_excl) \
> +  $(foreach path,$(NON_OMNIJAR_FILES),-x $(path)) \

NON_OMNIJAR_FILES is empty now, so there's no need for this line, no?

@@ -493,5 @@
> -  chrome/icons/\* \
> -  $(PREF_DIR)/channel-prefs.js \
> -  defaults/pref/channel-prefs.js \
> -  res/cursors/\* \
> -  res/MainMenu.nib/\* \

Your tree is not up to date.  '\*/.mkdir.done' has been added to this list and needs to be added to your response file.

Comment 12

[Joey Armstrong [:joey]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #11)
> Comment on attachment 650108 [details] [diff] [review]
> update {pack,unpack}_omnijar zip call to use response files.
> 
> Review of attachment 650108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assumed that you've tested this, and zip supports these files on all the
> platforms we care about, especially Windows.
> 
> ::: toolkit/mozapps/installer/omnijar/excludes
> @@ +1,2 @@
> > +chrome/icons/*
> > +$(PREF_DIR)/channel-prefs.js
> 
> No point in putting this here, since $(PREF_DIR) doesn't get expanded.

The entry is bogus but was added as a reminder that expansion will be needed to completely move the file list into a response file.  The comment in packager.mk relates to it:

% todo: PREF_DIR/key=value expansion or explicit value list pending.

> @@ +487,5 @@
> >  # bug 756325
> > +# todo: PREF_DIR/key=value expansion or explicit value list pending.
> > +omnijarExcl =\
> > +  -x@$(ojcfg_excl) \
> > +  $(foreach path,$(NON_OMNIJAR_FILES),-x $(path)) \
> 
> NON_OMNIJAR_FILES is empty now, so there's no need for this line, no?

Since $(PREF_DIR)/channel-prefs is the only remaining entry yes the for loop can be removed.

> 
> @@ -493,5 @@
> > -  chrome/icons/\* \
> > -  $(PREF_DIR)/channel-prefs.js \
> > -  defaults/pref/channel-prefs.js \
> > -  res/cursors/\* \
> > -  res/MainMenu.nib/\* \
> 
> Your tree is not up to date.  '\*/.mkdir.done' has been added to this list
> and needs to be added to your response file.

This is odd ?  The patch tested on try contains this edit:
https://bugzilla.mozilla.org/show_bug.cgi?id=775154