923306 - Standardize Java JAR building into rules.mk

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

Bug 919563 standardized building APKs (for all but mobile/android/base); this ticket tracks standardizing building JARs (which only arise in mobile/android/base).  This is a step towards defining a sensible moz.build grammar for JAR files.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Add JAVA_JAR_TARGETS. r=glandium

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Make annotationProcessors build with -Xlint:all. r=glandium

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Build annotationProcessors with JAVA_JAR_TARGETS. r=glandium

Comment 4

[Nick Alexander :nalexander [he/him]]

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

Comment 5

[Chris Kitching [:ckitching]]

Comment on attachment 813349 [details] [diff] [review]
Make annotationProcessors build with -Xlint:all. r=glandium

Ah, yes. Well spotted. There's not really anything in this patch that can go wrong.

Comment 6

[Chris Kitching [:ckitching]]

Comment on attachment 813350 [details] [diff] [review]
Build annotationProcessors with JAVA_JAR_TARGETS. r=glandium

This seems to be much neater than the earlier approach. If it compiles (Which I assume you tried before uploading it) then I can't find anything to complain about.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 813349 [details] [diff] [review]
Make annotationProcessors build with -Xlint:all. r=glandium

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

I'm not the right reviewer for java code. Not sure who's best to review this.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 813348 [details] [diff] [review]
Add JAVA_JAR_TARGETS. r=glandium

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

This is sensible, but i'd like to see at least the include part fixed.

::: config/makefiles/java-build.mk
@@ +100,5 @@
> +#        jars must be relative to $(CURDIR).
> +# Arg 4: Additional JAVAC_FLAGS.
> +define java_jar_template
> +$(1): $(2) $(3)
> +	@echo "JAR $(1)"

Use $(REPORT_BUILD) instead.

@@ +107,5 @@
> +    $(4)\
> +		-d $(1:.jar=)-classes\
> +		$(if $(strip $(3)),-classpath $(subst $(NULL) ,:,$(strip $(3))))\
> +		$$(filter %.java,$$^)
> +	$$(NSINSTALL) -D $$(@D)

We may not want to run $(NSINSTALL) -D . when $(1) doesn't contain a path.

::: js/src/config/rules.mk
@@ +1217,5 @@
>  
>  ###############################################################################
>  # Java rules
>  ###############################################################################
> +include $(topsrcdir)/config/makefiles/java-build.mk

Please add more things to the ifneq instead of removing it.

::: mobile/android/base/Makefile.in
@@ +288,4 @@
>    WebRTCAudioDevice.java \
>    $(NULL)
>  
> +WEBRTC_JAVA_FILES := \

Those are unrelated. Can you leave them alone in this patch?

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Add JAVA_JAR_TARGETS. r=glandium

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Add JAVA_JAR_TARGETS. r=glandium

This version adds target_JAVAFILES and target_PP_JAVAFILES.  The two
are functionally equivalent, but later moz.build work (in progress)
needs to know which inputs were preprocessed to create IDE projects,
etc.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 814230 [details] [diff] [review]
Add JAVA_JAR_TARGETS. r=glandium

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

::: config/makefiles/java-build.mk
@@ +98,5 @@
> +#        $(srcdir), but we use VPATH and $^ so sources can be relative
> +#        to $(srcdir) or $(CURDIR).
> +# Arg 3: Preprocessed Java sources list.  These sources are usually
> +#        relative to $(CURDIR), but we use VPATH and $^ so sources can
> +#        be relative to $(srcdir) or $(CURDIR).

I see how that can be useful to have those variables, but you don't actually need to separate them in the arguments to this macro.

@@ +106,5 @@
> +define java_jar_template
> +$(1): $(2) $(3) $(4)
> +	$$(REPORT_BUILD)
> +	@$$(NSINSTALL) -D $(1:.jar=)-classes
> +	@$$(ifneq ./,$$(@D),$$(NSINSTALL) -D $$(@D))

$(ifneq) doesn't exist.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Add JAVA_JAR_TARGETS. r=glandium

Good catch on the ifneq -- does the replacement $(if $(filter-out look
good?  Only other change is to roll back the new template argument but
keep the target_PP_SOURCES.

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Add JAVA_JAR_TARGETS. r=glandium

Huh, lost an old review comment?  Not sure how that happened.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 815136 [details] [diff] [review]
Add JAVA_JAR_TARGETS. r=glandium

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

r+, provided you fixup the filter-out.

::: config/makefiles/java-build.mk
@@ +102,5 @@
> +define java_jar_template
> +$(1): $(2) $(3)
> +	$$(REPORT_BUILD)
> +	@$$(NSINSTALL) -D $(1:.jar=)-classes
> +	@$$(if $$(filter-out ./,$$(@D)),$$(NSINSTALL) -D $$(@D))

AFAICT, both make and pymake return "." when $(1) has no path, not "./"

Comment 15

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/a29a192dbfe8
https://hg.mozilla.org/integration/fx-team/rev/61b6e63fe7f9
https://hg.mozilla.org/integration/fx-team/rev/905feca3578b
https://hg.mozilla.org/integration/fx-team/rev/5055f99ce7c9

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a29a192dbfe8
https://hg.mozilla.org/mozilla-central/rev/61b6e63fe7f9
https://hg.mozilla.org/mozilla-central/rev/905feca3578b
https://hg.mozilla.org/mozilla-central/rev/5055f99ce7c9