Closed Bug 794982 Opened 12 years ago Closed 11 years ago

Automagically produce APKOpen.cpp macro goop for native java functions

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(fennec+)

Status:
RESOLVED FIXED
Milestone:
mozilla21
Tracking Flags:
Tracking Status
fennec + ---

People

(Reporter: blassey, Assigned: kats)

References

Details

Attachments

(2 files, 5 obsolete files)

Part 1 - Move non-libxul native JNI functions out of GeckoAppShell
48.92 KB, patch
cpeterson
: review+
glandium
: review+
Details | Diff | Splinter Review
Part 2 - Autogenerate libxul JNI wrapper glue stuff (v4)
43.19 KB, patch
glandium
: review+
Details | Diff | Splinter Review 
      No description provided.
Summary: Automagically produce OpenAPK macro goop for native java functions → Automagically produce APKOpen.cpp macro goop for native java functions
tracking-fennec: ? → +
there is no blocking relationship here
No longer depends on: 794981
It looks like fully automating this into the build might be hard, because the APKOpen.cpp stuff belongs in the base tier and gets built before the java code. As a first step though I could add a script which can generate the APKOpen goop from the java code, and you'd just have to run that manually whenever you want to regenerate the APKOpen goop.

A second step might be to have the generator run as part of the java build, compare the generated APKOpen to the existing one, and fail the build if it is different. That way you would be forced to update the APKOpen and re-run the build.
Just to be clear, this is about the Java_org_mozilla_gecko_GeckoAppShell_* functions, right?
Yeah. There's a bunch of SHELL_WRAPPER macros in mozglue/android/APKOpen.cpp that define the GeckoAppShell native function wrappers, and we would like to automatically generate those rather than having to manually maintain it.
(In reply to Kartikaya Gupta (:kats) from comment #2)
> It looks like fully automating this into the build might be hard, because
> the APKOpen.cpp stuff belongs in the base tier and gets built before the
> java code. As a first step though I could add a script which can generate
> the APKOpen goop from the java code, and you'd just have to run that
> manually whenever you want to regenerate the APKOpen goop.

Alternatively, you could have a header produced by mobile/android/base in the export phase that APKOpen.cpp uses. That should get all the dependencies right.
(In reply to Brad Lassey [:blassey] from comment #5)
> Alternatively, you could have a header produced by mobile/android/base in
> the export phase that APKOpen.cpp uses. That should get all the dependencies
> right.

That could work, if I move some/all of the java compilation into the export tier as well. The javah tool that generates the JNI signatures only operates on .class files, so it requires the javac compilation to already have taken place.
mobile/android/base and mozglue are not built in the same tier, so mobile/android/base's export is not run before mozglue's libs.
I think running the tool manually would work fine. We have a similar thing for OpenGL shaders and it's not too bad.
Some of the native functions in GeckoAppShell are implemented directly in APKOpen.cpp and nsGeckoUtils.cpp and don't have wrapper goop, so moving them out of GeckoAppShell makes my life easier (and is better encapsulation, methinks).
Comment on attachment 710232 [details] [diff] [review]
Part 2 - Autogenerate libxul JNI wrapper glue stuff

We were discussing this on IRC today but I wasn't quite sure how we left things. I finished gluing my awk script into the build system and it seems to work fine and gets rid of all of the goop in APKOpen.cpp. I think this is cleaner than the preprocessor magic in bug 837821 but I'm open to discussing different ways of doing this or combining this with other approaches.
Attachment #710232 - Flags: feedback?(mh+mozilla)
Comment on attachment 710231 [details] [diff] [review]
Part 1 - Move non-libxul native JNI functions out of GeckoAppShell

This part I would like to land regardless since I think it's a good cleanup even if we don't use the other two patches. Requesting review (r? to glandium for the Makefile changes).
Attachment #710231 - Flags: review?(mh+mozilla)
Attachment #710231 - Flags: review?(cpeterson)
Comment on attachment 710232 [details] [diff] [review]
Part 2 - Autogenerate libxul JNI wrapper glue stuff

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

I'd really prefer if the script would just create the SHELL_WRAPPER invokations, because with code constructed in the awk script, it's really painful to change that code. Alternatively, use something other than awk to make things much clearer to edit.
Attachment #710232 - Flags: feedback?(mh+mozilla) → feedback-
Comment on attachment 710231 [details] [diff] [review]
Part 1 - Move non-libxul native JNI functions out of GeckoAppShell

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

r=me for the build system parts.
Attachment #710231 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #15)
> I'd really prefer if the script would just create the SHELL_WRAPPER
> invokations, because with code constructed in the awk script, it's really
> painful to change that code. Alternatively, use something other than awk to
> make things much clearer to edit.

I can format the awk script to be more readable/editable if that's the main concern. The other advantage of using this script over the macros, I just realized, is that if we want to have native functions in classes other than GeckoAppShell (and I do, for the ongoing work of PanZoomController unification), it's much easier to handle with the awk script than the macros.
You should also probably do it in python instead of awk.
Comment on attachment 710231 [details] [diff] [review]
Part 1 - Move non-libxul native JNI functions out of GeckoAppShell

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

LGTM!

::: mobile/android/base/Makefile.in
@@ +1178,3 @@
>  	@echo "JAR gecko-mozglue.jar"
>  	$(NSINSTALL) -D classes/gecko-mozglue
> +	$(JAVAC) $(JAVAC_FLAGS) -Xlint:all -d classes/gecko-mozglue $(addprefix $(srcdir)/,$(MOZGLUE_JAVA_FILES)) $(MOZGLUE_PP_JAVA_FILES)

Why does MOZGLUE_JAVA_FILES need `$(addprefix $(srcdir)` but not MOZGLUE_PP_JAVA_FILES/

::: mobile/android/base/mozglue/GeckoLoader.java.in
@@ +17,5 @@
> +import java.text.DecimalFormatSymbols;
> +import java.text.NumberFormat;
> +import java.util.Locale;
> +
> +public final class GeckoLoader {

Since GeckoLoader is a static class, please add a private GeckoLoader() constructor so no one can allocate a GeckoLoader instance.

@@ +104,5 @@
> +        File f = context.getDir("tmp", Context.MODE_WORLD_READABLE |
> +                                 Context.MODE_WORLD_WRITEABLE );
> +        if (!f.exists()) {
> +            f.mkdirs();
> +        }

If you extract this getDir("tmp") code into a getTmpDir() helper method, then the @SuppressWarnings("deprecation") annotation only has to apply to that smaller method. Not a big deal, though. :)
Attachment #710231 - Flags: review?(cpeterson) → review+
Now in python, and a single script to spit out both include files.
Attachment #710232 - Attachment is obsolete: true
Attachment #710236 - Attachment is obsolete: true
Attachment #710349 - Flags: review?(mh+mozilla)
(In reply to Chris Peterson (:cpeterson) from comment #19)
> Why does MOZGLUE_JAVA_FILES need `$(addprefix $(srcdir)` but not
> MOZGLUE_PP_JAVA_FILES/
> 

The non-preprocessed files are read straight from the srcdir, so they need the addprefix. The preprocessed ones are processed into the objdir, and those processed versions are passed to javac, so they don't need the addprefix. The other jar targets all do the same thing as well.

> 
> Since GeckoLoader is a static class, please add a private GeckoLoader()
> constructor so no one can allocate a GeckoLoader instance.
> 

Good point, will do.

> @@ +104,5 @@
> > +        File f = context.getDir("tmp", Context.MODE_WORLD_READABLE |
> > +                                 Context.MODE_WORLD_WRITEABLE );
> > +        if (!f.exists()) {
> > +            f.mkdirs();
> > +        }
> 
> If you extract this getDir("tmp") code into a getTmpDir() helper method,
> then the @SuppressWarnings("deprecation") annotation only has to apply to
> that smaller method. Not a big deal, though. :)

Good idea, I'll do that as well.
Comment on attachment 710349 [details] [diff] [review]
Part 2 - Autogenerate libxul JNI wrapper glue stuff (v2)

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

::: mobile/android/base/GeckoAppShell.java
@@ +174,5 @@
>      public static native Message getNextMessageFromQueue(MessageQueue queue);
>      public static native void onSurfaceTextureFrameAvailable(Object surfaceTexture, int id);
>  
> +    // this is only used in XUL fennec but we need to have this signature so that our build
> +    // scripts generate the mozglue JNI binding for it

Note that last I tried (a few weeks ago) XUL fennec didn't even build. It's probably not worth caring.

::: mobile/android/base/Makefile.in
@@ +1197,5 @@
>  	$(NSINSTALL) -D jars
>  
> +jni-stubs.inc jni-bindings.inc: jars/gecko-browser.jar jars/gecko-mozglue.jar jars/gecko-util.jar jars/sync-thirdparty.jar
> +	$(JAVAH) -o javah.out -bootclasspath $(JAVA_BOOTCLASSPATH) -classpath jars/gecko-browser.jar:jars/gecko-mozglue.jar:jars/gecko-util.jar:jars/sync-thirdparty.jar org.mozilla.gecko.GeckoAppShell
> +	$(PYTHON) $(topsrcdir)/mobile/android/base/jni-generator.py --stubs jni-stubs.inc --bindings jni-bindings.inc javah.out

This is a parallel build hazard. The least painful solution on the Makefile side is to just generate one file with ifdefs, and #define the relevant macro when including the file. Otherwise, you need to jump through hoops to do the right thing in the Makefile.

::: mobile/android/base/jni-generator.py
@@ +6,5 @@
> +import re
> +
> +class Generator:
> +    """
> +    Class to convert a javah-produced JNI stub file into stubs/bindings files for inclusion

Please run your script through flake8.

@@ +14,5 @@
> +        self.stubfile = stubfile
> +        self.bindingfile = bindingfile
> +
> +    def writeStub(self, stuff):
> +        if (self.stubfile != None):

if self.stubfile:

@@ +18,5 @@
> +        if (self.stubfile != None):
> +            self.stubfile.write(stuff)
> +
> +    def writeBinding(self, stuff):
> +        if (self.bindingfile != None):

if self.bindingfile:

@@ +32,5 @@
> +            # JNIEXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_nativeInit
> +            if re.search('JNIEXPORT', line) != None:
> +                tokens = re.split('\s', line)
> +                returnType = tokens[1]
> +                functionName = tokens[3]

match = re.match('JNIEXPORT\s+(?P<returnType>\S+)\s+JNICALL\s+(?P<functionName>\S+)', line)
if match:
    returnType = match.group('returnType')
    functionName = match.group('functionName')

@@ +45,5 @@
> +                    if i > 0:
> +                        parameters += ', '
> +                        arguments += ', '
> +                    parameters += ('%s arg%d' % (tokens[i], i))
> +                    arguments += ('arg%d' % i)

match = re.match('\((JNIEnv \*, jclass(?:,\s+(?:[^,]+))*)\)', line)
params = re.split(',\s+', match.group(1))

(here params would contain ['JNIEnv *', 'jclass', ...])

@@ +55,5 @@
> +                self.writeStub('typedef %s (*%s_t) %s\n' % (returnType, functionName, line))
> +                self.writeStub('static %s_t f_%s;\n' % (functionName, functionName))
> +                self.writeStub('extern "C" NS_EXPORT %s JNICALL %s %s) {\n' % (returnType, functionName, parameters))
> +                self.writeStub('   %s f_%s(%s);\n' % (returnKeyword, functionName, arguments))
> +                self.writeStub('}\n')

A single string enclosed with ''' would make things more understandable, as well as using parametrized formatting.
Something like:
  self.writeStub('''
typedef %(returnType)s (*%(functionName)s_t)(%(params)s);
static %(functionName)s_t;
(...)
''' % {"returnType": returnType, "functionName": functionName, "params": params })
Attachment #710349 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #22)
> > +    // this is only used in XUL fennec but we need to have this signature so that our build
> > +    // scripts generate the mozglue JNI binding for it
> 
> Note that last I tried (a few weeks ago) XUL fennec didn't even build. It's
> probably not worth caring.

Removed.

> This is a parallel build hazard. The least painful solution on the Makefile
> side is to just generate one file with ifdefs, and #define the relevant
> macro when including the file. Otherwise, you need to jump through hoops to
> do the right thing in the Makefile.

Ok, done. I also made the infile/outfile arguments to the script mandatory as a result.

> Please run your script through flake8.

Done, fixed a few things. The remaining things are line length warnings and the placement of the % {...} things. I thought the way I did it was more readable than what flake8 would have me do so I left those but I can change it if you want.

> match =
> re.match('JNIEXPORT\s+(?P<returnType>\S+)\s+JNICALL\s+(?P<functionName>\S+)',
> line)
> if match:
>     returnType = match.group('returnType')
>     functionName = match.group('functionName')

Done

> match = re.match('\((JNIEnv \*, jclass(?:,\s+(?:[^,]+))*)\)', line)
> params = re.split(',\s+', match.group(1))
> 
> (here params would contain ['JNIEnv *', 'jclass', ...])

I had to do a little differently because of the space between JNIEnv and * but it's close.

> A single string enclosed with ''' would make things more understandable, as
> well as using parametrized formatting.
> Something like:
>   self.writeStub('''
> typedef %(returnType)s (*%(functionName)s_t)(%(params)s);
> static %(functionName)s_t;
> (...)
> ''' % {"returnType": returnType, "functionName": functionName, "params":
> params })

Done. Thanks for that, it makes it much more readable. /me is a python n00b.
Attachment #710349 - Attachment is obsolete: true
Attachment #710755 - Flags: review?(mh+mozilla)
Oops, attached the wrong patch.
Attachment #710755 - Attachment is obsolete: true
Attachment #710755 - Flags: review?(mh+mozilla)
Attachment #710757 - Flags: review?(mh+mozilla)
Comment on attachment 710757 [details] [diff] [review]
Part 2 - Autogenerate libxul JNI wrapper glue stuff (v3)

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

Just python nits.

::: mobile/android/base/jni-generator.py
@@ +16,5 @@
> +
> +    def writeStub(self, stuff):
> +        self.outputfile.write('#ifdef JNI_STUBS\n')
> +        self.outputfile.write(stuff)
> +        self.outputfile.write('#endif\n\n')

Maybe make this a
  def write(self, type, stuff):
    self.outputfile.write('#ifdef %s\n' % type)
    (...)

and do self.write('JNI_STUBS', ''' (...) '''), self.write('JNI_BINDINGS', ''' (...) ''')

@@ +24,5 @@
> +        self.outputfile.write(stuff)
> +        self.outputfile.write('#endif\n\n')
> +
> +    def process(self, inputfile):
> +        self.outputfile.write('/* WARNING - This file is autogenerated by mobile/android/base/jni-generator.py. Do not edit manually! */\n')

Please try to fit in 80 columns.

@@ +30,5 @@
> +            line = line.strip()
> +
> +            # this matches lines such as:
> +            # JNIEXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_nativeInit
> +            match = re.match('JNIEXPORT\s+(?P<returnType>\S+)\s+JNICALL\s+(?P<functionName>\S+)', line)

FWIW, you can make regexps more readable with re.VERBOSE, if you want to.
http://docs.python.org/2/library/re.html?highlight=re.match#re.VERBOSE

@@ +39,5 @@
> +            # this matches lines such as:
> +            #   (JNIEnv *, jclass);
> +            match = re.match('\((JNIEnv \*(?:.*))*\);', line)
> +            if match:
> +                paramTypes = map(str.strip, re.split(',', match.group(1)))

paramTypes = re.split('\s*,\s*', match.group(1))
should do it.

@@ +40,5 @@
> +            #   (JNIEnv *, jclass);
> +            match = re.match('\((JNIEnv \*(?:.*))*\);', line)
> +            if match:
> +                paramTypes = map(str.strip, re.split(',', match.group(1)))
> +                paramNames = map(lambda i: 'arg%d' % i, range(0, len(paramTypes)))

paramName = ['arg%d' % i for i in range(0, len(paramTypes))]

@@ +45,5 @@
> +
> +                self.writeStub('''
> +typedef %(returnType)s (*%(functionName)s_t)(%(paramTypes)s);
> +static %(functionName)s_t f_%(functionName)s;
> +extern "C" NS_EXPORT %(returnType)s JNICALL %(functionName)s(%(parameterList)s) {

Maybe put the functionName on a new line.

@@ +51,5 @@
> +}
> +'''                 % {'returnType': returnType,
> +                       'functionName': functionName,
> +                       'paramTypes': ', '.join(paramTypes),
> +                       'parameterList': ', '.join(map(lambda *x: ' '.join(x), paramTypes, paramNames)),

', '.join('%s %s' % param for param in zip(a, b))

@@ +54,5 @@
> +                       'paramTypes': ', '.join(paramTypes),
> +                       'parameterList': ', '.join(map(lambda *x: ' '.join(x), paramTypes, paramNames)),
> +                       'arguments': ', '.join(paramNames),
> +                       'returnKeyword': 'return' if returnType != 'void' else ''})
> +                self.writeBinding('  xul_dlsym("%(functionName)s", &f_%(functionName)s);\n'

Please define the stub and bindings template strings as global constants. Will make it easier to spot them when we need to modify them.
Attachment #710757 - Flags: review?(mh+mozilla) → feedback+
Fixed up review comments. That re.VERBOSE thing is really cool :)
Attachment #710757 - Attachment is obsolete: true
Attachment #710824 - Flags: review?(mh+mozilla)
Comment on attachment 710824 [details] [diff] [review]
Part 2 - Autogenerate libxul JNI wrapper glue stuff (v4)

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

::: mobile/android/base/Makefile.in
@@ +1196,5 @@
>  	@echo "MKDIR jars"
>  	$(NSINSTALL) -D jars
>  
> +jni-stubs.inc: jars/gecko-browser.jar jars/gecko-mozglue.jar jars/gecko-util.jar jars/sync-thirdparty.jar
> +	$(JAVAH) -o javah.out -bootclasspath $(JAVA_BOOTCLASSPATH) -classpath jars/gecko-browser.jar:jars/gecko-mozglue.jar:jars/gecko-util.jar:jars/sync-thirdparty.jar org.mozilla.gecko.GeckoAppShell

If you feel like it, you can replace -classpath jars/... with -classpath $(subst $(NULL) $(NULL),:,$^)
Attachment #710824 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/5044d9a352ef
https://hg.mozilla.org/mozilla-central/rev/8373b1ce47f0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: