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)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: ckitching, Assigned: ckitching)

References

Details

Attachments

(2 files, 6 obsolete files)

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).
Attached patch Enable source level 7. (obsolete) — Splinter Review
Assuming I haven't misunderstood something, this seems to be all that's required.

Works for me, certainly...
Attachment #8475929 - Flags: review?(nalexander)
OS: Linux → Android
Hardware: x86_64 → All
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+
Attached patch uiAsyncTask.patch (obsolete) — Splinter Review
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)
Attachment #8476089 - Attachment is obsolete: true
Attachment #8476089 - Flags: review?(nalexander)
Attachment #8476099 - Flags: review?(nalexander)
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 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+
(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).
(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?
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 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 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.
Attached patch V2: Enable source level 7. (obsolete) — Splinter Review
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)
Attached patch V3: Enable source level 7. (obsolete) — Splinter Review
... Now with more qref.
Attachment #8477114 - Attachment is obsolete: true
Attachment #8477114 - Flags: review?(nalexander)
Attachment #8477116 - Flags: review?(nalexander)
Depends on: 1057086
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+
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?
Note: I found this as my Mac only has 1.6.0_65 installed by default.
Please backout.
(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
(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 :)
Attached patch V3: Enable source level 7.4 (obsolete) — Splinter Review
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 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+
(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 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+
Attachment #8480216 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/ce756025b879
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1062880
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.