Closed
Bug 1056128
Opened 10 years ago
Closed 10 years ago
Enable source level 7
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Firefox Build System
Android Studio and Gradle Integration
All
Android
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla34
People
(Reporter: ckitching, Assigned: ckitching)
References
Details
Attachments
(2 files, 6 obsolete files)
23.22 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
2.47 KB,
patch
|
nalexander
:
review+
glandium
:
review+
|
Details | Diff | Splinter Review |
Further to the mailing list post: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-August/000849.html We have Java 7! Woot! So let's enable source level 7 so we can start taking advantage of those shiny language features (almost all of which are syntactic sugar, but shhh).
Assignee | ||
Comment 1•10 years ago
|
||
Assuming I haven't misunderstood something, this seems to be all that's required. Works for me, certainly...
Attachment #8475929 -
Flags: review?(nalexander)
Updated•10 years ago
|
OS: Linux → Android
Hardware: x86_64 → All
Comment 2•10 years ago
|
||
Comment on attachment 8475929 [details] [diff] [review] Enable source level 7. Review of attachment 8475929 [details] [diff] [review]: ----------------------------------------------------------------- Try build needed, for sure, but some mxr-ing suggests this might be everything \o/ We will want to improve android.m4 to complain about 1.6 if we land this, since otherwise I think you'll find out very late in the build (app tier) and have to reconfigure, which clobbers. Tiny nit: please make the commit message more informative, perhaps: "Enable JDK source level 1.7 for Android Java code." Then you'll hit it grepping for JDK, Java, 1.7, and Android.
Attachment #8475929 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=861282a147d4
Assignee | ||
Comment 4•10 years ago
|
||
Found another dependency lurking on my patch queue. Java 7 adds some new compiler warnings. Only one of them ruins our day, and it's actually sort of bad. UiAsyncTask has copied a very odd pattern from Android's AsyncTask: the combination of varags with type parameters. That's something you should never, ever do. Varargs is reduced to an array, and Java doesn't support generics on arrays. As a result, the effect of writing `Params...` is equivalent to just saying "don't typecheck this". You can put literally *anything* in there, which can cause all kinds of "fun". See: http://docs.oracle.com/javase/tutorial/java/generics/nonReifiableVarargsType.html#vulnerabilities This is really fucking stupid behaviour by Java, to be honest. I suggest we simply replace every use of `Param...` with `Param`. Since Param can take on array types, this doesn't restrict us to a single parameter (remember that varargs parameters are secretly to a single array parameter, so this situation is literally equivalent). This does, however, allow some type-checking on Param. What we lose is the fluency of varargs. Notably, when Param is of type Void it is necessary to pass a null as the only parameter. That's ugly (conflating Void-null with real-null?), so I went made a specialised class for handling that case. (Hopefully reading the changes to UiAsyncTask will make this make sense). As it happens, we never actually *use* Param anyway. Every one of our UiAsyncTasks currently has no parameters. That's probably not a sufficiently good reason to remove this ability from the API, though. One last alternative is to just turn off the unchecked warnings for this jar. Not ideal. Try push of both: https://tbpl.mozilla.org/?tree=Try&rev=7d28556d8a4a
Assignee: nobody → chriskitching
Status: NEW → ASSIGNED
Attachment #8476089 -
Flags: review?(nalexander)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8476089 -
Attachment is obsolete: true
Attachment #8476089 -
Flags: review?(nalexander)
Attachment #8476099 -
Flags: review?(nalexander)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8476099 [details] [diff] [review] Improve type safety in UiAsyncTask. Actually, this is probably more of a lucasr-flavoured patch. (unless I've created a race condition in nalexander by doing this, that is)
Attachment #8476099 -
Flags: review?(nalexander) → review?(lucasr.at.mozilla)
Comment 7•10 years ago
|
||
Comment on attachment 8476099 [details] [diff] [review] Improve type safety in UiAsyncTask. Review of attachment 8476099 [details] [diff] [review]: ----------------------------------------------------------------- fly by: I like UiAsyncTask.WithoutParams. That's great. (There's an argument for being AsyncTask compatible vs. having an expressive API; I like the expressive API in this case.) I'm not convinced avoiding a well known, fairly well understood issue with Java generics is worth the churn in UiAsyncTask; we could avoid a lot just by adding .WithoutParams as a convenience. I don't feel strongly. ::: mobile/android/base/GeckoAppShell.java @@ +122,5 @@ > import android.webkit.URLUtil; > import android.widget.AbsoluteLayout; > import android.widget.Toast; > > +public class wat?
Attachment #8476099 -
Flags: feedback+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #7) > Comment on attachment 8476099 [details] [diff] [review] > Improve type safety in UiAsyncTask. > > Review of attachment 8476099 [details] [diff] [review]: > ----------------------------------------------------------------- > > fly by: I like UiAsyncTask.WithoutParams. That's great. (There's an > argument for being AsyncTask compatible vs. having an expressive API; I like > the expressive API in this case.) > > I'm not convinced avoiding a well known, fairly well understood issue with > Java generics is worth the churn in UiAsyncTask; we could avoid a lot just > by adding .WithoutParams as a convenience. I don't feel strongly. Ordinarily I'd agree, but in this case it creates a compiler warning that stops us building with source level 1.7. To disable the warning, we have to disable the "unchecked" warning type class, which also turns off a bunch of stuff we kinda want to keep (and have been having all along anyway). So, possible solutions I know of are: Make UiAsyncTask build without the warnings (but perhaps in a better way than I just did). Add "-Xlint:-unchecked" and lose checking of other generics problems (it'd no longer warn about stuff like List<String> cake = new ArrayList(), for example). Disable -werror so we can tolerate the warning. Use of @SafeVarags is a non-solution: It cannot be applied to abstract methods, so we can't use it in this case (and it's an evil hack at the best of times).
Comment 9•10 years ago
|
||
(In reply to Chris Kitching [:ckitching] from comment #8) > (In reply to Nick Alexander :nalexander from comment #7) > > Comment on attachment 8476099 [details] [diff] [review] > > Improve type safety in UiAsyncTask. > > > > Review of attachment 8476099 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > fly by: I like UiAsyncTask.WithoutParams. That's great. (There's an > > argument for being AsyncTask compatible vs. having an expressive API; I like > > the expressive API in this case.) > > > > I'm not convinced avoiding a well known, fairly well understood issue with > > Java generics is worth the churn in UiAsyncTask; we could avoid a lot just > > by adding .WithoutParams as a convenience. I don't feel strongly. > > Ordinarily I'd agree, but in this case it creates a compiler warning that > stops us building with source level 1.7. Ah, right, I lost this context. > To disable the warning, we have to disable the "unchecked" warning type > class, which also turns off a bunch of stuff we kinda want to keep (and have > been having all along anyway). > > So, possible solutions I know of are: > > Make UiAsyncTask build without the warnings (but perhaps in a better way > than I just did). I think if there was a clear way, you'd know it. > Add "-Xlint:-unchecked" and lose checking of other generics problems (it'd > no longer warn about stuff like List<String> cake = new ArrayList(), for > example). > Disable -werror so we can tolerate the warning. I agree that this is too valuable to disable. > Use of @SafeVarags is a non-solution: It cannot be applied to abstract > methods, so we can't use it in this case (and it's an evil hack at the best > of times). That's unfortunate. Thanks for the clear discussion of the tradeoffs. Can this land /before/ we flip to Java 1.7?
Assignee | ||
Comment 10•10 years ago
|
||
Churn in UiAsyncTask drastically reduced by undoing the movement of the inner class. By returning it and execute() to their original place the patch gets a great deal simpler.
Attachment #8476099 -
Attachment is obsolete: true
Attachment #8476099 -
Flags: review?(lucasr.at.mozilla)
Attachment #8476985 -
Flags: review?(lucasr.at.mozilla)
Comment 11•10 years ago
|
||
Comment on attachment 8476985 [details] [diff] [review] V2: Improve type safety in UiAsyncTask. Review of attachment 8476985 [details] [diff] [review]: ----------------------------------------------------------------- Cleaner patch is cleaner. LGTM. ::: mobile/android/base/util/UiAsyncTask.java @@ +31,5 @@ > + /** > + * Provide a convenient API for parameter-free UiAsyncTasks by wrapping parameter-taking methods > + * from UiAsyncTask in parameterless equivalents. > + */ > + public static abstract class WithoutParams<NPResult> extends UiAsyncTask<Void, NPResult> { nit: I assume you have to avoid shadowing Result, and NPResult is some shortening of "no parameter result". Consider "InnerResult" or "ResultWithoutParams" or if you must "WPResult". @@ +41,5 @@ > + execute(null); > + } > + > + @Override > + protected NPResult doInBackground(Void params) { Maybe "Void param" for clarity? @@ +70,5 @@ > return sHandler; > } > > private final class BackgroundTaskRunnable implements Runnable { > + private Param mParams; s/mParams/mParam/. Throughout, actually.
Attachment #8476985 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 12•10 years ago
|
||
Comment on attachment 8476099 [details] [diff] [review] Improve type safety in UiAsyncTask. Review of attachment 8476099 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by. Maybe ask lucasr for review, or feedback at least? He might know why we we copied the AsyncTask pattern even if we don't use all of it. I noticed you dropped Progress, but looking further, I guess we don't use it. ::: mobile/android/base/util/UiAsyncTask.java @@ +24,5 @@ > + * http://docs.oracle.com/javase/tutorial/java/generics/nonReifiableVarargsType.html#vulnerabilities > + * > + * Instead, we equivalently have a single type parameter "Param". A UiAsyncTask may take exactly one > + * parameter of type Param. Since Param can be an array type, this no more restrictive than the > + * other approach, it just provides additional type safety. I don't think this full explanation needs to be in our source code. You could simply keep the first sentence, and then say "See bug <this-bug>." @@ +29,2 @@ > */ > +public abstract class UiAsyncTask<Param, Result> { If we're already changing all this, maybe we could take this opportunity to change this naming to UIAsynceTask.
Assignee | ||
Comment 13•10 years ago
|
||
Now with autoconf. Autoconf is a truly horrible language to use, it turns out. Still, this seems to work (tested with javac 1.5, 1.6, 1.7, 1.8 - I didn't have any others installed, but it seems to do the trick. :P )
Attachment #8475929 -
Attachment is obsolete: true
Attachment #8477114 -
Flags: review?(nalexander)
Assignee | ||
Comment 14•10 years ago
|
||
... Now with more qref.
Attachment #8477114 -
Attachment is obsolete: true
Attachment #8477114 -
Flags: review?(nalexander)
Attachment #8477116 -
Flags: review?(nalexander)
Comment 15•10 years ago
|
||
Comment on attachment 8477116 [details] [diff] [review] V3: Enable source level 7. Review of attachment 8477116 [details] [diff] [review]: ----------------------------------------------------------------- Try build, post to mobile-firefox-dev, and bombs away. Thanks for teaching me some configure! ::: config/android-common.mk @@ +18,5 @@ > ifndef JAVA_BOOTCLASSPATH > JAVA_BOOTCLASSPATH = $(ANDROID_SDK)/android.jar > endif > > # For Android, we default to 1.5 nit: update the comment, too :) ::: configure.in @@ +5642,5 @@ > fi > > +AC_MSG_CHECKING([for minimum required javac version >= 1.7]) > + > +_javac_version=$($JAVAC -version 2>&1 | cut -d ' ' -f 2) Would you mind spelling out what this is doing, something like: # Turn "javac 1.7.0_60" into "1.7.0_60". and below: # Turn "1.7.0_60" into "1" and "7", respectively.
Attachment #8477116 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 16•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6cad5b2e9169
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5af48430563e
Comment 18•10 years ago
|
||
This patch now makes *every* app require javac to build, even though it is only for Android - do we really want that? If so, please detail on dev-platform or somewhere about the new requirement & how to get it. Also, shouldn't this have had build-config peer review?
Comment 19•10 years ago
|
||
Note: I found this as my Mac only has 1.6.0_65 installed by default.
Comment 20•10 years ago
|
||
Please backout.
Comment 21•10 years ago
|
||
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #20) > Please backout. backed this out on request from glandium via irc
Comment 22•10 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #18) > This patch now makes *every* app require javac to build, even though it is > only for Android - do we really want that? No; this was an oversight on my part. > If so, please detail on dev-platform or somewhere about the new requirement > & how to get it. > > Also, shouldn't this have had build-config peer review? I am a build peer for Android stuff. I certainly didn't intend to skirt build peer review :)
Assignee | ||
Comment 23•10 years ago
|
||
Whoops. Okay, so just move the new checks inside the conditional that makes sure we're building something that cares about Java and we're done. (In reply to Mark Banner (:standard8) from comment #19) > Note: I found this as my Mac only has 1.6.0_65 installed by default. Wow. Java 7 has been out since 2011...
Attachment #8477116 -
Attachment is obsolete: true
Attachment #8479474 -
Flags: review?(nalexander)
Attachment #8479474 -
Flags: review?(mh+mozilla)
Comment 24•10 years ago
|
||
Comment on attachment 8479474 [details] [diff] [review] V3: Enable source level 7.4 Review of attachment 8479474 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +5620,5 @@ > + > + dnl Fail if we have a version older than 1.7 > + if test "$_javac_major_version" -lt "1" -o \ > + \( "$_javac_minor_version" -lt "7" \); then > + AC_MSG_ERROR([javac 1.7 or higher is required.]) This test would fail for versions 2.0 to 2.6. That could sound unlikely, but 1.9 is already on track and who knows what the version will be after that. Better safe than randomly breaking in the future.
Attachment #8479474 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #24) > This test would fail for versions 2.0 to 2.6. That could sound unlikely, but > 1.9 is already on track and who knows what the version will be after that. > Better safe than randomly breaking in the future. The thinking was that anything sufficiently different to be called "Java 2.0" is unlikely to work well for us out of the box. ... But in which case the error message is stupid. In any case, builds with JDK 8 and up fail anyway (new compiler warnings get tripped by some of our sloppy code), so I'll change it to require exactly 7.
Attachment #8479474 -
Attachment is obsolete: true
Attachment #8479474 -
Flags: review?(nalexander)
Attachment #8480216 -
Flags: review?(nalexander)
Attachment #8480216 -
Flags: review?(mh+mozilla)
Comment 26•10 years ago
|
||
Comment on attachment 8480216 [details] [diff] [review] V4: Enable source level 7. Review of attachment 8480216 [details] [diff] [review]: ----------------------------------------------------------------- Fine by me if it's fine by glandium. ::: configure.in @@ +5612,5 @@ > fi > + > + AC_MSG_CHECKING([for minimum required javac version = 1.7]) > + > + dnl Javac spits out something like `javac 1.8.0`. This line cuts off the 'javac' These should be 1.7.0 and (7) below, since conceivably this would change, and we want to reference what is expected.
Attachment #8480216 -
Flags: review?(nalexander) → review+
Updated•10 years ago
|
Attachment #8480216 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ce756025b879
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce756025b879
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 34 → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•