Permit additional arguments to be added to GAIA_MAKE_FLAGS from outside gaia/Android.mk

RESOLVED WORKSFORME

Status

Firefox OS
General
RESOLVED WORKSFORME
5 years ago
5 years ago

People

(Reporter: m1, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Currently gaia/Android.mk forcibly clears GAIA_MAKE_FLAGS with a |GAIA_MAKE_FLAGS :=|.  

This doesn't seem to be necessary, and actively prevents additional GAIA_MAKE_FLAGS from being added by product level makefiles.
(Reporter)

Comment 1

5 years ago
Created attachment 817373 [details] [diff] [review]
0001-Bug-927087-Permit-additions-to-GAIA_MAKE_FLAGS-from-.patch
Assignee: nobody → mvines
Attachment #817373 - Flags: review?(fabrice)
Comment on attachment 817373 [details] [diff] [review]
0001-Bug-927087-Permit-additions-to-GAIA_MAKE_FLAGS-from-.patch

Review of attachment 817373 [details] [diff] [review]:
-----------------------------------------------------------------

redirecting to mwu since I don't know much about our usage of these flags.
Attachment #817373 - Flags: review?(fabrice) → review?(mwu)

Comment 3

5 years ago
My only concern here is that ensuring GAIA_MAKE_FLAGS is a simply expanded variable rather than recursively expanded. Removing this turns GAIA_MAKE_FLAGS into a recursively expanded variable when GAIA_MAKE_FLAGS isn't defined, which is often the case.
(Reporter)

Comment 4

5 years ago
I don't see how that could cause any problems given the current usage of GAIA_MAKE_FLAGS, and if it does in the future than I wonder what type of broken thing we are trying to shim in at that time.

Comment 5

5 years ago
It's not a problem, but it's best to stick to simply expanded variables when possible. It's a performance concern rather than correctness concern.

Comment 6

5 years ago
Simplest way to achieve this is probably to do:

GAIA_MAKE_FLAGS := $(GAIA_MAKE_FLAGS)
(Reporter)

Comment 7

5 years ago
In the worst case we append to GAIA_MAKE_FLAGS trice:
1. https://github.com/mozilla-b2g/gaia/blob/master/Android.mk#L27
2. https://github.com/mozilla-b2g/gaia/blob/master/Android.mk#L34
3. https://github.com/mozilla-b2g/gaia/blob/master/Android.mk#L38
and we build this thing on machines with 100GB of RAM and 128 cores.   There is no performance concern here.

Forcing the macro to be simply expanded like comment 6 proposes may actually cause correctness problems in certain cases, depending on the order in which gaia/Android.mk is imported relative to other .mk files.

Comment 8

5 years ago
Isn't the correctness problem only an issue if one is using recursively expanded variables? That's what we're disallowing here.
(Reporter)

Comment 9

5 years ago
Is an example of where we would have a correctness problem by forcing the macro to be simply expanded:

GAIA_MAKE_FLAGS = $(SOMETHING_TO_FIGURE_OUT_LATER)
import gaia/Android.mk
SOMETHING_TO_FIGURE_OUT_LATER=bar

Chances of this ever happening are admittedly slim and don't look now but I'm bikeshedding.  My main concern with comment 6 is that it's superfluous, so now we have something in the makefile that looks like it does something very important to folks who don't make very well, but it really doesn't.  

[ On the other hand, I don't care enough to not add comment 6 if that's simply what it'll take to get an r+ around these parts :) ]
(Reporter)

Comment 10

5 years ago
So do I need to add |GAIA_MAKE_FLAGS := $(GAIA_MAKE_FLAGS)| to get your r+?  I continue to believe this is unnecessary and arguably harmful, so I'll only add it if that's the only way to get this landed.  LMK!
Flags: needinfo?(mwu)

Comment 11

5 years ago
The suggestion in comment 6 isn't the only way. I think that the problem here is that we're mixing build configuration which is done by product files with the actual build machinery which is done by Android.mk files. This then makes it impossible to do the right and simple thing. If we move this logic out to a gaia.mk file (like gonk-misc/b2g.mk), we'll be able to eliminate all ordering problems with these variables. What do you think?
Flags: needinfo?(mwu)
(Reporter)

Comment 12

5 years ago
Yeah I like the idea of a gaia.mk and/or putting this in gonk-misc/b2g.mk if a stand-alone gaia.mk doesn't hold enough weight on it's own.
(Reporter)

Updated

5 years ago
Attachment #817373 - Flags: review?(mwu)
(Reporter)

Comment 13

5 years ago
I've accomplished what I needed to do without fixing this bug, so WORKSFORME.
Assignee: mvines → nobody
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.