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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Part 1 - Add a relative jar.mn in the end-to-end test

Comment 2

[Mike Hommey [:glandium]]

Attachment: Part 2 - Use SourcePaths for JAR_MANIFESTS

Comment 3

[Mike Hommey [:glandium]]

Attachment: Part 3 - Normalize paths from jar manifests before putting them in an install manifest in the FasterMake backend

Comment 4

[Mike Hommey [:glandium]]

Attachment: Part 4 - Normalize whitespaces in manifest entries emitted by the jar manifest parser

Comment 5

[Mike Hommey [:glandium]]

Attachment: Part 5 - Always use the % marker for preprocessed css files

Comment 6

[Mike Hommey [:glandium]]

Attachment: Part 6 - Use Path instances to handle paths in jar manifest in the FasterMake backend

Comment 7

[Mike Hommey [:glandium]]

Attachment: Part 7 - Rename Linkable.defines to linkable.lib_defines

Comment 8

[Mike Hommey [:glandium]]

Attachment: Part 8 - Associate a Defines instance to each ContextDerived instance

Comment 9

[Mike Hommey [:glandium]]

Attachment: Part 9 - Add a RenamedSourcePath helper class

Comment 10

[Mike Hommey [:glandium]]

Attachment: Part 10 - Re-emit ChromeManifestEntries from the jar manifest handler code in the FasterMake backend

Comment 11

[Mike Hommey [:glandium]]

Attachment: Part 11 - Re-emit FinalTarget{,Preprocessed}Files from the jar manifest handler code in the FasterMake backend

Comment 12

[Mike Hommey [:glandium]]

Attachment: Part 12 - Avoid passing defines to FasterMakeBackend._consume_jar_manifest

Comment 13

[Mike Hommey [:glandium]]

Attachment: Part 13 - Move FasterMakeBackend._consume_jar_manifest to CommonBackend

Comment 14

[Mike Hommey [:glandium]]

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):"

Comment 15

[Gregory Szorc [: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/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.

Comment 16

[Gregory Szorc [:gps]]

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.

Comment 17

[Gregory Szorc [:gps]]

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.

Comment 18

[Mike Hommey [:glandium]]

(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

[Pulsebot]

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

[[Back July 11] Ryan VanderMeulen]

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