Closed Bug 751156 Opened 13 years ago Closed 12 years ago

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

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: joey, Assigned: joey)

Details

Attachments

(1 file, 5 obsolete files)

robocop dependency build should be a nop
12.31 KB, patch
gps
: review+
Details | Diff | Splinter Review
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: nobody → joey
Attached patch dependency build should be a nop (obsolete) — — Splinter Review
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)
(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?
Attachment #691027 - Flags: feedback?(mshal)
(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.
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)
Attachment #691027 - Attachment is obsolete: true
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+
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+
(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.
(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.
(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}
(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?)
Attachment #693379 - Attachment is obsolete: true
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.
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
Attachment #697433 - Attachment is obsolete: true
Attachment #718740 - Attachment is obsolete: true
Attachment #723573 - Attachment is obsolete: true
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+
Pushed to inbound: o committed changeset 125434:0f012ef3d3ac https://hg.mozilla.org/integration/mozilla-inbound/rev/0f012ef3d3ac
secondary push to fix file copy into js/src/config/rules.mk https://hg.mozilla.org/integration/mozilla-inbound/rev/548ff2b01536
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Attachment #723932 - Flags: feedback?(mshal)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: