1064554 - Move build/annotationProcessors into mobile/android

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Nick Alexander :nalexander [he/him]]

This is part good hygiene and part a piece of a plan that ckitching proposes to improve Fennec's annotation processing.

We should get the mobile/android specific code into mobile/android.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: WIP on moving annotation processors into mobile/android.

This also starts building a gecko-annotations.jar.  Both this JAR and
annotationProcessors.jar are built before mobile/android/base, so they
can be linked and used there.

ckitching: here's some WIP that should get you past the one issue.  A
comment and a concern:

* I'd be pretty happy if we renamed annotationsProcessors.jar
  something like processors.jar.  CamelCase in the build system feels
  odd.

* I'm a little concerned that IDE users will need to include the
  annotations into the Fennec source tree directly; but we can burn
  that bridge when we get to it.  (I vaguely remember that missing
  annos are ignored; if that's the case, please confirm and rock on.)

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Move annotation processor into mobile/android

Mostly fleshed-out, though oddly builds are failing with https://pastebin.mozilla.org/6392557 . More magic needed.

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8486142 [details] [diff] [review]
Move annotation processor into mobile/android

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

I can't apply this patch cleanly, and it doesn't appear to build on the earlier patch.  It modifies build/annotationProcessors rather than working with the copy I had, for example; and the new m/a/annotations/processors appears malformed.  Pushing a gecko-dev git branch is always good...

Comment 4

[Chris Kitching [:ckitching]]

Ah, whoops, I appear to have merged my changes with your earlier patch.

I'll untangle that... (but you might be able to apply this cleanly by itself in the meantime)

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Part 1/4: Move the annotation processor and annotation classes

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Part 2/4: Update Proguard config for new annotation package names

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Part 3/4: Fix imports of users of annotation classes

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Part 4/4: Remove unnecessary Proguard annotations from testDistribution

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Part 1/4: Move the annotation processor and annotation classes

Now also renaming the jar from annotationProcessors to annotation-processors, as you commented before that the old name was strange.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8486890 [details] [diff] [review]
Part 4/4: Remove unnecessary Proguard annotations from testDistribution

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

I think it makes the most sense to land this before everything else.

Comment 11

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/db9eb06fcc38

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8486919 [details] [diff] [review]
Part 1/4: Move the annotation processor and annotation classes

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

This looks good, but I want to see a try build for the whole sequence.

Also, can you sketch how the proper annotation processing will look?  (That might be in a new dependent ticket.)  I'd like to have a notion of the payoff before we rush to land this.  I'm a little concerned that you're going to have to write multiple outputs in an incremental fashion (since we have multiple javac invocations).  Multiple outputs in Make are rough territory -- look at aapt and .aapt.deps, for example -- but I can help if I understand what you're up to.

So this is r+ but I'd like to iterate before landing.

::: build/mobile/robocop/Makefile.in
@@ +89,5 @@
>  # The test APK needs to know the contents of the target APK while not
>  # being linked against them.  This is a best effort to avoid getting
>  # out of sync with base's build config.
>  JARS_DIR := $(DEPTH)/mobile/android/base
> +JAVA_BOOTCLASSPATH := $(JAVA_BOOTCLASSPATH):$(subst $(NULL) ,:,$(wildcard $(JARS_DIR)/*.jar)):$(DEPTH)/mobile/android/annotations/definitions/gecko-annotations.jar:$(ANDROID_COMPAT_LIB)

I'm a little surprised by this.  Is this strictly necessary?  There are no annos in the Robocop package itself; and even if there were, I thought missing annos would be ignored.  The Fennec jars are already built.  Adding them to the BCP will only expose symbols to the CP, not require additional symbols.  Educate me.

My guess is that if this is necessary, we'll need to update geckoview_library to include this jar as well.  A single jar output location is sounding reasonable, isn't it?  Some other life.

::: mobile/android/annotations/definitions/Makefile.in
@@ +6,5 @@
> +
> +JAVA_CLASSPATH := $(ANDROID_SDK)/android.jar
> +include $(topsrcdir)/config/android-common.mk
> +
> +libs:: gecko-annotations.jar

Bug 106464 will get rid of this pattern, but for now, roll on.

::: mobile/android/annotations/definitions/generatorannotations/GeneratorOptions.java
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +package org.mozilla.annotations.generatorannotations;

How would you feel about a follow-up to shorten this?  Maybe o.m.annotations.generators?

::: mobile/android/annotations/processors/AnnotationProcessor.java
@@ +25,5 @@
>              "// methods and rerun the build. Manually updating this file will cause your build to fail.\n\n";
>  
>      public static void main(String[] args) {
> +
> +        System.err.println("Classpath:");

This will need to go.

@@ +32,3 @@
>          // We expect a list of jars on the commandline. If missing, whinge about it.
>          if (args.length <= 1) {
>              System.err.println("Usage: java AnnotationProcessor jarfiles ...");

It's conventional to fish the program name from |args[0]|; is that feasible here?

::: mobile/android/base/Makefile.in
@@ +178,3 @@
>  
>  GeneratedJNIWrappers.cpp: $(ANNOTATION_PROCESSOR_JAR_FILES)
> +GeneratedJNIWrappers.cpp: $(ALL_JARS) 

nit: trailing ws, and this echo needs to go.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8486889 [details] [diff] [review]
Part 3/4: Fix imports of users of annotation classes

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

Have a rubber stamp.  I'm going to assume this was auto-generated :)

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8486888 [details] [diff] [review]
Part 2/4: Update Proguard config for new annotation package names

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

I gave this a skim; is there anything not s/// here?

Comment 15

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #14)
> I gave this a skim; is there anything not s/// here?

Nope.

(In reply to Nick Alexander :nalexander from comment #13)
> Have a rubber stamp.  I'm going to assume this was auto-generated :)

Yup.

(In reply to Nick Alexander :nalexander from comment #12)
> Also, can you sketch how the proper annotation processing will look?  (That
> might be in a new dependent ticket.)  I'd like to have a notion of the
> payoff before we rush to land this.  I'm a little concerned that you're
> going to have to write multiple outputs in an incremental fashion (since we
> have multiple javac invocations).  Multiple outputs in Make are rough
> territory -- look at aapt and .aapt.deps, for example -- but I can help if I
> understand what you're up to.

When generating the C++ code in the annotation processor, you mean?

My solution to that problem at the moment is to simply write one file safely (so the fact that we're trying to write to it from a bunch of different invocations, possibly concurrently, in a random order, doesn't change the output).

The output of each invocation will be a set of C++ classes. These need to be written into the file, but they need to always end up in the same order in the output file (the processor also ensures elements *within* each generated class end up in the same order each time: simply by sorting them).
We also need to make sure that no two invocations try and write the output file concurrently: that can simply be achieved with file locking.

Next, we need to make sure each output blob ends up in the file in the same order with respect to all the others. That can be done with a bit of metadata: each output operation writes (somewhere) a unique key (currently the name of the first class) along with the length of the output blob.
So, each invocation:

1) Computes its output (independent of any other invocations)
2) Takes the file lock.
3) Load the metadata table and use the lexicographic ordering over the keys to work out what offset in the output file its output blob has to be inserted.
4) Write output.
5) Write new metadata.
6) Release lock.

The last question is "where to put the metadata?". My current absurd solution to that problem is in a comment at the top of the file.


Of course, all that's for another bug, as you say, but that's my approach for making the annotation processor into an annotation processor while avoiding the Makefile madness minefield.b

> So this is r+ but I'd like to iterate before landing.
> 
> ::: build/mobile/robocop/Makefile.in
> @@ +89,5 @@
> >  # The test APK needs to know the contents of the target APK while not
> >  # being linked against them.  This is a best effort to avoid getting
> >  # out of sync with base's build config.
> >  JARS_DIR := $(DEPTH)/mobile/android/base
> > +JAVA_BOOTCLASSPATH := $(JAVA_BOOTCLASSPATH):$(subst $(NULL) ,:,$(wildcard $(JARS_DIR)/*.jar)):$(DEPTH)/mobile/android/annotations/definitions/gecko-annotations.jar:$(ANDROID_COMPAT_LIB)
> 
> I'm a little surprised by this.  Is this strictly necessary?  There are no
> annos in the Robocop package itself; and even if there were, I thought
> missing annos would be ignored.  The Fennec jars are already built.  Adding
> them to the BCP will only expose symbols to the CP, not require additional
> symbols.  Educate me.

As far as I know, bootclasspath operations just the same as classpath, except it doesn't shout at you if you try and interact with a forbidden package. It lets you do things like redefine classes in java.lang, which classpath doesn't let you do (it'll error out if you try it).

Though, it is late and I'm hungry, so I might be talking rubbish.

As for the requireness of this: more investigation needed, which I'll do tomorrow morning. I think you should be right, but I believe it spits out an un-turn-off-able compiler warning while it does so, which is deeply irritating.
It'd be very nice to be able to not do this, not least because it would make it impossible for people to stick annotations in here in error.

> My guess is that if this is necessary, we'll need to update
> geckoview_library to include this jar as well.  A single jar output location
> is sounding reasonable, isn't it?  Some other life.

Very little about geckoview seems pleasant, from what little I know about it.

> :::
> mobile/android/annotations/definitions/generatorannotations/GeneratorOptions.
> java
> @@ +1,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +package org.mozilla.annotations.generatorannotations;
> 
> How would you feel about a follow-up to shorten this?  Maybe
> o.m.annotations.generators?

Go for it, I don't really mind.

> ::: mobile/android/annotations/processors/AnnotationProcessor.java
> @@ +25,5 @@
> >              "// methods and rerun the build. Manually updating this file will cause your build to fail.\n\n";
> >  
> >      public static void main(String[] args) {
> > +
> > +        System.err.println("Classpath:");
> 
> This will need to go.

Ah, debugging code. Whoops.

> @@ +32,3 @@
> >          // We expect a list of jars on the commandline. If missing, whinge about it.
> >          if (args.length <= 1) {
> >              System.err.println("Usage: java AnnotationProcessor jarfiles ...");
> 
> It's conventional to fish the program name from |args[0]|; is that feasible
> here?

Hilariously, Java breaks this convention. args[0] is the first argument, not the program name. Finding the program name is actually not trivial:
http://stackoverflow.com/questions/41894/0-program-name-in-java-discover-main-class
http://stackoverflow.com/questions/11158235/get-name-of-executable-jar-from-within-main-method

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db9eb06fcc38

Comment 17

[Chris Kitching [:ckitching]]

(In reply to Chris Kitching [:ckitching] from comment #15)
> As far as I know, bootclasspath operations just the same as classpath,
> except it doesn't shout at you if you try and interact with a forbidden
> package. It lets you do things like redefine classes in java.lang, which
> classpath doesn't let you do (it'll error out if you try it).

Hmm, so after re-reading:
http://docs.oracle.com/javase/7/docs/technotes/tools/findingclasses.html

It definitely seems like bootclasspath = "classpath that doesn't yell at you if you redefine the platform (such as things in java.lang)". Did I miss something, or is your use of it in robocop not really achieving anything? (it's just the same as if you'd used classpath, isn't it?)

Java doesn't have anything that works brilliantly for "just exposing symbols". In this case, it's trying to load the annotation classes, failing, and printing a warning: https://pastebin.mozilla.org/6431414 . This should be fine, if we can just make it shut up... (I can't seem to find where to stick the flags for Robocop, and I'm not entirely sure there *is* a flag to turn this warning off, other than "nowarn", or disabling werror, neither of which I especially like). You're right, missing annotations are ignored, but Java whines about it first.

(I should also add that linking in Java is done entirely by name. Provided the compiler can find stuff with the right names on the classpath, it'll spit out code to the effect of "Load the class org.mozilla.gecko.Whatever and access method doSomething(String):void". Yes, this does mean the fully-qualified class name of everything is sprinkled liberally though the binary. We'd actually see meaningful binary size reduction if we s/org.mozilla.gecko//. At runtime, the system just follows the rules for finding classes to search the classpath until it finds something with the right name, which it uses.)

On a related note, did you register that the fact we do this with Robocop, and the fact we don't strip the RobocopTarget-annotated symbols from release builds, means that our Proguard optimisation performance is inversely proportional to our test coverage? The solution there is to omit @RobocopTarget from the list of things Proguard considers a sentinel annotation when doing a release build (which will allow it to delete the code that's only for testing, as well as inline things more aggressively, and so on).
I digress...

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Kitching [:ckitching] from comment #17)
> (In reply to Chris Kitching [:ckitching] from comment #15)
> > As far as I know, bootclasspath operations just the same as classpath,
> > except it doesn't shout at you if you try and interact with a forbidden
> > package. It lets you do things like redefine classes in java.lang, which
> > classpath doesn't let you do (it'll error out if you try it).
> 
> Hmm, so after re-reading:
> http://docs.oracle.com/javase/7/docs/technotes/tools/findingclasses.html
> 
> It definitely seems like bootclasspath = "classpath that doesn't yell at you
> if you redefine the platform (such as things in java.lang)". Did I miss
> something, or is your use of it in robocop not really achieving anything?
> (it's just the same as if you'd used classpath, isn't it?)

Huh, I was not aware of this distinction.  Thanks for the education.

> Java doesn't have anything that works brilliantly for "just exposing
> symbols". In this case, it's trying to load the annotation classes, failing,
> and printing a warning: https://pastebin.mozilla.org/6431414 . This should
> be fine, if we can just make it shut up... (I can't seem to find where to
> stick the flags for Robocop, and I'm not entirely sure there *is* a flag to
> turn this warning off, other than "nowarn", or disabling werror, neither of
> which I especially like). You're right, missing annotations are ignored, but
> Java whines about it first.
> 
> (I should also add that linking in Java is done entirely by name. Provided
> the compiler can find stuff with the right names on the classpath, it'll
> spit out code to the effect of "Load the class org.mozilla.gecko.Whatever
> and access method doSomething(String):void". Yes, this does mean the
> fully-qualified class name of everything is sprinkled liberally though the
> binary. We'd actually see meaningful binary size reduction if we
> s/org.mozilla.gecko//. At runtime, the system just follows the rules for
> finding classes to search the classpath until it finds something with the
> right name, which it uses.)

Yes, I'm aware of this -- it's one of the reasons I think pushing on Proguard obfuscation (which does these sorts of transformations) might be valuable.  I wonder if we can de-obfuscate Java stacks -- if we can't our crash reporting tools are going to have a hard time correlating crashes over time.  That's a big no go :(

> On a related note, did you register that the fact we do this with Robocop,
> and the fact we don't strip the RobocopTarget-annotated symbols from release
> builds, means that our Proguard optimisation performance is inversely
> proportional to our test coverage? The solution there is to omit
> @RobocopTarget from the list of things Proguard considers a sentinel
> annotation when doing a release build (which will allow it to delete the
> code that's only for testing, as well as inline things more aggressively,
> and so on).

I am aware.  I haven't filed yet, mostly because I don't have a good handle on what we should do.  I am working on some test packaging code in a different ticket, and I'm mooting whether we could:

1) move the Proguard phase into the packaging process; and
2) package (and upload) both the release and debug versions of the APK.

Alternately, we might package a debug version of Fennec into the tests APK, just for the use of our instrumentation tests.

Open question: how much simpler do the annotation processor bits get if you don't have to worry about RobocopTarget?  My understanding is that RobocopTarget is just a retention policy, so it's basically free for us.

Comment 19

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #18)
> Yes, I'm aware of this -- it's one of the reasons I think pushing on
> Proguard obfuscation (which does these sorts of transformations) might be
> valuable.  I wonder if we can de-obfuscate Java stacks -- if we can't our
> crash reporting tools are going to have a hard time correlating crashes over
> time.  That's a big no go :(

Proguard's obfuscation step spits out a file that maps mangled names to original names. Proguard also provides a utility that takes a stacktrace, the corresponding mapping file, and spits out an unscrambled stacktrace.
All we need to do is combine that with our crashstats system. Naturally you'll need to keep a collection of these mapping files for each build of Fennec that's about (though Proguard supports "incremental obfuscation"

See:
http://proguard.sourceforge.net/manual/usage.html#obfuscationoptions
)

As for debugging local builds, IDEA integrates with Proguard's descrambler, you don't even need to run the build through IDEA for this to work. You can say "Everything from this package you see in logcat output needs to be descrambled with this mapping file before display", and then just watch logcat from IDEA's UI (which provides colour coding and hyperlinks to source code in stacktraces, as well as other coolness, anyway).
I imagine Eclipse has a comparable feature.
As for people who still want to party like it's 1970, I think you can do something similar to `adb logcat | proguard-descramble objdir/whatever/somemapping.txt`.

> I am aware.  I haven't filed yet, mostly because I don't have a good handle
> on what we should do.  I am working on some test packaging code in a
> different ticket, and I'm mooting whether we could:
> 
> 1) move the Proguard phase into the packaging process; and
> 2) package (and upload) both the release and debug versions of the APK.

Upload to where? The Play store?
Generally, moving Proguard into the packaging step seems like a good idea if you want to apply it twice in two different ways. The interesting part of the Proguard config is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/config/proguard.cfg#142

You'd just need to rerun Proguard on the program classes, omitting that chunk from the config, and you'll get a build with the test-specific code stripped.

You could also just not run proguard on the test builds. If we did that, I'd at least want us to have a test which makes sure all the appropriate sentinel annotations are in place to ensure it's not about to delete something essential. (I did some work in this direction over in Bug 944553, but it was the most ridiculous hack you'll ever see (ever seen a sed script with GOTOs? In a Java program?)). Of course, in general, figuring that out at build time is the halting problem (though virtually all sane code specifies its target Java method as a literal or constant, so we ought to be able to work it out almost all the time). 
We could instead have a neat little test that just tries to call every JNI entry point to verify they still exist, but that's got somewhat larger turnaround when someone makes a mistake.

> Alternately, we might package a debug version of Fennec into the tests APK,
> just for the use of our instrumentation tests.

That could work.
One extra thing to consider: when doing a test build you could apply Proguard to the test classes and the Fennec classes at once. If you do this, you can delete all RobocopTarget annotations, since Proguard will now realise that the things referenced from the tests are useful, so it won't break tests.
That said, applying Proguard to the test harness seems like a slightly icky thing to do, with potential for much "fun" in the case of Proguard bugs.

> Open question: how much simpler do the annotation processor bits get if you
> don't have to worry about RobocopTarget?  My understanding is that
> RobocopTarget is just a retention policy, so it's basically free for us.

That's right. It's just a sentinel used to stop Proguard from deleting stuff the tests need. 
A handy summary of the annotations we currently have in service:

Sentinels:
@JNITarget: "This thing is referenced from non-generated JNI code as part of Fennec, so is essential and mustn't be deleted"
@RobocopTarget: "Referenced from Robocop, so must be kept to keep the tests happy :("
@WebRTCTarget: "As JNITarget, but for WebRTC code (distinct in case the WebRTC people want to do something special, like transition to generated JNI stubs (Bug 917512 - I wasn't familiar enough with WebRTC at the time and my ulterior motive for all the generated code work was to be able to get Proguard working without requiring 300+ @JNITarget annotations"

Generator:
@WrapElementForJNI: Causes the annotation processor to generate C++ code wrapping this field/method.
@WrapEntireClassForJNI: Behaves as if every member of the class has a @WrapElementForJNI
@OptionalGeneratedParameter: Annotation for method parameters causing them to be optional in the generated C++ wrapper (they get a default value if omitted)
@GeneratorOptions: A place to specify per-class options for the generator, only currently supports setting the name of the generated class to something other than the name of the Java class, which doesn't seem terribly useful and it's sort of unpleasant. It's used only once, on some code that's been being wrapped since the first implementation of the generator and has never, ever been used. So, yes, I'll be killing this.

Proguard is configured to not rename or delete anything with any of the sentinels, or WrapElementForJNI (plus all members), or WrapEntireClassForJNI.


Discussion of robocop issue is offtopic for this bug, I just mentioned it as it's sort of vaguely related. But yes, you're right, RobocopTarget doesn't have any affect at all on the annotation processor's complexity. It's a few lines of Proguard config, and it's a pain to maintain the annotations to keep the tests working.

Comment 20

[Chris Kitching [:ckitching]]

Finally got back to this...

(In reply to Nick Alexander :nalexander from comment #12)
> ::: build/mobile/robocop/Makefile.in
> @@ +89,5 @@
> >  # The test APK needs to know the contents of the target APK while not
> >  # being linked against them.  This is a best effort to avoid getting
> >  # out of sync with base's build config.
> >  JARS_DIR := $(DEPTH)/mobile/android/base
> > +JAVA_BOOTCLASSPATH := $(JAVA_BOOTCLASSPATH):$(subst $(NULL) ,:,$(wildcard $(JARS_DIR)/*.jar)):$(DEPTH)/mobile/android/annotations/definitions/gecko-annotations.jar:$(ANDROID_COMPAT_LIB)
> 
> I'm a little surprised by this.  Is this strictly necessary?  There are no
> annos in the Robocop package itself; and even if there were, I thought
> missing annos would be ignored.  The Fennec jars are already built.  Adding
> them to the BCP will only expose symbols to the CP, not require additional
> symbols.  Educate me.
> 
> My guess is that if this is necessary, we'll need to update
> geckoview_library to include this jar as well.  A single jar output location
> is sounding reasonable, isn't it?  Some other life.

So, I poked this with a stick for a while.
Yes, we don't need the annotations, yes missing annotations are ignored, and no, we can't make javac stop whinging about them.
So, things will compile and work just fine, but we can't stop javac from printing warnings for all of the missing annotations when it loads the classes.

So, our options are:
* Disable -werror
* -nowarn
* Add the classes to Robocop's classpath.

Whichever of these three we pick will be a temporary measure. Consider: Why do we have the annotations in the bytecode at all?

At the moment, they need to be there so they can be loaded by the annotation processor (that isn't *really* an annotation processor). They also need to be there so Proguard can find them and use them as sentinels.
But, hey, we're about to start processing annotations at compile-time. This means:
* Annotations used for generating C++ code will not be used for this purpose after compilation.
* To fix Bug 1048651, we'll need the annotation processor to generate explicit -keep directives for Proguard for return types of annotated methods. If we take the simple step to expand that to generating explicit -keep directives for everything, we have no need of these annotations.

(Conceptually, the proguard-config-generation step is going to be replacing what we currently have ("Keep everything with these annotations on it") with an explicit list of -keeps ("Keep this and keep this and keep this..."))

Once both of those things are done (ie. by Monday, hopefully), we can then set the RetentionPolicy of all these annotations to SOURCE, meaning they won't exist in our bytecode. Robocop will then cease to encounter these warnings, because there will be no annotations present inthe bytecode (and we'll have a very slight bytecode size reduction, too: yay!).

Long story short: I suggest we go with the original patch for now, and come back and remove the annotations from the classpath when we land the other stuff.


... Landing time, then?

Comment 21

[Chris Kitching [:ckitching]]

Attachment: Move the annotation processor and annotation classes

Current patch for your perusal. Has been rebased atop Bug 1067056, as I am ever the optimist. Hasn't meaningfully changed since you last saw it sans the obvious cleanup you spotted.

Comment 22

[Nick Alexander :nalexander [he/him]]

OK, finally coming back to this.  Can you freshen this up and make sure we're green on try?  If we're going to move on this, doing it at the beginning of the cycle is key.

Comment 23

[Chris Kitching [:ckitching]]

https://tbpl.mozilla.org/?tree=Try&rev=fcb1bccb2ec1

Comment 24

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: Move the annotation processor and annotation classes

Comment 25

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Update Proguard config for new annotation package names

Comment 26

[Chris Kitching [:ckitching]]

Attachment: Part 3/3: Update annotations in users of moved annotation classes

Comment 27

[Chris Kitching [:ckitching]]

Patches unbitrotted and pushed to try above. Looks like everything works (just a large amount of the usual intermittent stupidity). Poked it for retrigger, but generally looks like it's golden.


Landing time?

Comment 28

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Kitching [:ckitching] from comment #27)
> Patches unbitrotted and pushed to try above. Looks like everything works
> (just a large amount of the usual intermittent stupidity). Poked it for
> retrigger, but generally looks like it's golden.
> 
> 
> Landing time?

Yes, I intend to test this locally and land today.  Thanks for your hard work and your patience!

Comment 29

[Chris Kitching [:ckitching]]

https://tbpl.mozilla.org/?tree=Try&rev=51d0c3e2bbc5

Comment 30

[Chris Kitching [:ckitching]]

Attachment: Move the annotation processor and annotation classes

Updated for Snorp's new stuff. Essentially the same, added a new Makefile at mobile/annotations/processors which I *think* is following the right conventions.

Comment 31

[Chris Kitching [:ckitching]]

Attachment: Update annotations in users of moved annotation classes

Nothing interesting here.

Comment 32

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8532117 [details] [diff] [review]
Move the annotation processor and annotation classes

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

ckitching: I'm closing these reviews because we've both lost energy for this project.  Sorry that I was some of that stop energy :(  I would like to pick this up again and hope your strong work doesn't die on the vine.

Comment 33

[Nick Alexander :nalexander [he/him]]

I did this work in https://bugzilla.mozilla.org/show_bug.cgi?id=1444546.