Closed Bug 1235021 Opened 4 years ago Closed 4 years ago

Refactor jar manifest handler code in FasterMake backend, and move it to CommonBackend

Categories

(Firefox Build System :: General, defect)

defect
Not set

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
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: nobody → mh+mozilla
Attachment #8701763 - Flags: review?(gps)
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):"
Attachment #8701763 - Flags: review?(gps) → review+
Attachment #8701764 - Flags: review?(gps) → review+
Attachment #8701765 - Flags: review?(gps) → review+
Attachment #8701766 - Flags: review?(gps) → review+
Attachment #8701767 - Flags: review?(gps) → review+
Attachment #8701769 - Flags: review?(gps) → review+
Attachment #8701770 - Flags: review?(gps) → review+
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 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+
Attachment #8701773 - Flags: review?(gps) → review+
Attachment #8701774 - Flags: review?(gps) → review+
Attachment #8701775 - Flags: review?(gps) → review+
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+
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.