Closed Bug 1032615 Opened 8 years ago Closed 8 years ago

Build fails using zsh to build geckoview_resources.zip.

Categories

(Firefox for Android Graveyard :: General, defect)

All
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: ckitching, Assigned: ckitching)

Details

Attachments

(1 file)

When building using zsh, the following error is encountered after Proguard completes:

../../../dist/bin/nsinstall: cannot access geckoview_resources.zip: No such file or directory

The file does not exist when the libs target runs, despite being marked as a dependency.

The rule for building geckoview_resources.zip is:

# We delete the archive before updating so that resources removed from
# the filesystem are removed from the archive.
geckoview_resources.zip: $(all_resources) $(GLOBAL_DEPS)
	$(REPORT_BUILD)
	$(RM) -rf $@
	$(foreach dir,$(ANDROID_RES_DIRS),$(call zip_directory_with_relative_paths,$(CURDIR)/$@,$(dir)))


Under zsh, the removal works, but the command to construct it silently fails.
The zip command expands to something like:

zip -q /home/chris/Fennec/Fennec/objdir-droid/mobile/android/base/geckoview_resources.zip -r * -x *.mkdir.done* *.DS_Store* *#* *.rej *.orig

The asterisks in the argument list to -x should be escaped, else the shell may attempt to expand them.
This seems to be a recommended approach - from the `zip' manpage:

       -x files
       --exclude files
              Explicitly exclude the specified files, as in:

                     zip -r foo foo -x \*.o

              which  will  include  the  contents of foo in foo.zip while excluding all the files that end in .o.  The backslash avoids the shell filename substitution, so that the name matching is performed by zip at all directory levels.


I hadn't realised zsh was used by so few people for this to have not yet been noticed!
A simple patch to fix the problem. You'll probably want a neater solution, though hopefully this concisely expresses precisely where the problem is.
Attachment #8448450 - Flags: review?(nalexander)
Comment on attachment 8448450 [details] [diff] [review]
Prevent wildcard expansion from killing zsh.

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

I'm a little bit surprised that we're hitting shell incompatibilities, but obviously, you are :)  I'd like to test against sh/bash just to be sure -- you could test the zip command manually -- and then bombs away.

::: mobile/android/base/Makefile.in
@@ +279,5 @@
>  
>  # $(1): zip file to add to (or create).
>  # $(2): directory to zip contents of.
>  define zip_directory_with_relative_paths
> +cd $(2) && zip -q $(1) -r * -x $(subst *, \\*, $(not_android_res_files))

Whitespace counts, so that should be

... (subst *,\\*,$(not_android_res_files))

(no extra spaces).  You could also add a new, hard-coded, not_android_res_files_patterns or similar.
Attachment #8448450 - Flags: review?(nalexander) → review+
Assignee: nobody → chriskitching
Status: NEW → ASSIGNED
(In reply to Nick Alexander :nalexander from comment #2)
> I'm a little bit surprised that we're hitting shell incompatibilities, but
> obviously, you are :)  I'd like to test against sh/bash just to be sure --
> you could test the zip command manually -- and then bombs away.

Already did so for bash/zsh. Tried it also on dash and fish to make sure.

> Whitespace counts, so that should be...

Whoops.

Anyhoo, landed:
https://hg.mozilla.org/integration/fx-team/rev/767d3a8e6491


... Now I can actually build at last. :P
https://hg.mozilla.org/mozilla-central/rev/767d3a8e6491
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.