Closed Bug 1413232 Opened 7 years ago Closed 7 years ago

move the default value of FILES_PER_UNIFIED_FILE to a more obvious location

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file)

From #build this morning:

9:24 AM <jwatt> can FILES_PER_UNIFIED_FILE be set in your .mozconfig somehow?
9:25 AM <jwatt> looking at https://dxr.mozilla.org/mozilla-central/search?q=FILES_PER_UNIFIED_FILE
9:25 AM <@ted> no
9:25 AM <jwatt> on the surface of it it looks like only dom/media and js/src are build unified
9:25 AM <jwatt> which is obviously not the case
9:25 AM <@ted> there's a default value for it
9:26 AM <jwatt> since the root moz.build sets its value to 1
9:27 AM <jwatt> ted: yup, indeed - I'm saying I don't see where that's set
9:27 AM <froydnj> the default lives here https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/data.py#902
9:27 AM <froydnj> this is a pretty poor place for it
9:27 AM <@ted> indeed
9:27 AM <@ted> but there it is
9:27 AM <froydnj> should really get it moved to emitter.py, so we can do something like context.get('FILES_PER_UNIFIED_FILE', 16)
9:27 AM <@ted> jwatt: moz.build values don't get propogated to other moz.build files (unless you explicitly export them)
9:28 AM <jwatt> froydnj: thanks, and yes, that'd be nice
9:28 AM <@ted> froydnj: that would have been my guess for where it would be
Putting it in emitter.py increases the obviousness of its default value
when searching for things.
Attachment #8923872 - Flags: review?(ted)
Attachment #8923872 - Flags: review?(ted) → review?(core-build-config-reviews)
Comment on attachment 8923872 [details] [diff] [review]
move the default value for FILES_PER_UNIFIED_FILE to a more logical location

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

I agree that this is more obvious, but it's really relying on there being no mistakes in the Python -- let's hope `UnifiedSources` is never instantiated elsewhere.  Keyword arguments and error checking in the constructor would be more robust.  Next time!
Attachment #8923872 - Flags: review?(core-build-config-reviews) → review+
I don't know what the interaction is, but it seems there are still a couple of "hidden" default values here:

https://dxr.mozilla.org/mozilla-central/search?q=files_per_unified_file+path%3Acommon.py

Are they still required?
Also the setting of FILES_PER_UNIFIED_FILE in the root moz.build has no effect, right? (There are no UNIFIED_SOURCES in that moz.build.) Can that be removed, because for people who are unaware that isn't inherited it looks like this sets the default for the codebase.
There is an export() in moz.build that can be used to have child moz.build inherit variable values. I'm not a fan of this primitive because it will undermine our ability to evaluate moz.build files concurrently. I'd rather we set a default variable value via configure and enable individual moz.build to override it.
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #3)
> I don't know what the interaction is, but it seems there are still a couple
> of "hidden" default values here:
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=files_per_unified_file+path%3Acommon.py
> 
> Are they still required?

Yes.  We could define the 16 value somewhere, and have those places key off of it, I suppose.  Or we could be really clever and teaching the IPDL/WebIDL bits to pass their generated files into the same mechanism used for UNIFIED_SOURCES.  The former is easy, the latter is relatively difficult.

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #4)
> Also the setting of FILES_PER_UNIFIED_FILE in the root moz.build has no
> effect, right? (There are no UNIFIED_SOURCES in that moz.build.) Can that be
> removed, because for people who are unaware that isn't inherited it looks
> like this sets the default for the codebase.

Yes, that can be removed.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5434af4989
move the default value for FILES_PER_UNIFIED_FILE to a more logical location; r=nalexander
https://hg.mozilla.org/mozilla-central/rev/de5434af4989
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: