Closed Bug 543038 Opened 10 years ago Closed 10 years ago

pymake can't build mobile-browser

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: blassey, Assigned: blassey)

Details

Attachments

(1 file, 1 obsolete file)

it currently chokes on calling redit, saying the command returns 127

http://mxr.mozilla.org/mobile-browser/source/app/Makefile.in#150 

and on our search plugins packaging:

/bin/bash: dpkg-architecture: command not found
/bin/bash: -c: line 0: syntax error near unexpected token `('
/bin/bash: -c: line 0: `for line in "locale/en-US/browser/searchplugins/amazondotcom.xml ($(plugin).xml "locale/en-US/br
owser/searchplugins/google.xml ($(plugin).xml "locale/en-US/browser/searchplugins/twitter.xml ($(plugin).xml "locale/en-
US/browser/searchplugins/wikipedia.xml ($(plugin).xml"); do \'
c:\mozilla-central\objdir-wm-standalone\mobile\locales\Makefile:76:0: command 'for line in "locale/en-US/browser/searchp
lugins/amazondotcom.xml ($(plugin).xml "locale/en-US/browser/searchplugins/google.xml ($(plugin).xml "locale/en-US/brows
er/searchplugins/twitter.xml ($(plugin).xml "locale/en-US/browser/searchplugins/wikipedia.xml ($(plugin).xml"); do \
    echo " $line" >> tmp-search.jar.mn; \
done' failed, return code 258

http://mxr.mozilla.org/mobile-browser/source/locales/Makefile.in#73

there may be other failure points
For that particular case, try replacing it with:
$(foreach plugin,$(SEARCH_PLUGINS),echo "locale/$(AB_CD)/browser/searchplugins/$(notdir $(plugin)).xml ($(plugin).xml)" >> tmp-search.jar.mn;)

This will still result in a shell invocation in pymake, but it's not terrible. The alternative looks like this:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#149

so something like:
define ADD_SEARCH_PLUGIN
echo "$1" >> $2

endef # do not remove the blank line!

searchplugins::
         @echo "$(AB_CD).jar:" > tmp-search.jar.mn
         $(foreach plugin,$(SEARCH_PLUGINS),$(call,ADD_SEARCH_PLUGIN,locale/$(AB_CD)/browser/searchplugins/$(notdir $(plugin)).xml ($(plugin).xml),tmp-search.jar.mn))

This should not result in shell invocation in pymake.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bugmail
Attachment #429924 - Flags: review?(mark.finkle)
Comment on attachment 429924 [details] [diff] [review]
patch

>diff --git a/app/Makefile.in b/app/Makefile.in
>--- a/app/Makefile.in
>+++ b/app/Makefile.in
>-	$(REDIT_PATH)/redit $(DIST)/bin/$(APP_BINARY) $(srcdir)/$(APP_ICON).ico
>+	$(shell $(REDIT_PATH)/redit $(DIST)/bin/$(APP_BINARY) $(srcdir)/$(APP_ICON).ico)

You sure about this? This will run this command (possibly at makefile parse time), then execute the stdout as a shell command in the rule.


>diff --git a/locales/Makefile.in b/locales/Makefile.in
>--- a/locales/Makefile.in
>+++ b/locales/Makefile.in
>@@ -61,29 +61,30 @@ SUBMAKEFILES += \
>-searchplugins::
>-	@echo "$(AB_CD).jar:" > tmp-search.jar.mn
>-	@for line in $(foreach plugin,$(SEARCH_PLUGINS),"locale/$(AB_CD)/browser/searchplugins/$(notdir $(plugin)).xml ($(plugin).xml)"); do \
>-	    echo " $$line" >> tmp-search.jar.mn; \
>-	done
>+tmp-search.jar.mn::
>+	@echo -n "$(AB_CD).jar:" > $@
>+	printf "$(foreach plugin,$(SEARCH_PLUGINS),$(subst __PLUGIN_SUBST__,$(plugin), \n locale/$(AB_CD)/browser/searchplugins/__PLUGIN_SUBST__.xml (__PLUGIN_SUBST__.xml)))" >>  $@
>+	@echo   >> $@

Did my suggestions above not work? This seems really complicated, and it still doesn't save you a shell invocation (since you have redirection there, pymake needs to invoke sh).
(In reply to comment #3)
> (From update of attachment 429924 [details] [diff] [review])
> >diff --git a/app/Makefile.in b/app/Makefile.in
> >--- a/app/Makefile.in
> >+++ b/app/Makefile.in
> >-	$(REDIT_PATH)/redit $(DIST)/bin/$(APP_BINARY) $(srcdir)/$(APP_ICON).ico
> >+	$(shell $(REDIT_PATH)/redit $(DIST)/bin/$(APP_BINARY) $(srcdir)/$(APP_ICON).ico)
> 
> You sure about this? This will run this command (possibly at makefile parse
> time), then execute the stdout as a shell command in the rule.

perhaps this worked for me because it wasn't a clobber build.  Got any suggestions?

> Did my suggestions above not work? This seems really complicated, and it still
> doesn't save you a shell invocation (since you have redirection there, pymake
> needs to invoke sh).

I couldn't get your code to work, did you test it?
Can you show me the exact error that redit line gives?

I tested my code in this sample Makefile:
ALL = 1 2 3 4 5

$(shell rm a)
a:
	$(foreach x,$(ALL),echo $(strip $(x)) >> $@;)

invoking gnu make shows:
make: Entering directory `/tmp/foo'
echo 1 >> a; echo 2 >> a; echo 3 >> a; echo 4 >> a; echo 5 >> a;
make: Leaving directory `/tmp/foo'

and the file 'a' contains the expected output. (using pymake produces the same output.)
redit failure:

c:\mozilla-central\objdir-x86\mobile\app\Makefile:195:0$ c:/mozilla-central/objdir-x86/dist/bin/redit ../../dist/bin/fen
nec.exe ../../../mobile/app/mobile.ico
[Error 2] The system cannot find the file specified
c:\mozilla-central\objdir-x86\mobile\app\Makefile:195:0: command 'c:/mozilla-central/objdir-x86/dist/bin/redit ../../dis
t/bin/fennec.exe ../../../mobile/app/mobile.ico' failed, return code -127
c:\mozilla-central\config\rules.mk:736:0: command 'c:/mozilla-build/python25/python.exe c:/pymake/make.py libs' failed,
return code 2


> Did my suggestions above not work? This seems really complicated, and it still
> doesn't save you a shell invocation (since you have redirection there, pymake
> needs to invoke sh).
btw... do you think its overly complicated because of the subst?  That is to work around an apparent bug in pymake that it only substitutes the first occurrence of $(plugin). This works with regular make bug fails for pymake:

printf "$(foreach plugin,$(SEARCH_PLUGINS), \n locale/$(AB_CD)/browser/searchplugins/$(plugin).xml ($(plugin).xml))" >>  $@
For the record, we sorted out the redit failure, it needs a $(BIN_SUFFIX) or $(HOST_BIN_SUFFIX).
Attached patch patch v.2Splinter Review
this also has a fix for a build bustage on windows desktop
Attachment #429924 - Attachment is obsolete: true
Attachment #430109 - Flags: review?(mark.finkle)
Attachment #429924 - Flags: review?(mark.finkle)
Attachment #430109 - Flags: review?(mark.finkle) → review+
For the record, at some point we might end up doing similar code for Firefox for searchplugins.

Blunt question, should we just code that piece of tmp-search.jar.mn creation in a python script?
That'd also be fine.
pushed http://hg.mozilla.org/mobile-browser/rev/47b658f2a87d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
echo -n isn't cross-platform. I don't remember which platform busted on that, but I do recall having to patch that in favor of printf, IIRC.
I think it's Mac, actually.

Brad: just replace the "echo -n" with printf. (printf doesn't put a newline in by default.)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.