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.
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)
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.
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.
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.
Simplest way to achieve this is probably to do: GAIA_MAKE_FLAGS := $(GAIA_MAKE_FLAGS)
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.
Isn't the correctness problem only an issue if one is using recursively expanded variables? That's what we're disallowing here.
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 :) ]
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!
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?
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.
Attachment #817373 - Flags: review?(mwu)
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.