Closed
Bug 870366
Opened 12 years ago
Closed 10 years ago
Move PREF_JS_EXPORTS to moz.build
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox38 fixed)
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: joey, Assigned: bokeefe)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 15 obsolete files)
(deleted),
patch
|
bokeefe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bokeefe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bokeefe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bokeefe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → joey
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #748957 -
Attachment is obsolete: true
Reporter | ||
Comment 3•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #748959 -
Attachment is obsolete: true
Reporter | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Reporter | ||
Comment 6•12 years ago
|
||
(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.
Reporter | ||
Comment 7•12 years ago
|
||
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)
Reporter | ||
Comment 8•12 years ago
|
||
/locales/Makefile.in conversion will be handled in another bug.
PREF_JS_EXPORTS = $(call MERGE_FILE,firefox-l10n.js)
Reporter | ||
Comment 9•12 years ago
|
||
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)
Reporter | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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+
Reporter | ||
Comment 12•12 years ago
|
||
(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
Reporter | ||
Comment 13•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Attachment #750074 -
Attachment is obsolete: true
Reporter | ||
Comment 14•11 years ago
|
||
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)
Reporter | ||
Updated•11 years ago
|
Comment 15•11 years ago
|
||
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+
Reporter | ||
Comment 16•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
Why the depends?
Reporter | ||
Comment 19•11 years ago
|
||
(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
Reporter | ||
Updated•11 years ago
|
Comment 20•11 years ago
|
||
(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•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #18)
> Why the depends?
They were a byproduct of cloning existing conversion bugs.
Reporter | ||
Updated•11 years ago
|
Assignee: joey → nobody
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8553155 -
Flags: review?(gps)
Assignee | ||
Comment 24•10 years ago
|
||
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 25•10 years ago
|
||
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
Assignee | ||
Comment 26•10 years ago
|
||
Obviously, if Part 2b is r-'ed, this can't land.
Attachment #8553164 -
Flags: review?(gps)
Assignee | ||
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
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)
Assignee | ||
Comment 29•10 years ago
|
||
Rebased because of the Part 1 update
Attachment #8553160 -
Attachment is obsolete: true
Attachment #8553160 -
Flags: review?(gps)
Attachment #8553339 -
Flags: review?(gps)
Updated•10 years ago
|
Attachment #8553338 -
Flags: review?(gps) → review+
Updated•10 years ago
|
Attachment #8553155 -
Flags: review?(gps) → review+
Comment 30•10 years ago
|
||
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 31•10 years ago
|
||
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 32•10 years ago
|
||
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-
Assignee | ||
Comment 33•10 years ago
|
||
(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)
Assignee | ||
Comment 34•10 years ago
|
||
> --- 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 35•10 years ago
|
||
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+
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8553338 -
Attachment is obsolete: true
Attachment #8557201 -
Flags: review+
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8553155 -
Attachment is obsolete: true
Attachment #8557203 -
Flags: review+
Assignee | ||
Comment 38•10 years ago
|
||
(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+
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8553164 -
Attachment is obsolete: true
Attachment #8557205 -
Flags: review+
Assignee | ||
Comment 40•10 years ago
|
||
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
Comment 41•10 years ago
|
||
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
Keywords: checkin-needed
Comment 42•10 years ago
|
||
(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•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 44•10 years ago
|
||
This appears to have broken Android nightly builds.
Comment 45•10 years ago
|
||
Backed out part 3 in https://hg.mozilla.org/mozilla-central/rev/9c465a62f8a9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 46•10 years ago
|
||
MXR says this is the only one I missed
Attachment #8557618 -
Flags: review?(gps)
Comment 47•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 48•10 years ago
|
||
can we get a try run for this ? thanks :)
Flags: needinfo?(bokeefe)
Keywords: checkin-needed
Assignee | ||
Comment 49•10 years ago
|
||
(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
Comment 50•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65ca700dfff
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d90b4aaa576
Keywords: checkin-needed
Comment 51•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f65ca700dfff
https://hg.mozilla.org/mozilla-central/rev/4d90b4aaa576
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•