777591 - Compile new namespaces packages into separate .jar packages (and link .dex from .jar files, not .class files)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Chris Peterson [:cpeterson]]

Attachment: WIP-separate-jars.patch

Linking our Android .dex file from .jar files, rather than .class files, is a baby step towards modularizing the big ball o' Java .class files we have today.

As a proof of concept, this patch creates two .jar files, *arbitrarily* chosen to be class files in the "ch.*" and "org.*" top-level namespaces.

* Reviewers: is there a standard Mozilla convention for creating and cleaning up intermediate build directories? This patch creates an intermediate directory called "jars".

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 645971 [details] [diff] [review]
WIP-separate-jars.patch

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

Make a separate makefile each subdir/subpackage

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-1-compile-separate-jars.patch

Part 1: Compile Java packages into separate jar files.

My previous patch was r-'d with the recommendation to create separate directories and makefiles for each Java package. Since I am postponing my work on our Java refactoring, I would like to land this Makefile patch as a temporary fix.

This patch compiles a separate jar file for each of the Java packages I have isolated to date:

  sync-thirdparty.jar (third-party libraries needed by Sync)
  gecko-mozglue.jar   (misc JNI methods with no other Gecko dependencies)
  gecko-util.jar      (misc utility classes with no other Gecko dependencies)
  gecko-browser.jar   (everything else that depends on the three previous jars)

Compiling and linking separate jar ensures that "expedient" dependencies do not quietly creep into these new packages.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-2-fix-deprecation-warning.patch

Part 2: Fix Favicon deprecation warning.

We can compile each jar with different -Xlint warning flags. This patch fixes some deprecation warnings in gecko-util.jar and removes `-deprecation` from that jar.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 654004 [details] [diff] [review]
part-1-compile-separate-jars.patch

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

::: mobile/android/base/Makefile.in
@@ +1064,5 @@
> +	$(JAVAC) $(JAVAC_FLAGS) -d classes/sync-thirdparty $(addprefix $(srcdir)/,$(SYNC_THIRDPARTY_JAVA_FILES))
> +	jar cMf jars/sync-thirdparty.jar -C classes/sync-thirdparty .
> +
> +	$(NSINSTALL) -D classes/gecko-mozglue
> +	$(JAVAC) $(JAVAC_FLAGS) -Xlint:all -d classes/gecko-mozglue $(addprefix $(srcdir)/,$(MOZGLUE_JAVA_FILES))

have separate rules for each jar you create and then make classes.dex depend on those jars

Comment 5

[Chris Peterson [:cpeterson]]

Patch #2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/012b2f6825ee

Comment 6

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/012b2f6825ee

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: part-1-compile-separate-jars-v2.patch

Patch part 1 v2 adds separate makefile rules (and dependencies) for each jar file. If (say) AwesomeBar.java changes, we no longer recompile mozglue/LibraryLoader.java or util/FloatUtils.java.

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 657022 [details] [diff] [review]
part-1-compile-separate-jars-v2.patch

(I forgot to r=? blassey.)

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 657022 [details] [diff] [review]
part-1-compile-separate-jars-v2.patch

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

::: mobile/android/base/Makefile.in
@@ +1079,5 @@
> +	$(NSINSTALL) -D classes/gecko-mozglue
> +	$(JAVAC) $(JAVAC_FLAGS) -Xlint:all -d classes/gecko-mozglue $(addprefix $(srcdir)/,$(MOZGLUE_JAVA_FILES))
> +	jar cMf jars/gecko-mozglue.jar -C classes/gecko-mozglue .
> +
> +jars/gecko-util.jar: $(addprefix $(srcdir)/,$(UTIL_JAVA_FILES)) jars

you can probably have a generic rule for creating jars here, like:

SUB_JARS=jars/gecko-mozglue.jar jars/gecko-util.jar
PER_JAR_SOURCE_FILES=$(MOZGLUE_JAVA_FILES) $(UTIL_JAVA_FILES)

$(SUB_JARS): $(addprefix $(srcdir)/,$(PER_JAR_SOURCE_FILES)) jars
...

But, I don't want to rat hole on this any further, please just file a follow up bug for it.

Comment 10

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] from comment #9)
> you can probably have a generic rule for creating jars here, like:
> 
> SUB_JARS=jars/gecko-mozglue.jar jars/gecko-util.jar
> PER_JAR_SOURCE_FILES=$(MOZGLUE_JAVA_FILES) $(UTIL_JAVA_FILES)
> 
> $(SUB_JARS): $(addprefix $(srcdir)/,$(PER_JAR_SOURCE_FILES)) jars
> ...
> 
> But, I don't want to rat hole on this any further, please just file a follow
> up bug for it.

I opened bug 788282 to consolidate the jar creation boilerplate code.

Comment 11

[Chris Peterson [:cpeterson]]

Landed patch #1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57237ee1f788

Comment 12

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/57237ee1f788

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Chris Peterson (:cpeterson) from comment #11)
> Landed patch #1:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/57237ee1f788

Seems to have been landed without review from a build system peer.

Comment 14

[Chris Peterson [:cpeterson]]

khuey, would you like to retroactively review this Android makefile change? I can back it out or post a follow-up patch if you want changes. The patch was originally r+'d by blassey.

> > https://hg.mozilla.org/integration/mozilla-inbound/rev/57237ee1f788


(In reply to :Ms2ger from comment #13)
> (In reply to Chris Peterson (:cpeterson) from comment #11)
> > Landed patch #1:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/57237ee1f788
> 
> Seems to have been landed without review from a build system peer.