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)
Tracking
(fennec+)
RESOLVED
FIXED
mozilla21
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: blassey, Assigned: kats)
References
Details
Attachments
(2 files, 5 obsolete files)
48.92 KB,
patch
|
cpeterson
:
review+
glandium
:
review+
|
Details | Diff | Splinter Review |
43.19 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•12 years ago
|
Summary: Automagically produce OpenAPK macro goop for native java functions → Automagically produce APKOpen.cpp macro goop for native java functions
Reporter | ||
Updated•12 years ago
|
tracking-fennec: ? → +
Reporter | ||
Comment 1•12 years ago
|
||
there is no blocking relationship here
No longer depends on: 794981
Assignee | ||
Comment 2•12 years ago
|
||
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.
Comment 3•12 years ago
|
||
Just to be clear, this is about the Java_org_mozilla_gecko_GeckoAppShell_* functions, right?
Assignee | ||
Comment 4•12 years ago
|
||
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.
Reporter | ||
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Comment 7•12 years ago
|
||
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.
Comment 8•12 years ago
|
||
I think running the tool manually would work fine. We have a similar thing for OpenGL shaders and it's not too bad.
Assignee | ||
Comment 9•11 years ago
|
||
I have the start of a WIP at https://github.com/staktrace/mozilla-central/commit/9f6c8571fa28f088bb754505e41efe5b2d805587
Assignee | ||
Comment 10•11 years ago
|
||
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).
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
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 16•11 years ago
|
||
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+
Assignee | ||
Comment 17•11 years ago
|
||
(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.
Comment 18•11 years ago
|
||
You should also probably do it in python instead of awk.
Comment 19•11 years ago
|
||
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+
Assignee | ||
Comment 20•11 years ago
|
||
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)
Assignee | ||
Comment 21•11 years ago
|
||
(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 22•11 years ago
|
||
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-
Assignee | ||
Comment 23•11 years ago
|
||
(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)
Assignee | ||
Comment 24•11 years ago
|
||
Oops, attached the wrong patch.
Attachment #710755 -
Attachment is obsolete: true
Attachment #710755 -
Flags: review?(mh+mozilla)
Attachment #710757 -
Flags: review?(mh+mozilla)
Comment 25•11 years ago
|
||
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+
Assignee | ||
Comment 26•11 years ago
|
||
Fixed up review comments. That re.VERBOSE thing is really cool :)
Attachment #710757 -
Attachment is obsolete: true
Attachment #710824 -
Flags: review?(mh+mozilla)
Comment 27•11 years ago
|
||
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+
Assignee | ||
Comment 28•11 years ago
|
||
Landed with that change as well: https://hg.mozilla.org/integration/mozilla-inbound/rev/5044d9a352ef https://hg.mozilla.org/integration/mozilla-inbound/rev/8373b1ce47f0 Thanks for the detailed reviews! :)
Comment 29•11 years ago
|
||
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
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•