makefiles: mobile/android/robocop - dependency build is not a NOP

RESOLVED FIXED in mozilla22

Status

RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: joey, Assigned: joey)

Tracking

unspecified
mozilla22

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
Review deps for android/robocop/Makefile.in.  A dependency build beneath robocop/ will run nsinstall and compile files on subsequent traversal requests.


mozmake[6]: Leaving directory `/local/mozilla/bugs/748470/objdir-droid/build/mobile/sutagent/android/fencp', start=1335967083, end=1335967083, elapsed=0 sec, dir[n]=0
/usr/local/bin/mozmake -j1 -C mobile/robocop tools
mozmake[6]: Entering directory `/local/mozilla/bugs/748470/objdir-droid/build/mobile/robocop', tod=1335967083
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/MotionEventReplayer.java.in > ../../../mobile/android/base/tests/MotionEventReplayer.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/MotionEventHelper.java.in > ../../../mobile/android/base/tests/MotionEventHelper.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testBookmark.java.in > ../../../mobile/android/base/tests/testBookmark.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testAxisLocking.java.in > ../../../mobile/android/base/tests/testAxisLocking.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testWebContentContextMenu.java.in > ../../../mobile/android/base/tests/testWebContentContextMenu.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testBrowserProviderPerf.java.in > ../../../mobile/android/base/tests/testBrowserProviderPerf.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testBookmarklets.java.in > ../../../mobile/android/base/tests/testBookmarklets.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testFormHistory.java.in > ../../../mobile/android/base/tests/testFormHistory.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testBrowserProvider.java.in > ../../../mobile/android/base/tests/testBrowserProvider.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testPasswordProvider.java.in > ../../../mobile/android/base/tests/testPasswordProvider.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testPanCorrectness.java.in > ../../../mobile/android/base/tests/testPanCorrectness.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testPan.java.in > ../../../mobile/android/base/tests/testPan.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testAboutPage.java.in > ../../../mobile/android/base/tests/testAboutPage.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testJarReader.java.in > ../../../mobile/android/base/tests/testJarReader.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testPasswordEncrypt.java.in > ../../../mobile/android/base/tests/testPasswordEncrypt.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testCheck.java.in > ../../../mobile/android/base/tests/testCheck.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/ContentProviderTest.java.in > ../../../mobile/android/base/tests/ContentProviderTest.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testCheck2.java.in > ../../../mobile/android/base/tests/testCheck2.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/test_bug720538.java.in > ../../../mobile/android/base/tests/test_bug720538.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testLoad.java.in > ../../../mobile/android/base/tests/testLoad.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testAwesomebar.java.in > ../../../mobile/android/base/tests/testAwesomebar.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testNewTab.java.in > ../../../mobile/android/base/tests/testNewTab.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testFlingCorrectness.java.in > ../../../mobile/android/base/tests/testFlingCorrectness.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/testOverscroll.java.in > ../../../mobile/android/base/tests/testOverscroll.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/PixelTest.java.in > ../../../mobile/android/base/tests/PixelTest.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D ../../../mobile/android/base/tests
/usr/bin/python2.7 /local/mozilla/bugs/748470/config/Preprocessor.py -DANDROID_PACKAGE_NAME=org.mozilla.fennec_joey  /local/mozilla/bugs/748470/mobile/android/base/tests/BaseTest.java.in > ../../../mobile/android/base/tests/BaseTest.java
/local/mozilla/bugs/748470/objdir-droid/config/nsinstall -D classes
"/usr/local/bin/javac" -target 1.5 -source 1.5 -classpath /usr/local/src/android-sdk-linux/platforms/android-13/android.jar:/local/mozilla/bugs/748470/build/mobile/robocop/robotium-solo-3.1.jar -bootclasspath /usr/local/src/android-sdk-linux/platforms/android-13/android.jar -encoding UTF8 -g:source,lines -Werror  -d classes R.java  Actions.java Assert.java Driver.java Element.java FennecNativeActions.java FennecMochitestAssert.java FennecTalosAssert.java FennecNativeDriver.java FennecNativeElement.java RoboCopException.java PaintedSurface.java  ../../../mobile/android/base/tests/MotionEventReplayer.java ../../../mobile/android/base/tests/MotionEventHelper.java ../../../mobile/android/base/tests/testBookmark.java ../../../mobile/android/base/tests/testAxisLocking.java ../../../mobile/android/base/tests/testWebContentContextMenu.java ../../../mobile/android/base/tests/testBrowserProviderPerf.java ../../../mobile/android/base/tests/testBookmarklets.java ../../../mobile/android/base/tests/testFormHistory.java ../../../mobile/android/base/tests/testBrowserProvider.java ../../../mobile/android/base/tests/testPasswordProvider.java ../../../mobile/android/base/tests/testPanCorrectness.java ../../../mobile/android/base/tests/testPan.java ../../../mobile/android/base/tests/testAboutPage.java ../../../mobile/android/base/tests/testJarReader.java ../../../mobile/android/base/tests/testPasswordEncrypt.java ../../../mobile/android/base/tests/testCheck.java ../../../mobile/android/base/tests/ContentProviderTest.java ../../../mobile/android/base/tests/testCheck2.java ../../../mobile/android/base/tests/test_bug720538.java ../../../mobile/android/base/tests/testLoad.java ../../../mobile/android/base/tests/testAwesomebar.java ../../../mobile/android/base/tests/testNewTab.java ../../../mobile/android/base/tests/testFlingCorrectness.java ../../../mobile/android/base/tests/testOverscroll.java ../../../mobile/android/base/tests/PixelTest.java ../../../mobile/android/base/tests/BaseTest.java
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
/usr/local/src/android-sdk-linux/platforms/android-13/../../platform-tools/dx --dex --output=classes.dex classes /local/mozilla/bugs/748470/build/mobile/robocop/robotium-solo-3.1.jar
cp /local/mozilla/bugs/748470/mobile/android/base/tests/robocop.ini robocop.ini
cp /local/mozilla/bugs/748470/build/mobile/robocop/parse_ids.py parse_ids.py
mozmake[6]: Leaving directory `/local/mozilla/bugs/748470/objdir-droid/build/mobile/robocop', start=1335967083, end=1335967089, elapsed=6 sec, dir[n]=0
(Assignee)

Updated

7 years ago
Assignee: nobody → joey
(Assignee)

Comment 1

6 years ago
Created attachment 691027 [details] [diff] [review]
dependency build should be a nop
(Assignee)

Comment 2

6 years ago
Comment on attachment 691027 [details] [diff] [review]
dependency build should be a nop

Replaced $(_JAVA_TESTS) : pattern : target with dependencies so sources will only be processed when files change.

Replaced nsinstall -D calls with mkdir_deps in a few places so directories will only need to be crated once.

Declared a few local variables to shorten file path and remove hardcoded values.

export:: target - use rsync when available to copy files.  Fallback to using tar as the android build was not able to locate the rsync binary.
Attachment #691027 - Flags: feedback?(mshal)
Comment on attachment 691027 [details] [diff] [review]
dependency build should be a nop

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

A few drive-by comments.

::: build/mobile/robocop/Makefile.in
@@ +81,5 @@
>  AndroidManifest.xml: % : %.in
>  	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $< > $@
>  
> +$(dir-tests)/%.java : $(TESTPATH)/%.java.in $(call mkdir_deps,$(dir-tests))
> +	$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $< > $@

Is it possible to rewrite these using PP_TARGETS (not totally sure since we're using a static pattern rule here)
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1559

@@ +87,5 @@
>  $(_ROBOCOP_TOOLS):
>  	cp $(TESTPATH)/robocop.ini robocop.ini
>  
> +libs:: $(call mkdir_deps,$(dir-mochitest))
> +	$(INSTALL) $(foreach f,$(_TEST_FILES),"$f") $(dir-mochitest)/

We have a special MOCHITEST_FILES variable that handles installing normal mochitests:
http://mxr.mozilla.org/mozilla-central/source/config/makefiles/mochitest.mk

Perhaps we should make an analogous variable for robocop tests?

@@ +108,5 @@
>  	cp $(srcdir)/parse_ids.py parse_ids.py
>  
> +export:: $(call mkdir_deps,res)
> +	rsync --checksum -a $(srcdir)/res/. res \
> +	  || (cd $(srcdir)/res && tar $(TAR_CREATE_FLAGS) - *) | (cd $(DEPTH)/build/mobile/robocop/res && tar -xf -)

We normally avoid using rsync because we don't have it available on Windows, but maybe it's ok for mobile builds because we don't fully support building them on Windows. I think we should probably add a generic directory-copying rule to the build system, we use logic like this all over the place, and it'd be nicer to be able to say like:
$(COPY_DIR) $(src) $(dest)
(Assignee)

Comment 5

6 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> Comment on attachment 691027 [details] [diff] [review]
> dependency build should be a nop
> 
> Review of attachment 691027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few drive-by comments.

Thanks I'll check on the library rules.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> @@ +108,5 @@
> >  	cp $(srcdir)/parse_ids.py parse_ids.py
> >  
> > +export:: $(call mkdir_deps,res)
> > +	rsync --checksum -a $(srcdir)/res/. res \
> > +	  || (cd $(srcdir)/res && tar $(TAR_CREATE_FLAGS) - *) | (cd $(DEPTH)/build/mobile/robocop/res && tar -xf -)
> 
> We normally avoid using rsync because we don't have it available on Windows,
> but maybe it's ok for mobile builds because we don't fully support building
> them on Windows. I think we should probably add a generic directory-copying
> rule to the build system, we use logic like this all over the place, and
> it'd be nicer to be able to say like:
> $(COPY_DIR) $(src) $(dest)

It doesn't look like rsync is supported on the try server for Android at least (not sure how that's configured):

rsync --checksum -a /builds/slave/try-andrd/build/build/mobile/robocop/res/. res \
	  || (cd /builds/slave/try-andrd/build/build/mobile/robocop/res && tar -cvhf - *) | (cd ../../../build/mobile/robocop/res && tar -xf -)
/bin/sh: rsync: command not found

Is there somewhere where rsync is beneficial here?

Also, what is the point of exporting res/values/strings.xml in the first place? Seems the consumer of that file could just pull it from srcdir, rather than export it and use it in objdir?
(Assignee)

Updated

6 years ago
Attachment #691027 - Flags: feedback?(mshal)
(Assignee)

Comment 7

6 years ago
(In reply to Michael Shal [:mshal] from comment #6)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> > @@ +108,5 @@
> > >  	cp $(srcdir)/parse_ids.py parse_ids.py
> >
> > it'd be nicer to be able to say like:
> > $(COPY_DIR) $(src) $(dest)
> 
> It doesn't look like rsync is supported on the try server for Android at
> least (not sure how that's configured):
> 
> rsync --checksum -a /builds/slave/try-andrd/build/build/mobile/robocop/res/.
> res \
> 	  || (cd /builds/slave/try-andrd/build/build/mobile/robocop/res && tar
> -cvhf - *) | (cd ../../../build/mobile/robocop/res && tar -xf -)
> /bin/sh: rsync: command not found
> 
> Is there somewhere where rsync is beneficial here?

For the general case rsync avoids having to stream the entire contents of a directory through a tar pipeline.  Files are only copied when modified and large files can be copied within one process space.


> Also, what is the point of exporting res/values/strings.xml in the first
> place? Seems the consumer of that file could just pull it from srcdir,
> rather than export it and use it in objdir?

Not sure that logic is pre-existing.  If strings.xml is branded with any ids while building that might prevent simply using $(srcdir)/strings.xml directly.
(Assignee)

Comment 8

6 years ago
Created attachment 693379 [details] [diff] [review]
robocop dependency build should be a nop

Replaced "nsinstall -D" calls with mkdir_deps so creation calls will only be made when needed.
Replaced Preprocessor targets with _PP library macros.
Use the PP _TARGET macro to install android manifest.
libs:: target handling deferred to mochitest.mk.

Construct dependency paths for parse_ids.py and robocop.ini relative to $(CURDIR).
vpath search w/DEPTH=../.. is able to locate these files in $(srcdir) and handle as a nop.
Likely why robocp.apk was explicitly installing them.
Installing these files as dependencies will remove a FORCE attribute causing a perpetually stale classes.dex file.

Replaced inlined tar pipeline with make library macro copy_dir().
Attachment #693379 - Flags: review?(ted)
(Assignee)

Updated

6 years ago
Attachment #691027 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #693379 - Flags: feedback?(mshal)
Looks like all other Makefiles just use cp directly instead of defining something like COPY. Should this do the same for consistency? Maybe cp->COPY can be done globally as another bug if that's worth doing.
Attachment #693379 - Flags: feedback?(mshal) → feedback+
(Assignee)

Comment 12

6 years ago
Code review ping.
Comment on attachment 693379 [details] [diff] [review]
robocop dependency build should be a nop

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

Overall this looks about as good as a Java-using Makefile in our tree can look. :) I have a few nits, and one thing I'd really like to see changed before landing this (the parse_ids.py and robocop.ini change), but I wouldn't block you on landing if you can't or don't have time for that.

::: build/mobile/robocop/Makefile.in
@@ +31,5 @@
>    RoboCopException.java \
>    PaintedSurface.java \
>    $(NULL)
>  
> +dir-obj   := $(DEPTH)/build/mobile/robocop

Isn't this just $(CURDIR)?

@@ +33,5 @@
>    $(NULL)
>  
> +dir-obj   := $(DEPTH)/build/mobile/robocop
> +dir-res   := $(dir-obj)/res
> +dir-tests := $(subst $(topsrcdir),$(DEPTH),$(TESTPATH))

I always hate seeing $(subst) in pathnames, it just feels really fragile to me. Maybe factor out the relative pathname from TESTPATH into a separate variable and use it in both places instead?

@@ +38,5 @@
>  
> +# preprocessing deps
> +java-harness-src  := $(addprefix $(srcdir)/,$(addsuffix .in,$(_JAVA_HARNESS)))
> +java-harness-dep  := $(addprefix $(dir-obj)/,$(_JAVA_HARNESS))
> +java-harness      := $(java-harness-src)

Is there any reason to use two separate variables for java-harness and java-harness-src when they have the same value? (I realize java-harness is for PP_TARGETS, but feels extraneous to have both.)

@@ +50,5 @@
> +PP_TARGETS      += java-tests
> +
> +manifest          := $(srcdir)/AndroidManifest.xml.in
> +manifest_TARGET   := AndroidManifest.xml
> +PP_TARGETS        += manifest

nit: I think that putting the PP_TARGETS line before the other two lines would make this more readable (it's clearer to me what the purpose of those two lines is when I've seen the PP_TARGETS line first).

@@ +65,5 @@
> +#    No need to remake target `$(DEPTH)/build/mobile/robocop/parse_ids.py'
> +#    using VPATH name `$(topsrcdir)/build/mobile/robocop/$(DEPTH)/build/mobile/robocop/parse_ids.py'.
> +
> +parse-ids-py := $(CURDIR)/parse_ids.py
> +robocop-ini  := $(CURDIR)/robocop.ini

