Closed
Bug 1235021
Opened 5 years ago
Closed 5 years ago
Refactor jar manifest handler code in FasterMake backend, and move it to CommonBackend
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox46 fixed)
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(13 files)
2.94 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
7.33 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
1.67 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
2.14 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
10.12 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
5.90 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
6.08 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
7.48 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
8.54 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
Part 10 - Re-emit ChromeManifestEntries from the jar manifest handler code in the FasterMake backend
4.52 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
8.29 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
4.35 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
12.63 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
There are mainly two reasons I want to do this: - Allow to reuse this code in e.g. bug 1214885 - Allow to not duplicate code between the jar manifest handler code and future changes to the handling of FinalTargetFiles in the FasterMake backend wrt GeneratedFiles.
Assignee | ||
Comment 1•5 years ago
|
||
Assignee: nobody → mh+mozilla
Attachment #8701763 -
Flags: review?(gps)
Assignee | ||
Comment 2•5 years ago
|
||
Attachment #8701764 -
Flags: review?(gps)
Assignee | ||
Comment 3•5 years ago
|
||
Attachment #8701765 -
Flags: review?(gps)
Assignee | ||
Comment 4•5 years ago
|
||
Attachment #8701766 -
Flags: review?(gps)
Assignee | ||
Comment 5•5 years ago
|
||
Attachment #8701767 -
Flags: review?(gps)
Assignee | ||
Comment 6•5 years ago
|
||
Attachment #8701769 -
Flags: review?(gps)
Assignee | ||
Comment 7•5 years ago
|
||
Attachment #8701770 -
Flags: review?(gps)
Assignee | ||
Comment 8•5 years ago
|
||
Attachment #8701771 -
Flags: review?(gps)
Assignee | ||
Comment 9•5 years ago
|
||
Attachment #8701772 -
Flags: review?(gps)
Assignee | ||
Comment 10•5 years ago
|
||
Attachment #8701773 -
Flags: review?(gps)
Assignee | ||
Comment 11•5 years ago
|
||
Attachment #8701774 -
Flags: review?(gps)
Assignee | ||
Comment 12•5 years ago
|
||
Attachment #8701775 -
Flags: review?(gps)
Assignee | ||
Comment 13•5 years ago
|
||
Attachment #8701776 -
Flags: review?(gps)
Assignee | ||
Comment 14•5 years ago
|
||
Comment on attachment 8701771 [details] [diff] [review] Part 8 - Associate a Defines instance to each ContextDerived instance Review of attachment 8701771 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/data.py @@ +88,5 @@ > > > +class HostMixin(object): > + @property > + def _defines(self): This is meant to be "def defines(self):"
Updated•5 years ago
|
Attachment #8701763 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701764 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701765 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701766 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701767 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701769 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701770 -
Flags: review?(gps) → review+
Comment 15•5 years ago
|
||
Comment on attachment 8701771 [details] [diff] [review] Part 8 - Associate a Defines instance to each ContextDerived instance Review of attachment 8701771 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/fastermake.py @@ +59,5 @@ > depfile, > **kwargs) > > def consume_object(self, obj): > + if isinstance(obj, (JARManifest, FinalTargetPreprocessedFiles)): It feels slightly odd that you're not checking the actual base classes that have .defines available. Perhaps hasattr() or getattr() could even be used instead? defines = getattr(obj, 'defines', {}) if defines: defines = defines.defines ::: python/mozbuild/mozbuild/frontend/data.py @@ +79,5 @@ > > @property > + def defines(self): > + defines = self._context.get('DEFINES') > + return Defines(self._context, defines) if defines else None At first glance, an empty Defines instance seems like a better return value than None, as the consumer treats None as an empty dict.
Attachment #8701771 -
Flags: review?(gps) → review+
Comment 16•5 years ago
|
||
Comment on attachment 8701772 [details] [diff] [review] Part 9 - Add a RenamedSourcePath helper class Review of attachment 8701772 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/context.py @@ +447,5 @@ > + """ > + def __init__(self, context, value): > + assert isinstance(value, tuple) > + assert len(value) == 2 > + source, self._target_basename = value You can drop the len() assertion since the destructed assignment should raise on length mismatch.
Attachment #8701772 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701773 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701774 -
Flags: review?(gps) → review+
Updated•5 years ago
|
Attachment #8701775 -
Flags: review?(gps) → review+
Comment 17•5 years ago
|
||
Comment on attachment 8701776 [details] [diff] [review] Part 13 - Move FasterMakeBackend._consume_jar_manifest to CommonBackend Review of attachment 8701776 [details] [diff] [review]: ----------------------------------------------------------------- I didn't look at the actual changes - assumed it was copied. Thanks for the imports cleanup.
Attachment #8701776 -
Flags: review?(gps) → review+
Assignee | ||
Comment 18•5 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #15) > Comment on attachment 8701771 [details] [diff] [review] > Part 8 - Associate a Defines instance to each ContextDerived instance > > Review of attachment 8701771 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: python/mozbuild/mozbuild/backend/fastermake.py > @@ +59,5 @@ > > depfile, > > **kwargs) > > > > def consume_object(self, obj): > > + if isinstance(obj, (JARManifest, FinalTargetPreprocessedFiles)): > > It feels slightly odd that you're not checking the actual base classes that > have .defines available. Perhaps hasattr() or getattr() could even be used > instead? They all have it, and I'm not interested in any other than those two. > ::: python/mozbuild/mozbuild/frontend/data.py > @@ +79,5 @@ > > > > @property > > + def defines(self): > > + defines = self._context.get('DEFINES') > > + return Defines(self._context, defines) if defines else None > > At first glance, an empty Defines instance seems like a better return value > than None, as the consumer treats None as an empty dict. Let's leave it like this for now, eventually I actually want to remove the extra (more or less useless) intermediate object.
Comment 19•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/12386f9fb717 https://hg.mozilla.org/integration/mozilla-inbound/rev/40ae4d686d46 https://hg.mozilla.org/integration/mozilla-inbound/rev/cce5cfe52a3d https://hg.mozilla.org/integration/mozilla-inbound/rev/7880d2168cf7 https://hg.mozilla.org/integration/mozilla-inbound/rev/666f5a3e18a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/73e69f3c9ea1 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d167756f2f https://hg.mozilla.org/integration/mozilla-inbound/rev/531617914786 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d191d8983d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/a99b3de8d858 https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3037f69b58 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d25c8199fec https://hg.mozilla.org/integration/mozilla-inbound/rev/70708efd7d3b
Comment 20•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/12386f9fb717 https://hg.mozilla.org/mozilla-central/rev/40ae4d686d46 https://hg.mozilla.org/mozilla-central/rev/cce5cfe52a3d https://hg.mozilla.org/mozilla-central/rev/7880d2168cf7 https://hg.mozilla.org/mozilla-central/rev/666f5a3e18a4 https://hg.mozilla.org/mozilla-central/rev/73e69f3c9ea1 https://hg.mozilla.org/mozilla-central/rev/a9d167756f2f https://hg.mozilla.org/mozilla-central/rev/531617914786 https://hg.mozilla.org/mozilla-central/rev/4d191d8983d6 https://hg.mozilla.org/mozilla-central/rev/a99b3de8d858 https://hg.mozilla.org/mozilla-central/rev/dc3037f69b58 https://hg.mozilla.org/mozilla-central/rev/3d25c8199fec https://hg.mozilla.org/mozilla-central/rev/70708efd7d3b
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•3 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•