Closed Bug 925185 Opened 6 years ago Closed 6 years ago

Standardize Java JAR building into moz.build

Categories

(Firefox Build System :: General, defect)

All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

This is a follow-on to Bug 923306.  Having defined a rules.mk grammar for Java JAR files

(target_DEST, target_SOURCES, target_PP_SOURCES, target_JAVAC_FLAGS)

we can convert it fairly easily to moz.build.  This gets us to having Android resources and sources defined in moz.build files, which is getting us closer to generating IDE projects.
I chose a single add_java_jar function in the sandbox, but since I
just saw glandium say he doesn't like functions in moz.build files,
we could go:

JAVA_JARS['name.jar'].SOURCES = ...
JAVA_JARS['name.jar'].PP_SOURCES = ...

This is based on some other work, but really only depends on Bug
923306.
Attachment #815170 - Flags: review?(gps)
(In reply to Nick Alexander :nalexander from comment #1)
> JAVA_JARS['name.jar'].SOURCES = ...
> JAVA_JARS['name.jar'].PP_SOURCES = ...

JAVA_JARS['name'] ?
Attachment #815170 - Flags: review?(gps)
This needs another pass to incorporate design discussion from #build.  Basically, add_java_jar should mirror it's settings in some sort of JAVA_JARS dictionary, as above, and expose JAVA_JARS[name].* from there.  Since gps's review queue is groaning, let's take this off the stack until I can update it.
Comment on attachment 815170 [details] [diff] [review]
Move JAVA_JAR_TARGETS to moz.build. r=gps

Review of attachment 815170 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to look at this soonish
Attachment #815170 - Flags: feedback?(Ms2ger)
Assignee: nobody → nalexander
I went for option C from the mailing list.  I was swayed by jcranmer's
"devolution from A to B" argument.  So:

jar = add_java_jar("only required arguments")
jar.sources = [ "list of declarative settings" ]
Attachment #815170 - Attachment is obsolete: true
Attachment #815170 - Flags: feedback?(Ms2ger)
Attachment #817202 - Flags: review?(gps)
Attachment #817197 - Flags: review?(gps)
Comment on attachment 817202 [details] [diff] [review]
Bug 925185 - Part 1: Add add_java_jar to moz.build. r=gps

Review of attachment 817202 [details] [diff] [review]:
-----------------------------------------------------------------

I like where this is going. But I would really like SandboxWrapped to be done properly. This may involve an IRC conversation.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +411,4 @@
>          elif isinstance(obj, LocalInclude):
>              self._process_local_include(obj.path, backend_file)
>  
> +        elif isinstance(obj, SandboxWrapped) and isinstance(obj.wrapped, JavaJarData):

Ewww.

::: python/mozbuild/mozbuild/frontend/data.py
@@ +364,5 @@
> +
> +    def __init__(self, sandbox, wrapped):
> +        SandboxDerived.__init__(self, sandbox)
> +
> +        self.wrapped = wrapped

OK. I see why this is needed. But, I think going through .wrapped is annoying. Can we just overload __getattr__ and friends to proxy to the wrapped type? http://docs.python.org/2/reference/datamodel.html#customizing-attribute-access

You could also do some more funky things, such as dynamic class swapping. But I think the proxy pattern is simplest.

@@ +367,5 @@
> +
> +        self.wrapped = wrapped
> +
> +
> +class JavaJarData():

Always inherit from object.

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +224,5 @@
>  
> +    def _add_java_jar(self, name):
> +        """Add a Java JAR build target."""
> +        if not name:
> +            raise Exception('Java JAR cannot be registered with name: %s' % name)

"without name"?

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +211,5 @@
> +        """Defines Java JAR targets to be built.
> +
> +        This variable should not be populated directly. Instead, it should
> +        populated by calling add_java_jar().
> +        """, 'binaries'),

Something to keep in mind as we use this pattern is that we may want to just not expose these special variables directly to the sandbox. If you manage to assign a key to the globals namespace, lookups will just work, even if the variable isn't declared. So in your populator function (_add_java_jar), you should be able to do something like:

  with sandbox._globals.allow_all_writes():
      sandbox._globals['JAVA_JAR_TARGETS'][k] = v

This does have the side-effect that those variables are exposed to the sandbox after they are written. Perhaps we need the sandbox to contain a third dict of symbols that are merged with the globals after execution?

This comment is totally scope bloat and can be a followup bug.

@@ +541,5 @@
> +        * generated_sources - list of generated input java sources
> +        * extra_jars - list of JAR file dependencies to include on the
> +          javac compiler classpath
> +        * javac_flags - string containing extra flags passed to the javac
> +          compiler

With the work on Sphinx, we can now have richer documentation here. Instead of documenting the attributes here, consider documenting them on the actual class instance. Here, you can add a reference to the class docs. e.g.

   See :py:class:`mozbuild.frontend.data.JavaJarData`.
Attachment #817202 - Flags: review?(gps) → feedback+
Comment on attachment 817197 [details] [diff] [review]
Part 2: Use add_java_jar and restrict JAVA_JAR_TARGETS to moz.build.

Review of attachment 817197 [details] [diff] [review]:
-----------------------------------------------------------------