I looked around, and I can't find any reason that these files are being copied from the srcdir to the objdir. Can you test that things work okay if you just stop copying them (and adjust the few references to them to use them from the srcdir)? Looks like it's only a few places:
http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#388
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#339
http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#62

@@ +79,1 @@
>    _JAVA_HARNESS \

This doesn't make any sense to me. I think the original Makefile is wrong. These are variable names, not filenames to be cleaned up, right?

@@ +114,3 @@
>  
> +export:: $(call mkdir_deps,$(dir-res))
> +	$(call copy_dir,$(srcdir)/res,$(dir-res))

Do we use this pattern elsewhere? If so might be ripe for a followup to add a magic variable like INSTALL_TARGETS for copying dirs.

::: js/src/config/makefiles/makeutils.mk
@@ +114,5 @@
>  endif
> +
> +## copy(src, dst): recursive copy
> +## rsync could selectively copy but cmd not available on windows
> +copy_dir = (cd $(1)/. && $(TAR) $(TAR_CREATE_FLAGS_QUIET) - .) | (cd $(2)/. && tar -xf -)

This is nice! At least we hide the gory implementation details. :) I think eventually I'd like to stop using tar here, but it works.

::: js/src/config/makefiles/mochitest.mk
@@ +50,5 @@
>  ifdef MOCHITEST_A11Y_FILES
>  $(eval $(call mochitest-libs-rule-template,MOCHITEST_A11Y_FILES,a11y))
>  endif
>  
> +ifdef MOCHITEST_ROBOCOP_FILES

