Closed Bug 777591 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 18
Tracking Status
firefox17 --- wontfix
firefox18 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch WIP-separate-jars.patch (obsolete) — Splinter Review
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".
Attachment #645971 - Flags: review?(blassey.bugs)
Attachment #645971 - Flags: feedback?(joey)
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
Attachment #645971 - Flags: review?(blassey.bugs) → review-
Blocks: 743998
Attachment #645971 - Flags: feedback?(joey)
Blocks: 778468
Summary: Link Android .dex from .jar files, not .class files → Compile new namespaces packages into separate .jar packages (and link .dex from .jar files, not .class files)
No longer blocks: 778468
Depends on: 778468
Blocks: 780209
Depends on: 783954
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.
Attachment #645971 - Attachment is obsolete: true
Attachment #654004 - Flags: review?(khuey)
Attachment #654004 - Flags: review?(blassey.bugs)
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.
Attachment #654005 - Flags: review?(blassey.bugs)
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
Attachment #654004 - Flags: review?(blassey.bugs) → review-
Attachment #654005 - Flags: review?(blassey.bugs) → review+
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.
Attachment #654004 - Attachment is obsolete: true
Attachment #654004 - Flags: review?(khuey)
Attachment #657022 - Flags: review?
Comment on attachment 657022 [details] [diff] [review]
part-1-compile-separate-jars-v2.patch

(I forgot to r=? blassey.)
Attachment #657022 - Flags: review? → review?(blassey.bugs)
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.
Attachment #657022 - Flags: review?(blassey.bugs) → review+
Blocks: 788282
(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.
Landed patch #1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57237ee1f788
Whiteboard: [leave open]
Target Milestone: Firefox 17 → Firefox 18
https://hg.mozilla.org/mozilla-central/rev/57237ee1f788
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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.
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.
Depends on: 789124
You need to log in before you can comment on or make changes to this bug.