Move PREF_JS_EXPORTS to moz.build

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: joey, Assigned: bokeefe)

Tracking

(Blocks 1 bug)

unspecified
mozilla38
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(5 attachments, 15 obsolete attachments)

10.42 KB, patch
bokeefe
: review+
Details | Diff | Splinter Review
16.53 KB, patch
bokeefe
: review+
Details | Diff | Splinter Review
4.36 KB, patch
bokeefe
: review+
Details | Diff | Splinter Review
1.23 KB, patch
bokeefe
: review+
Details | Diff | Splinter Review
1.20 KB, patch
gps
: review+
Details | Diff | Splinter Review
No description provided.
No longer blocks: 870076
No longer depends on: 869135
Blocks: 870370
No longer blocks: 870370
Assignee: nobody → joey
Attachment #748957 - Attachment is obsolete: true
Attachment #748959 - Attachment is obsolete: true
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.
Attachment #748960 - Flags: review?(gps)
Attachment #748960 - Flags: feedback?(mshal)
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.
Attachment #748960 - Flags: review?(gps) → feedback+
(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.
Morphed from a variable passthrough conversion into named functions.
Variable renamed from PREF_JS_EXPORTS to JS_PREFERENCES per one of the nits.
Attachment #748960 - Attachment is obsolete: true
Attachment #748960 - Flags: feedback?(mshal)
/locales/Makefile.in conversion will be handled in another bug.
PREF_JS_EXPORTS = $(call MERGE_FILE,firefox-l10n.js)
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.
Attachment #749464 - Attachment is obsolete: true
Attachment #750074 - Flags: review?(gps)
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 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)]
Attachment #750074 - Flags: review?(gps) → feedback+
(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
Attachment #750074 - Attachment is obsolete: true
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.
Attachment #762219 - Flags: review?(gps)
See Also: → 882856
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.
Attachment #762219 - Flags: review?(gps) → feedback+
(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 ?
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')
Depends on: 882904
Depends on: 882906
Depends on: 882907
Depends on: 882908
No longer depends on: 882904
Why the depends?
(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
No longer depends on: 882906, 882907, 882908
(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).
(In reply to Gregory Szorc [:gps] from comment #18)
> Why the depends?

They were a byproduct of cloning existing conversion bugs.
Assignee: joey → nobody
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
Assignee: nobody → bokeefe
Attachment #749465 - Attachment is obsolete: true
Attachment #762219 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8553153 - Flags: review?(gps)
Attachment #8553155 - Flags: review?(gps)
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.
Attachment #8553160 - Flags: review?(gps)
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
Obviously, if Part 2b is r-'ed, this can't land.
Attachment #8553164 - Flags: review?(gps)
Blocks: 1124736
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.
With both Ms2ger's and my 'libs' issues addressed
Attachment #8553153 - Attachment is obsolete: true
Attachment #8553153 - Flags: review?(gps)
Attachment #8553338 - Flags: review?(gps)
Rebased because of the Part 1 update
Attachment #8553160 - Attachment is obsolete: true
Attachment #8553160 - Flags: review?(gps)
Attachment #8553339 - Flags: review?(gps)
Attachment #8553338 - Flags: review?(gps) → review+
Attachment #8553155 - Flags: review?(gps) → review+
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.
Attachment #8553339 - Flags: review?(gps) → review?(mh+mozilla)
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.
Attachment #8553164 - Flags: review?(gps) → review+
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
Attachment #8553339 - Flags: review?(mh+mozilla) → feedback-
(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).
Attachment #8553339 - Attachment is obsolete: true
Attachment #8555300 - Flags: review?(mh+mozilla)
> --- 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.
Attachment #8555300 - Attachment is obsolete: true
Attachment #8555300 - Flags: review?(mh+mozilla)
Attachment #8555355 - Flags: review?(mh+mozilla)
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.
Attachment #8555355 - Flags: review?(mh+mozilla) → review+
Attachment #8553155 - Attachment is obsolete: true
Attachment #8557203 - Flags: review+
(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.
Attachment #8555355 - Attachment is obsolete: true
Attachment #8557204 - Flags: review+
I rebased all four patches onto tip (the last set was based on a rev from last week), so carrying r+'s forward.
Keywords: checkin-needed
(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.
This appears to have broken Android nightly builds.
Backed out part 3 in https://hg.mozilla.org/mozilla-central/rev/9c465a62f8a9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
MXR says this is the only one I missed
Attachment #8557618 - Flags: review?(gps)
Blocks: 1128291
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.
Attachment #8557618 - Flags: review?(gps) → review+
Keywords: checkin-needed
can we get a try run for this ? thanks :)
Flags: needinfo?(bokeefe)
Keywords: checkin-needed
(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.
Flags: needinfo?(bokeefe)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f65ca700dfff
https://hg.mozilla.org/mozilla-central/rev/4d90b4aaa576
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.