Thanks for adding this!
Attachment #693379 - Flags: review?(ted) → review+
(Assignee)

Comment 14

6 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> Comment on attachment 693379 [details] [diff] [review]
> robocop dependency build should be a nop
> 
> Review of attachment 693379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this looks about as good as a Java-using Makefile in our tree can
> look. :) I have a few nits, and one thing I'd really like to see changed
> before landing this (the parse_ids.py and robocop.ini change), but I
> wouldn't block you on landing if you can't or don't have time for that.
> 
> ::: build/mobile/robocop/Makefile.in
> @@ +31,5 @@
> >    RoboCopException.java \
> >    PaintedSurface.java \
> >    $(NULL)
> >  
> > +dir-obj   := $(DEPTH)/build/mobile/robocop
> 
> Isn't this just $(CURDIR)?
> 
> @@ +33,5 @@
> >    $(NULL)
> >  
> > +dir-obj   := $(DEPTH)/build/mobile/robocop
> > +dir-res   := $(dir-obj)/res
> > +dir-tests := $(subst $(topsrcdir),$(DEPTH),$(TESTPATH))
> 
> I always hate seeing $(subst) in pathnames, it just feels really fragile to
> me. Maybe factor out the relative pathname from TESTPATH into a separate
> variable and use it in both places instead?
> 
> @@ +38,5 @@
> >  
> > +# preprocessing deps
> > +java-harness-src  := $(addprefix $(srcdir)/,$(addsuffix .in,$(_JAVA_HARNESS)))
> > +java-harness-dep  := $(addprefix $(dir-obj)/,$(_JAVA_HARNESS))
> > +java-harness      := $(java-harness-src)
> 
> Is there any reason to use two separate variables for java-harness and
> java-harness-src when they have the same value? (I realize java-harness is
> for PP_TARGETS, but feels extraneous to have both.)
> 
> @@ +50,5 @@
> > +PP_TARGETS      += java-tests
> > +
> > +manifest          := $(srcdir)/AndroidManifest.xml.in
> > +manifest_TARGET   := AndroidManifest.xml
> > +PP_TARGETS        += manifest
> 
> nit: I think that putting the PP_TARGETS line before the other two lines
> would make this more readable (it's clearer to me what the purpose of those
> two lines is when I've seen the PP_TARGETS line first).
> 
> @@ +65,5 @@
> > +#    No need to remake target `$(DEPTH)/build/mobile/robocop/parse_ids.py'
> > +#    using VPATH name `$(topsrcdir)/build/mobile/robocop/$(DEPTH)/build/mobile/robocop/parse_ids.py'.
> > +
> > +parse-ids-py := $(CURDIR)/parse_ids.py
> > +robocop-ini  := $(CURDIR)/robocop.ini
> 
> I looked around, and I can't find any reason that these files are being
> copied from the srcdir to the objdir. Can you test that things work okay if
> you just stop copying them (and adjust the few references to them to use
> them from the srcdir)? Looks like it's only a few places:
> http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.
> mk#388
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> packager.mk#339
> http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#62
> 
> @@ +79,1 @@
> >    _JAVA_HARNESS \
> 
> This doesn't make any sense to me. I think the original Makefile is wrong.
> These are variable names, not filenames to be cleaned up, right?

I had not checked the actual usage but varnames were used often enough assumed the library clean target was setup to handle expansion.  If it is not that would be easy enough to support with $(foreach) $(or) & $(value).  This and the rest to be fixed.
(Assignee)

Comment 15

6 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> Comment on attachment 693379 [details] [diff] [review]
> robocop dependency build should be a nop
> 
> Review of attachment 693379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this looks about as good as a Java-using Makefile in our tree can
> look. :) 

