Standardize Java JAR building into rules.mk

RESOLVED FIXED in mozilla27

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

unspecified
mozilla27
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 813348 [details] [diff] [review]
Add JAVA_JAR_TARGETS. r=glandium
Attachment #813348 - Flags: review?(mh+mozilla)
(Assignee)

Comment 2

4 years ago
Created attachment 813349 [details] [diff] [review]
Make annotationProcessors build with -Xlint:all. r=glandium
Attachment #813349 - Flags: review?(mh+mozilla)
Attachment #813349 - Flags: feedback?(chriskitching)
(Assignee)

Comment 3

4 years ago
Created attachment 813350 [details] [diff] [review]
Build annotationProcessors with JAVA_JAR_TARGETS. r=glandium
Attachment #813350 - Flags: review?(mh+mozilla)
Attachment #813350 - Flags: feedback?(chriskitching)
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.
Attachment #813349 - Flags: feedback?(chriskitching) → feedback+
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.
Attachment #813350 - Flags: feedback?(chriskitching) → feedback+
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.
Attachment #813349 - Flags: review?(mh+mozilla)
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?
Attachment #813348 - Flags: review?(mh+mozilla) → feedback+
Attachment #813350 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 9

4 years ago
Created attachment 814162 [details] [diff] [review]
Add JAVA_JAR_TARGETS. r=glandium
Attachment #814162 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #813348 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 814230 [details] [diff] [review]
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.
Attachment #814162 - Attachment is obsolete: true
Attachment #814162 - Flags: review?(mh+mozilla)
Attachment #814230 - Flags: review?(mh+mozilla)
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.
Attachment #814230 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 12

4 years ago
Created attachment 815104 [details] [diff] [review]
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.
Attachment #814230 - Attachment is obsolete: true
Attachment #815104 - Flags: review?(mh+mozilla)
(Assignee)

Comment 13

4 years ago
Created attachment 815136 [details] [diff] [review]
Add JAVA_JAR_TARGETS. r=glandium

Huh, lost an old review comment?  Not sure how that happened.
Attachment #815104 - Attachment is obsolete: true
Attachment #815104 - Flags: review?(mh+mozilla)
Attachment #815136 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Blocks: 900522
(Assignee)

Updated

4 years ago
No longer blocks: 900522
Blocks: 924738
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 "./"
Attachment #815136 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

4 years ago
Blocks: 925185
You need to log in before you can comment on or make changes to this bug.