Closed Bug 775154 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: joey, Assigned: joey)

References

Details

Attachments

(1 file, 1 obsolete file)

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'
Assignee: nobody → joey
See Also: → metro-build
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)
(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.
The zip shippped with MozillaBuild is the only supported zip on Windows, FWIW. (We don't support any non-standard build configurations on Windows.)
Attachment #643431 - Attachment is obsolete: true
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.
Attachment #650108 - Flags: review?(ted.mielczarek)
https://tbpl.mozilla.org/?tree=Try&rev=a9088f6a1894

win32 opt failure was due to a timeout while attempting to clone a repository.
ping on the code review
Attachment #650108 - Flags: review?(ted.mielczarek)
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.
Attachment #650108 - Flags: review?(khuey)
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.
Attachment #650108 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.