So are you saying why bother ?

> I have a few nits, and one thing I'd really like to see changed before landing this (the parse_ids.py and robocop.ini change), but I
> wouldn't block you on landing if you can't or don't have time for that.

I'll remove these deps with a followup patch.  Unrelated tests should be triggered to verify removing from dist and using them from src will not cause any breakage.
(Assignee)

Comment 16

6 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> Comment on attachment 693379 [details] [diff] [review]
> robocop dependency build should be a nop
> 
> Review of attachment 693379 [details] [diff] [review]:
> -----------------------------------------------------------------
> > +dir-obj   := $(DEPTH)/build/mobile/robocop
> > +dir-res   := $(dir-obj)/res
> > +dir-tests := $(subst $(topsrcdir),$(DEPTH),$(TESTPATH))
> 
> I always hate seeing $(subst) in pathnames, it just feels really fragile to
> me. Maybe factor out the relative pathname from TESTPATH into a separate
> variable and use it in both places instead?

Subst usage here will create a derived path, topsrcdir= is absolute and reasonably unique so there should not be an issue with this type of path construction.

I'm not a big fan of hardcoding but refactoring into an extra var would avoid having to perform the string transform.  Hmmm, actually the hierarchy could probably benefit from a little more isolation.  If the robocop makfile no longer had to reach outside of it's area for deps, path construction and hardcoding disappear and the logic can be somewhat simplified.


