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



5 years ago
5 years ago


(Reporter: julienw, Assigned: julienw)


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



5 years ago
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.


5 years ago
Assignee: nobody → felash
Blocks: 906316

Comment 1

5 years ago
Created attachment 795015 [details] [diff] [review]
patch v1

github pull request at

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.

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 "" and let the e-mail app "include ../../".  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.
$(error This Makefile needs to be run by the root gaia makefile. Use APP=email)

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 4

5 years ago
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)

Comment 5

5 years ago
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+

Comment 9

5 years ago
Created attachment 795884 [details] [diff] [review]
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.
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: 884399

Comment 11

5 years ago
This patch will be integrated in the new patch for bug 906316.
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.