870366 - Move PREF_JS_EXPORTS to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Joey Armstrong [:joey]]

Attachment: logic: move PREF_JS_EXPORTS logic to moz.build

Comment 2

[Joey Armstrong [:joey]]

Attachment: logic: move PREF_JS_EXPORTS logic to moz.build

Comment 3

[Joey Armstrong [:joey]]

Attachment: logic: move PREF_JS_EXPORTS logic to moz.build

Comment 4

[Joey Armstrong [:joey]]

Comment on attachment 748960 [details] [diff] [review]
logic: move PREF_JS_EXPORTS logic to moz.build

Add PREF_JS_EXPORT as a passthrough variable for future conversion.

Dictionary iterations from the ASFILES patch used for initialization and testing included.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 748960 [details] [diff] [review]
logic: move PREF_JS_EXPORTS logic to moz.build

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +86,5 @@
> +            XPIDLSRCS='XPIDL_SOURCES',
> +            )
> +        for mak,moz in varmap.items():
> +            if sandbox[moz]:
> +                passthru.variables[mak] = sandbox[moz]

Didn't you make this patch in the ASFILES bug? Why am I seeing it again?

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +83,5 @@
> +    'PREF_JS_EXPORTS': (list, [],
> +        """ Javascript preferences
> +
> +        A list of javascript files to 'export'.  Export action is currently
> +        defined as copying files beneath $(DIST)/.

How about a better name? JS_PREFERENCE_FILES? Maybe a better description too. "export" doesn't really say that much.

Comment 6

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #5)
> Comment on attachment 748960 [details] [diff] [review]
> logic: move PREF_JS_EXPORTS logic to moz.build
> 
> Review of attachment 748960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +86,5 @@
> > +            XPIDLSRCS='XPIDL_SOURCES',
> > +            )
> > +        for mak,moz in varmap.items():
> > +            if sandbox[moz]:
> > +                passthru.variables[mak] = sandbox[moz]
> 
> Didn't you make this patch in the ASFILES bug? Why am I seeing it again?

Yes the logic is in the ASFILES patch.  This patch will probably be going in sooner so submitting with the final answer and will merge duplication out the one following landing.
 
> ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
> @@ +83,5 @@
> > +    'PREF_JS_EXPORTS': (list, [],
> > +        """ Javascript preferences
> > +
> > +        A list of javascript files to 'export'.  Export action is currently
> > +        defined as copying files beneath $(DIST)/.
> 
> How about a better name? JS_PREFERENCE_FILES? Maybe a better description
> too. "export" doesn't really say that much.

The variable can be renamed but I'm mostly pushing bulk conversion at this point to get conversions out of the way.  The plan was to open followup bugs to cleanup, swap variables around in conditionals, prune stray conversion whitespace from moz.build files, etc.  Descriptive commentary can be added to the list of items.

Comment 7

[Joey Armstrong [:joey]]

Attachment: logic portion of PREF_JS_EXPORTS conversion to moz.build

Morphed from a variable passthrough conversion into named functions.
Variable renamed from PREF_JS_EXPORTS to JS_PREFERENCES per one of the nits.

Comment 8

[Joey Armstrong [:joey]]

Attachment: makefile/config portion of PREF_JS_EXPORTS move to mozbuild.

/locales/Makefile.in conversion will be handled in another bug.
PREF_JS_EXPORTS = $(call MERGE_FILE,firefox-l10n.js)

Comment 9

[Joey Armstrong [:joey]]

Attachment: move PREF_JS_EXPORTS to mozbuild (logic portion).

Rebase, remove logic cloned from ASFILES conversion, that patch landed on inbound so no longer needed.  mozbuild flavor of the variable renamed to JS_PREFERENCE_FILES per the earlier feedback request.

Comment 10

[Joey Armstrong [:joey]]

browser/app/Makefile.in may need special handling, passthrough variable not working properly.  $(PREF_JS_EXPORTS_PATH) not yet created when handled by backend.mk

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 750074 [details] [diff] [review]
move PREF_JS_EXPORTS to mozbuild (logic portion).

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +219,5 @@
>          elif isinstance(obj, Program):
>              self._process_program(obj.program, backend_file)
>  
> +        elif isinstance(obj, PreferencesJS):
> +            self._process_preferences_js(obj.preferences_js, backend_file)

I'd just write it out without requiring an extra function call. Function calls are relatively expensive in Python and inlining single line functions with only one call site is a good practice.

::: python/mozbuild/mozbuild/frontend/data.py
@@ +166,5 @@
>          if not program.endswith(bin_suffix):
>              program += bin_suffix
>          self.program = program
>  
> +class PreferencesJS(SandboxDerived):

JSPreferencesFile is a better class name IMO.

@@ +169,5 @@
>  
> +class PreferencesJS(SandboxDerived):
> +    """Build object container for (was PREF_JS_EXPORTS).
> +
> +    This object contains a list javascript preference files.

The first line should probably be replaced by the second. This file doesn't care what PREF_JS_EXPORTS is!

Also, let's only store a single path in each instance unless there is a reason for a single instance to describe multiple paths.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +101,5 @@
>          if program:
>              yield Program(sandbox, program, sandbox['CONFIG']['BIN_SUFFIX'])
>  
> +        for src in sandbox.get('JS_PREFERENCE_FILES', []):
> +            yield PreferencesJS(sandbox, src)

Oh, so you do emit one object per source file. I guess the description in data.py is wrong.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +86,5 @@
>          likely go away.
>          """),
>  
> +    'JS_PREFERENCE_FILES': (list, [],
> +        """ Exported javascript files (formerly PREF_JS_EXPORTS)

Nit: no space after """

You should add a note in here that the destination could be either GRE or app depending on...

Also, no need to mention PREF_JS_EXPORTS in the main summary line.

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py
@@ +194,5 @@
> +
> +        inis = []
> +        for o in objs:
> +            if isinstance(o, PreferencesJS):
> +                inis.append(o.preferences_js)

inis = [o.preferences_js for o in objs if isinstance(o, PreferencesJS)]

Comment 12

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #11)
> Comment on attachment 750074 [details] [diff] [review]
> move PREF_JS_EXPORTS to mozbuild (logic portion).
> 
> Review of attachment 750074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +219,5 @@
> >          elif isinstance(obj, Program):
> >              self._process_program(obj.program, backend_file)
> >  
> > +        elif isinstance(obj, PreferencesJS):
> > +            self._process_preferences_js(obj.preferences_js, backend_file)
> 
> I'd just write it out without requiring an extra function call. Function
> calls are relatively expensive in Python and inlining single line functions
> with only one call site is a good practice.

For maintainability I think it would make a lot of sense to have named functions/callout point for each variable here.  Over time they are not likely to remain as a single write call so just build in extensibility from the start.

Also the PROGRAM= variable added was already following this convention.


> ::: python/mozbuild/mozbuild/frontend/data.py
> @@ +166,5 @@
> >          if not program.endswith(bin_suffix):
> >              program += bin_suffix
> >          self.program = program
> >  
> > +class PreferencesJS(SandboxDerived):
> 
> JSPreferencesFile is a better class name IMO.

Was trying to account for adding several preferenc* functions but since this is the first yes that name would match up better with the external names.


> @@ +169,5 @@
> >  
> > +class PreferencesJS(SandboxDerived):
> > +    """Build object container for (was PREF_JS_EXPORTS).
> > +
> > +    This object contains a list javascript preference files.
> 
> The first line should probably be replaced by the second. This file doesn't
> care what PREF_JS_EXPORTS is!

Until backend.mk is deprecated or references to the old var are removed from rules.mk I think it makes sense referencing old names here and in sandbox_*.py so people have a signpost to help find where the variables within backend.mk are being used within mozbuild.

> Also, let's only store a single path in each instance unless there is a
> reason for a single instance to describe multiple paths.
> 
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +101,5 @@
> >          if program:
> >              yield Program(sandbox, program, sandbox['CONFIG']['BIN_SUFFIX'])
> >  
> > +        for src in sandbox.get('JS_PREFERENCE_FILES', []):
> > +            yield PreferencesJS(sandbox, src)
> 
> Oh, so you do emit one object per source file. I guess the description in
> data.py is wrong.
> 
> ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
> @@ +86,5 @@
> >          likely go away.
> >          """),
> >  
> > +    'JS_PREFERENCE_FILES': (list, [],
> > +        """ Exported javascript files (formerly PREF_JS_EXPORTS)
> 
> Nit: no space after """
> 
> You should add a note in here that the destination could be either GRE or
> app depending on...

Ok, to be documented when the actual var conversions begin.


> ::: python/mozbuild/mozbuild/test/frontend/test_emitter.py
> @@ +194,5 @@
> > +
> > +        inis = []
> > +        for o in objs:
> > +            if isinstance(o, PreferencesJS):
> > +                inis.append(o.preferences_js)
> 
> inis = [o.preferences_js for o in objs if isinstance(o, PreferencesJS)]

good refactor

Comment 13

[Joey Armstrong [:joey]]

Attachment: move PREF_JS_EXPORTS to moz.build (logic only).

Comment 14

[Joey Armstrong [:joey]]

Comment on attachment 762219 [details] [diff] [review]
move PREF_JS_EXPORTS to moz.build (logic only).

nit fixes, inlined backend_write() call to avoid using a subroutine.  Changed *js* name to js_preference_files to be consistent.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 762219 [details] [diff] [review]
move PREF_JS_EXPORTS to moz.build (logic only).

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

Looks mostly good. Please address first 2 inline comments.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +170,5 @@
>              self._process_program(obj.program, backend_file)
>  
> +        elif isinstance(obj, JSPreferenceFiles):
> +            backend_file.write('PREF_JS_EXPORTS = %s\n' % obj.js_preference_files)
> +            backend_file.write('\n') # intentional: workaround until buffering problem fixed.

Buffering problem?! Please detail!

::: python/mozbuild/mozbuild/frontend/data.py
@@ +175,5 @@
> +    __slots__ = ('js_preference_files')
> +
> +    def __init__(self, sandbox, files):
> +        SandboxDerived.__init__(self, sandbox)
> +        self.js_preference_files = files

The singular and plural names don't match up. According to emitter.py, we emit multiple instances of this. So, this object really represents a single file, not multiple ones. It should be called JSPreferenceFile. The attribute name should also be singular. And, it should not be redundant with the class name. .path or .file should be fine (I prefer .path because "file" is a built-in Python symbol and I don't like re-using built-in names).

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +143,5 @@
> +    'JS_PREFERENCE_FILES': (StrictOrderingOnAppendList, list, [],
> +        """Exported javascript files.
> +
> +        A list of files copied into the dist directory for packaging and installation.
> +        Path will be defined for gre or application prefs dir based on what is building.

Yeah, we'll likely want to encode gre or application into this at a later date. Follow-up, of course.

Comment 16

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #15)
> Comment on attachment 762219 [details] [diff] [review]
> move PREF_JS_EXPORTS to moz.build (logic only).
> 
> Review of attachment 762219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks mostly good. Please address first 2 inline comments.
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +170,5 @@
> >              self._process_program(obj.program, backend_file)
> >  
> > +        elif isinstance(obj, JSPreferenceFiles):
> > +            backend_file.write('PREF_JS_EXPORTS = %s\n' % obj.js_preference_files)
> > +            backend_file.write('\n') # intentional: workaround until buffering problem fixed.
> 
> Buffering problem?! Please detail!

There is another bug on file with the details, also referenced by this bug.


> ::: python/mozbuild/mozbuild/frontend/data.py
> @@ +175,5 @@
> > +    __slots__ = ('js_preference_files')
> > +
> > +    def __init__(self, sandbox, files):
> > +        SandboxDerived.__init__(self, sandbox)
> > +        self.js_preference_files = files
> 
> The singular and plural names don't match up. According to emitter.py, we
> emit multiple instances of this. So, this object really represents a single
> file, not multiple ones. It should be called JSPreferenceFile. The attribute
> name should also be singular. And, it should not be redundant with the class
> name. .path or .file should be fine (I prefer .path because "file" is a
> built-in Python symbol and I don't like re-using built-in names).

To keep the setup simple how about using one variable form - either plural or singular throughout ?  Yes for accuracy there is a single value available from emitter and a list of values from the input source so both types are in play.

But having to bounce back and forth between case use can easily become a failure point for typos.  Just use a single value consistently throught from variable declaration to internal object storage down to testing and everything matches up nicely and is a lot easier to maintain.

Thoughts ?

Comment 17

[Gregory Szorc [:gps]]

I don't follow half of comment #16.

Use singular names for things that hold scalars. Use plural names for collections.

As I've said before, I prefer to emit multiple objects from the emitter, each representing an individual instance as opposed to emitting a single object holding a collection of similar things. e.g. when we have a list of things in moz.build, that gets emitted as N = len(list) objects, not as 1 object wrapping the list.

In this case:

class JSPreferenceFile(SandboxDerived):
    _slots__ = ('path')

Comment 18

[Gregory Szorc [:gps]]

Why the depends?

Comment 19

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #17)
> I don't follow half of comment #16.

Well in a nutshell could grep be run on python/mozbuild with a single sting and find all instances where a variable is used?  The answer currently no due to singular-vs-plural naming of variables within the code.  Depending on the variable name may have to start searching for variants.

moz.build: plural, declared as a list.
sandbox_symbols.py: plural, declared as a list.
data.py: singular, value stored in __slots__
emitter.py: plural/singular - read moz.build and output/yield single values
data.py: singular, stored value
recursivemake.py: singular, values written one line per file.
test_emitter.py, test_recursive.py: singular/plural, read singular values from a stream (ie __slot__ reference) into a list then make list comparison.

From end-to-end: config->IO->storage->output->testing

Values bounce back and forth between singular and plural case which can make it difficult to find/follow and prone to typo induced problems.

What I am proposing is consistency in naming for all references to a variable, from config until final testing, to remove the naming inconsistency when viewed from more than a single source file.
 

> Use singular names for things that hold scalars. Use plural names for
> collections.
> 
> As I've said before, I prefer to emit multiple objects from the emitter,
> each representing an individual instance as opposed to emitting a single
> object holding a collection of similar things. e.g. when we have a list of
> things in moz.build, that gets emitted as N = len(list) objects, not as 1
> object wrapping the list.
> 
> In this case:
> 
> class JSPreferenceFile(SandboxDerived):
>     _slots__ = ('path')

This may need to be revisited at some point then because not all of the examples I've been working from are following that pattern.


>> Why the depends?

Hmmm ?!? they should not be there.  Either a stray paste or byproduct of cloning

Comment 20

[Gregory Szorc [:gps]]

(In reply to Joey Armstrong [:joey] from comment #19)
> Well in a nutshell could grep be run on python/mozbuild with a single sting
> and find all instances where a variable is used?  The answer currently no
> due to singular-vs-plural naming of variables within the code.  Depending on
> the variable name may have to start searching for variants.
> 
> moz.build: plural, declared as a list.
> sandbox_symbols.py: plural, declared as a list.
> data.py: singular, value stored in __slots__
> emitter.py: plural/singular - read moz.build and output/yield single values
> data.py: singular, stored value
> recursivemake.py: singular, values written one line per file.
> test_emitter.py, test_recursive.py: singular/plural, read singular values
> from a stream (ie __slot__ reference) into a list then make list comparison.
> 
> From end-to-end: config->IO->storage->output->testing
> 
> Values bounce back and forth between singular and plural case which can make
> it difficult to find/follow and prone to typo induced problems.
> 
> What I am proposing is consistency in naming for all references to a
> variable, from config until final testing, to remove the naming
> inconsistency when viewed from more than a single source file.

We purposefully bounce between singular and plural depending on the context. That's how things work. Those are the abstraction layers we've gone with. I'm sorry, but you'll have to deal with slightly different names.

If it's any recourse, one of the reasons I want to use Python classes for representing the build config (as opposed to dicts or other anonymous data structures) is so you can grep on the class name and find both the source (in emitter.py) and consumers (in the backends).

Comment 21

[jarmstrong]

(In reply to Gregory Szorc [:gps] from comment #18)
> Why the depends?

They were a byproduct of cloning existing conversion bugs.

Comment 22

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Move PREF_JS_EXPORTS to moz.build (mozbuild logic)

I had some free time, so I updated this to the current state of the tree. I think I managed to pull in all of the earlier review issues.

Try push (for this plus the following patches):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e0966051d68

Comment 23

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Move PREF_JS_EXPORTS to moz.build

Comment 24

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2b: Move PREF_JS_EXPORTS to moz.build (l10n MERGE_FILE hack)

I'm aware this is a pretty awful looking hack. It's only here to work around the MERGE_FILE stuff for l10n. I don't think a better option is possible until l10n repacks get overhauled.

Feel free to r- if you'd prefer to keep the hacks out.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8553153 [details] [diff] [review]
Part 1: Move PREF_JS_EXPORTS to moz.build (mozbuild logic)

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +446,5 @@
>          elif isinstance(obj, Resources):
>              self._process_resources(obj, obj.resources, backend_file)
>  
> +        elif isinstance(obj, JsPreferenceFile):
> +            if (obj.path.startswith('/')):

No parens

Comment 26

[Brian O'Keefe [:bokeefe]]

Attachment: Part 3: Prohibit PREF_JS_EXPORTS in Makefile.ins

Obviously, if Part 2b is r-'ed, this can't land.

Comment 27

[Brian O'Keefe [:bokeefe]]

Comment on attachment 8553153 [details] [diff] [review]
Part 1: Move PREF_JS_EXPORTS to moz.build (mozbuild logic)

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

I'll post a new patch shortly, assuming I can get comm-try green.

::: python/mozbuild/mozbuild/frontend/context.py
@@ +652,5 @@
> +        """Exported javascript files.
> +
> +        A list of files copied into the dist directory for packaging and installation.
> +        Path will be defined for gre or application prefs dir based on what is building.
> +        """, None),

This should be 'libs' - it doesn't make a difference in m-c (yet), but it breaks c-c.

Comment 28

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Move PREF_JS_EXPORTS to moz.build (mozbuild logic)

With both Ms2ger's and my 'libs' issues addressed

Comment 29

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2b: Move PREF_JS_EXPORTS to moz.build (l10n MERGE_FILE hack)

Rebased because of the Part 1 update

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8553339 [details] [diff] [review]
Part 2b: Move PREF_JS_EXPORTS to moz.build (l10n MERGE_FILE hack)

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

I'm going to defer this to glandium since he's doing l10n foo and his opinion counts more than mine.

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8553164 [details] [diff] [review]
Part 3: Prohibit PREF_JS_EXPORTS in Makefile.ins

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

This obviously depends on the outcome of the previous patch. But this part is good.

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 8553339 [details] [diff] [review]
Part 2b: Move PREF_JS_EXPORTS to moz.build (l10n MERGE_FILE hack)

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +452,5 @@
> +                # MERGE_FILES to pick the right set of preferences. This should
> +                # be able to go away once L10N builds are done differently (see
> +                # bug 1107628).
> +                backend_file.write('PREF_JS_EXPORTS += %s\n' % obj.path)
> +            elif obj.path.startswith('/'):

if obj.path.startswith('/'):
elif obj.path.startswith('/'):

One of those branches is never going to happen. I guess you meant $ for the first one.

That said, I'd rather not do this*: what's the point of moving stuff to moz.build if all they're doing is a hack to get the same value that was in Makefile.in in backend.mk...

* unless you *also* deprecate PREF_JS_EXPORTS such that defining it in Makefile.in throws an error. And even then, I /think/ I'd rather just change PREF_JS_EXPORTS to something else in those few locales files, like:

L10N_PREF_JS_EXPORTS_PATH = $(FINAL_TARGET)/$(PREF_DIR)
L10N_PREF_JS_EXPORTS_FLAGS = $(PREF_PPFLAGS) --silence-missing-directive-warnings
PP_TARGETS += L10N_PREF_JS_EXPORTS

Comment 33

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2b: Use PP_TARGETS instead of PREF_JS_EXPORTS for l10 prefs

(In reply to Mike Hommey [:glandium] from comment #32)
> Comment on attachment 8553339 [details] [diff] [review]
> Part 2b: Move PREF_JS_EXPORTS to moz.build (l10n MERGE_FILE hack)
> 
> Review of attachment 8553339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +452,5 @@
> > +                # MERGE_FILES to pick the right set of preferences. This should
> > +                # be able to go away once L10N builds are done differently (see
> > +                # bug 1107628).
> > +                backend_file.write('PREF_JS_EXPORTS += %s\n' % obj.path)
> > +            elif obj.path.startswith('/'):
> 
> if obj.path.startswith('/'):
> elif obj.path.startswith('/'):
> 
> One of those branches is never going to happen. I guess you meant $ for the
> first one.

Good catch; I must have botched a rebase somewhere along the line. However:

> That said, I'd rather not do this*: what's the point of moving stuff to
> moz.build if all they're doing is a hack to get the same value that was in
> Makefile.in in backend.mk...

That's fair. I was mostly just trying to stop using PREF_JS_EXPORTS entirely.

> * unless you *also* deprecate PREF_JS_EXPORTS such that defining it in
> Makefile.in throws an error. And even then, I /think/ I'd rather just change
> PREF_JS_EXPORTS to something else in those few locales files, like:
> 
> L10N_PREF_JS_EXPORTS_PATH = $(FINAL_TARGET)/$(PREF_DIR)
> L10N_PREF_JS_EXPORTS_FLAGS = $(PREF_PPFLAGS)
> --silence-missing-directive-warnings
> PP_TARGETS += L10N_PREF_JS_EXPORTS

That works for me. I didn't push to try, seeing as it was a simple patch and builds locally, but I can if you'd like.


If you'd prefer to just leave these alone, feel free to r- and I'll just land the first two parts (and file a followup for the rest).

Comment 34

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2b: Use PP_TARGETS instead of PREF_JS_EXPORTS for l10 prefs

> --- a/browser/metro/locales/import/Makefile.in
> +++ b/browser/metro/locales/import/Makefile.in
> @@ -6,18 +6,21 @@
>  relativesrcdir = browser/locales
>  
>  # copying firefox-l10n.js over from LOCALE_SRCDIR or browser
> -PREF_JS_EXPORTS = $(firstword $(wildcard $(LOCALE_SRCDIR)/firefox-l10n.js) \
> -                    $(topsrcdir)/$(relativesrcdir)/en-US/firefox-l10n.js )
> +L10N_PREF_JS_EXPORTS = $(firstword $(wildcard $(LOCALE_SRCDIR)/firefox-l10n.js) \
> +                         $(srcdir)/en-US/firefox-l10n.js )

Just realized I can't use $(srcdir) here because relativesrcdir gets reassigned above. May as well fix it now before someone tries to build metro.

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8555355 [details] [diff] [review]
Part 2b: Use PP_TARGETS instead of PREF_JS_EXPORTS for l10 prefs

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

You can add PREF_JS_EXPORTS to _MOZBUILD_EXTERNAL_VARIABLES or _DEPRECATED_VARIABLES in config/baseconfig.mk. Not sure which one is best.

Comment 36

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Move PREF_JS_EXPORTS to moz.build (mozbuild logic) r=gps

Comment 37

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Move PREF_JS_EXPORTS to moz.build. r=gps

Comment 38

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2b: Use PP_TARGETS instead of PREF_JS_EXPORTS for l10 prefs. r=glandium

(In reply to Mike Hommey [:glandium] from comment #35)
> Comment on attachment 8555355 [details] [diff] [review]
> Part 2b: Use PP_TARGETS instead of PREF_JS_EXPORTS for l10 prefs
> 
> Review of attachment 8555355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You can add PREF_JS_EXPORTS to _MOZBUILD_EXTERNAL_VARIABLES or
> _DEPRECATED_VARIABLES in config/baseconfig.mk. Not sure which one is best.

Part 3 takes care of that. I went with EXTERNAL, to get the "define it in moz.build" message.

Comment 39

[Brian O'Keefe [:bokeefe]]

Attachment: Part 3: Prohibit PREF_JS_EXPORTS in Makefile.ins. r=gps

Comment 40

[Brian O'Keefe [:bokeefe]]

I rebased all four patches onto tip (the last set was based on a rev from last week), so carrying r+'s forward.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7872e4c87991
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8289226c9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/65d56ae5d79c
https://hg.mozilla.org/integration/mozilla-inbound/rev/21284a1b338f

Comment 42

[Mike Hommey [:glandium]]

(In reply to Brian O'Keefe [:bokeefe] from comment #38)
> Created attachment 8557204 [details] [diff] [review]
> Part 2b: Use PP_TARGETS instead of PREF_JS_EXPORTS for l10 prefs. r=glandium
> 
> (In reply to Mike Hommey [:glandium] from comment #35)
> > Comment on attachment 8555355 [details] [diff] [review]
> > Part 2b: Use PP_TARGETS instead of PREF_JS_EXPORTS for l10 prefs
> > 
> > Review of attachment 8555355 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > You can add PREF_JS_EXPORTS to _MOZBUILD_EXTERNAL_VARIABLES or
> > _DEPRECATED_VARIABLES in config/baseconfig.mk. Not sure which one is best.
> 
> Part 3 takes care of that. I went with EXTERNAL, to get the "define it in
> moz.build" message.

Yeah, but that usually means you use the same variable name in the moz.build, which is not the case here.

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7872e4c87991
https://hg.mozilla.org/mozilla-central/rev/6d8289226c9a
https://hg.mozilla.org/mozilla-central/rev/65d56ae5d79c
https://hg.mozilla.org/mozilla-central/rev/21284a1b338f

Comment 44

[Bill Gianopoulos [:WG9s]]

This appears to have broken Android nightly builds.

Comment 45

[Phil Ringnalda (:philor)]

Backed out part 3 in https://hg.mozilla.org/mozilla-central/rev/9c465a62f8a9

Comment 46

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2c: Fix Makefile.in for android multilocale builds

MXR says this is the only one I missed

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8557618 [details] [diff] [review]
Part 2c: Fix Makefile.in for android multilocale builds

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

This works. Main thing to watch out for when converting rules to run as part of l10n is to ensure they are in the "libs" target. Fortunately, PP_TARGETS defaults to the libs target.

Comment 48

[Carsten Book [:Tomcat]]

can we get a try run for this ? thanks :)

Comment 49

[Brian O'Keefe [:bokeefe]]

(In reply to Carsten Book [:Tomcat] from comment #48)
> can we get a try run for this ? thanks :)

Sure thing. The try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d083598f17d2 was green with parts 2c and 3 applied.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f65ca700dfff
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d90b4aaa576

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f65ca700dfff
https://hg.mozilla.org/mozilla-central/rev/4d90b4aaa576