> @@ +38,5 @@
> >  
> > +# preprocessing deps
> > +java-harness-src  := $(addprefix $(srcdir)/,$(addsuffix .in,$(_JAVA_HARNESS)))
> > +java-harness-dep  := $(addprefix $(dir-obj)/,$(_JAVA_HARNESS))
> > +java-harness      := $(java-harness-src)
> 
> Is there any reason to use two separate variables for java-harness and
> java-harness-src when they have the same value? (I realize java-harness is
> for PP_TARGETS, but feels extraneous to have both.)

This was simply for naming consistency with the 2nd variable set java-tests{,-src,-dep}
(Assignee)

Comment 17

6 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> Comment on attachment 693379 [details] [diff] [review]
> robocop dependency build should be a nop
> 
> Review of attachment 693379 [details] [diff] [review]:
> -----------------------------------------------------------------
> > ::: js/src/config/makefiles/makeutils.mk
> @@ +114,5 @@
> >  endif
> > +
> > +## copy(src, dst): recursive copy
> > +## rsync could selectively copy but cmd not available on windows
> > +copy_dir = (cd $(1)/. && $(TAR) $(TAR_CREATE_FLAGS_QUIET) - .) | (cd $(2)/. && tar -xf -)
> 
> This is nice! At least we hide the gory implementation details. :) I think
> eventually I'd like to stop using tar here, but it works.