*Shakes fist at excessively long lists*

I'm not sure how I feel about the nearly empty constructor followed by .attribute assignment. I think I'd rather the static bits get passed directly into the ctor. I also don't care too much for the "jar" variable reuse. Could be confusing, especially when reading diffs in context. (Although with Phabricator you can expand context :D)

But, these are style nits. I'm fine with experimenting with this early. I have no doubt a superior style will surface eventually. Let's not try to predict history unless you feel similarly.
Attachment #817197 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 817202 [details] [diff] [review]
> Bug 925185 - Part 1: Add add_java_jar to moz.build. r=gps
> 
> Review of attachment 817202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like where this is going. But I would really like SandboxWrapped to be
> done properly. This may involve an IRC conversation.
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +411,4 @@
> >          elif isinstance(obj, LocalInclude):
> >              self._process_local_include(obj.path, backend_file)
> >  
> > +        elif isinstance(obj, SandboxWrapped) and isinstance(obj.wrapped, JavaJarData):
> 
> Ewww.

This looks less bad when there are lots of wrapped types.  And I think there will be lots: we're going to have a bunch of things that can take a few parameters in moz.build files and then get fed as-is to the backend.  They can all use the trivial wrapper.
 
> ::: python/mozbuild/mozbuild/frontend/data.py
> @@ +364,5 @@
> > +
> > +    def __init__(self, sandbox, wrapped):
> > +        SandboxDerived.__init__(self, sandbox)
> > +
> > +        self.wrapped = wrapped
> 
> OK. I see why this is needed. But, I think going through .wrapped is
> annoying. Can we just overload __getattr__ and friends to proxy to the
> wrapped type?
> http://docs.python.org/2/reference/datamodel.html#customizing-attribute-
> access
> 
> You could also do some more funky things, such as dynamic class swapping.
> But I think the proxy pattern is simplest.

The backends will need to inspect on the wrapped type, though -- so how much does this fanciness get us?

> ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
> @@ +211,5 @@
> > +        """Defines Java JAR targets to be built.
> > +
> > +        This variable should not be populated directly. Instead, it should
> > +        populated by calling add_java_jar().
> > +        """, 'binaries'),
> 
> Something to keep in mind as we use this pattern is that we may want to just
> not expose these special variables directly to the sandbox. If you manage to
> assign a key to the globals namespace, lookups will just work, even if the
> variable isn't declared. So in your populator function (_add_java_jar), you
> should be able to do something like:
> 
>   with sandbox._globals.allow_all_writes():
>       sandbox._globals['JAVA_JAR_TARGETS'][k] = v
> 
> This does have the side-effect that those variables are exposed to the
> sandbox after they are written. Perhaps we need the sandbox to contain a
> third dict of symbols that are merged with the globals after execution?
> 
> This comment is totally scope bloat and can be a followup bug.

This is against glandium's stated perspective that moz.build's are "just lists" or very close to it.  My perspective is that nobody should be consuming moz.build output; instead, they should be consuming the output of the TreeMetadataEmitter.  (Example: ANDROID_RESFILES and ANDROID_GENERATED_RESFILES are separate moz.build variables but should be combined by the emitter into a coherent unit.)
(In reply to Gregory Szorc [:gps] from comment #10)
> Comment on attachment 817197 [details] [diff] [review]
> Part 2: Use add_java_jar and restrict JAVA_JAR_TARGETS to moz.build.
> 
> Review of attachment 817197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> *Shakes fist at excessively long lists*

I gave some thought to allowing directories, but it's not easy to handle dynamic file lists with make.  At least, I don't see how to do it with javac.

> I'm not sure how I feel about the nearly empty constructor followed by
> .attribute assignment. I think I'd rather the static bits get passed
> directly into the ctor. I also don't care too much for the "jar" variable
> reuse. Could be confusing, especially when reading diffs in context.
> (Although with Phabricator you can expand context :D)

I actually have the trivial rename patch and forgot to attach it.  I actually prefer _ as a temporary name, 'cuz then there's no doubt that the name is not meaningful.

> But, these are style nits. I'm fine with experimenting with this early. I
> have no doubt a superior style will surface eventually. Let's not try to
> predict history unless you feel similarly.

I don't have a strong opinion.
Comment on attachment 821996 [details] [diff] [review]
Review comments. TO BE FOLDED INTO PARENT

Just the changes from review.  I'll fold this down before committing.
Attachment #821996 - Flags: review?(gps)
Attachment #821996 - Flags: review?(gps) → review+
Unfortunately I've had to back this out, since it caused merge conflicts with bug 928709 (which landed on inbound). If possible, please can you land build changes on inbound (since fx-team is primarily for browser/* and mobile/* work). Thank you :-)

remote:   https://hg.mozilla.org/integration/fx-team/rev/7f4e3bf3de5c
remote:   https://hg.mozilla.org/integration/fx-team/rev/55ffecb3e3b8
remote:   https://hg.mozilla.org/integration/fx-team/rev/cb59117d41f5
https://hg.mozilla.org/mozilla-central/rev/41d2964e2231
https://hg.mozilla.org/mozilla-central/rev/a41ce04d632d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.