Closed Bug 908984 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → felash
Blocks: 906316
Attached patch patch v1 (obsolete) — Splinter Review
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
Attachment #795015 - Flags: review?(poirot.alex)
Attachment #795015 - Flags: review?(bugmail)
Attachment #795015 - Flags: feedback?(jlal)
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 :)
Attachment #795015 - Flags: review?(bugmail) → review+
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 on attachment 795015 [details] [diff] [review]
patch v1

Hey Yuren, if you can review this before Alexandre, please do :)
Attachment #795015 - Flags: review?(yurenju.mozilla)
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 ?
sounds good for gmake user on mac/bsd  :-)
I'll review the PR tomorrow morning since I'm leaving from office.
Comment on attachment 795015 [details] [diff] [review]
patch v1

Looks good.
Attachment #795015 - Flags: review?(yurenju.mozilla) → review+
Attached patch patch v2Splinter Review
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.
Attachment #795015 - Attachment is obsolete: true
Attachment #795015 - Flags: review?(poirot.alex)
Attachment #795015 - Flags: feedback?(jlal)
Attachment #795884 - Flags: review+
Doesn't impact mainline dogfooding per recent smoketests.
No longer blocks: b2g-central-dogfood
This patch will be integrated in the new patch for bug 906316.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.