I think copy_dir function may have been your idea in an earlier review.  The initial patch was written to use rsync rather thank tar for copying what was needed rather than spooling an entire directory but the binary is not available everywhere.

As a library function this will be prone to breakage if pre-reqs are not added to explicitly create $(2).
(In reply to Joey Armstrong [:joey] from comment #15)
> So are you saying why bother ?

Hah! No, this is good dependency-sanitizing, which is a noble thing to fix. All the Java building in our tree just feels really shoehorned in. It'd be nice to review it in the future and see if we could push the complexity back into rules.mk and hide it behind some magic variables. (Would help with an eventual switch to tup, as well.)

(In reply to Joey Armstrong [:joey] from comment #17)
> As a library function this will be prone to breakage if pre-reqs are not
> added to explicitly create $(2).

This is a good point. There's probably room for some directory-copying magic similar to INSTALL_TARGETS that copies directories recursively and ensures that the destination exists. (Perhaps we could extend INSTALL_TARGETS to support recursive copying?)
(Assignee)

Comment 19

6 years ago
Created attachment 697433 [details] [diff] [review]
robocop dependency build should be a nop
(Assignee)

Updated

6 years ago
Attachment #693379 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Comment on attachment 697433 [details] [diff] [review]
robocop dependency build should be a nop

Change path derivation to construction with a common relative path.
Changed build/mobile/robocop use to $(CURDIR).
Assign local vars for use as target deps.
Use robocop.ini an parse_ids.py from the source directory rather than introducing deps, copying and using from dist/.
Test targets updated to be processed by config/makefiles/mochitest.mk library rules.  MOCHITEST_ROBOCOP_FILES will be the trigger mechanism.
mkdir_deps dependencies added for directory targets to avoid calling $(NSINSTALL) -D every time.
Clean target changed to use expanded macros rather than macro name.  Library clean target could be changed to hand this expansion automatically.
(Assignee)

Comment 21

6 years ago
Pushed to inbound: mozilla-inbound - changeset - 117446:119c60543071 

https://tbpl.mozilla.org/?tree=Try&rev=c60f6fd0b0a0
 - mochitests ran with altered path to robocop.ini and parse_ids.py
(Assignee)

Comment 23

6 years ago
Created attachment 718740 [details] [diff] [review]
robocop dependency build should be a nop
(Assignee)

Updated

6 years ago
Attachment #697433 - Attachment is obsolete: true
(Assignee)

Comment 24

6 years ago
Created attachment 723573 [details] [diff] [review]
robocop dependency build should be a nop
(Assignee)

Updated

6 years ago
Attachment #718740 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
Created attachment 723932 [details] [diff] [review]
robocop dependency build should be a nop
(Assignee)

Updated

6 years ago
Attachment #723573 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Comment on attachment 723932 [details] [diff] [review]
robocop dependency build should be a nop

try job succeded: https://tbpl.mozilla.org/?tree=Try&rev=96e2f108f511

Interactive intermittent failures are known:
log.robocop.testBookmark Failed: 1
log.robocop.testHistoryTab Failed: 2
log.robocop.testSearchSuggestions Failed: 2
log.robocop.testWebContentContextMenu Failed: 1

Patch edits:
  o added mochitest.mk::MOCHITEST_ROBOCOP_FILES= for library copy of test files.
  o extra option added {$3} in mochitest_path, robocop currently requires destination to have a flat directory structure.
  o refactored redundant paths in the makefile.
  o use $@, $< to shorten paths in target rules.
  o Use PP_* & INSTALL_* library rules to install *.java sources and robocop config files.
  o classes.dex pre-reqs altered to allow recompiling only when sources/deps change.
Attachment #723932 - Flags: review?(mshal)
Attachment #723932 - Flags: review?(gps)
Comment on attachment 723932 [details] [diff] [review]
robocop dependency build should be a nop

Mike Shal is not a build peer, so he can only grant f+, not r+.
Attachment #723932 - Flags: review?(mshal) → feedback?(mshal)
Comment on attachment 723932 [details] [diff] [review]
robocop dependency build should be a nop

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

This looks good to me. I'm glad this Makefile.in finally resembles sanity!
Attachment #723932 - Flags: review?(gps) → review+
(Assignee)

Comment 29

6 years ago
Pushed to inbound:
  o committed changeset 125434:0f012ef3d3ac

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f012ef3d3ac
(Assignee)

Comment 30

6 years ago
secondary push to fix file copy into js/src/config/rules.mk

https://hg.mozilla.org/integration/mozilla-inbound/rev/548ff2b01536
(Assignee)

Comment 31

6 years ago
Let's try this again with patch.751156 instead of patch.xpt

% grep ^bug ~/Downloads/patch.751156 
bug 751156: robocop dependency build should be a nop

committed changeset 125473:e9e5e2a8a52b

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9e5e2a8a52b
https://hg.mozilla.org/mozilla-central/rev/e9e5e2a8a52b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Assignee)

Updated

6 years ago
Attachment #723932 - Flags: feedback?(mshal)

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.