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

RESOLVED WORKSFORME

Status

RESOLVED WORKSFORME
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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.
(Assignee)

Updated

5 years ago
Assignee: nobody → felash
Blocks: 906316
(Assignee)

Comment 1

5 years ago
Created attachment 795015 [details] [diff] [review]
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
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.
(Assignee)

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)
(Assignee)

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+
(Assignee)

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
(Assignee)

Comment 11

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