908984 - follow-up to bug 906316: submake launches need to know the new directory too

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Gaia, defect)

Description

[Julien Wajsberg [:julienw]]

Bug #908580 was a symptom of this problem and fixed it only partly: submakes uses XULrunner too and they need to know it is.

For instance, in bug 908698, TBPL redefines the Xulrunner directory, and therefore the email makefile will fail there too.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

github pull request at https://github.com/mozilla-b2g/gaia/pull/11738

In this patch, we export the new XULRUNNER_DIRECTORY variable along with the
existing various xulrunner program paths. These paths have been made absolute
using make's `abspath` operation.

The patch also removes the ./ prefixes to those paths as they're likely useless
and prevent from using an absolute path when defining XULRUNNER_DIRECTORY from
the command line.
---
 Makefile            |   12 +++++++-----
 apps/email/Makefile |   15 ---------------
 2 files changed, 7 insertions(+), 20 deletions(-)

asking review from Alexandre, Andrew for the email bit, and I'd like to have James feedback too.

Especially, I'd like someone to test this on MacOS as I don't have that here, and file path operations can bring issues.

Thanks

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 795015 [details] [diff] [review]
patch v1

This seems like a good idea to avoid having our sub-Makefile try and figure things out that the top-level Makefile knows.  Another alternative would be to create an "xulrunner.mk" and let the e-mail app "include ../../xulrunner.mk".  I think the increase in complexity is unlikely to be worth the cost, especially since we already can do APP=email in the root Makefile.

So I suggest we add the following to the top:

=== cut here ===
# We can't figure out XULRUNNERSDK on our own; it's complex and some builders
# may want to override our find logic (ex: TBPL), so let's just leave it up to
# the root Makefile.  If you know what you're doing, you can manually define
# XULRUNNERSDK and XPCSHELLSDK on the command line.
ifndef XULRUNNERSDK
$(error This Makefile needs to be run by the root gaia makefile. Use APP=email)
endif
===

r=asuth with that change, assuming it works :)

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

James Burke also setup the Makefile immediately, so he might have some thoughts.

Oh, and let me revise my r=asuth to be something along the lines of what I propose.  Mainly I want us to error out if you try and run the Makefile from within our subdir, and explain a little context so we're never tempted to copy the logic back down into our Makefile again.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 795015 [details] [diff] [review]
patch v1

Hey Yuren, if you can review this before Alexandre, please do :)

Comment 5

[Julien Wajsberg [:julienw]]

Andrew> I think I'll use your code as-is, thanks !
(the other possibility is to check for this in the target using bash, which I find less readable)

Alexandre, Yuren> I'd like to replace "make" with "$(MAKE)" in the "app-makefiles" target, so that it will just work for people that use "gmake" instead of "make". WHat do you think ?

Comment 6

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

sounds good for gmake user on mac/bsd  :-)

Comment 7

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

I'll review the PR tomorrow morning since I'm leaving from office.

Comment 8

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 795015 [details] [diff] [review]
patch v1

Looks good.

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Carrying r=yurenju,asuth

Uses $(MAKE) instead of "make" when calling submake in the apps (I decided to not change the other "make" call, as I don't know what it's for).

Will try this patch on Windows and MacOS X before landing.

Comment 10

[Jason Smith [:jsmith]]

Doesn't impact mainline dogfooding per recent smoketests.

Comment 11

[Julien Wajsberg [:julienw]]

This patch will be integrated in the new patch for bug 906316.