We need to implement make files to incorporate SDK modules from mozilla-central's **addon-sdk** folder into a firefox builds. See: Bug 793916

Comment on attachment 698180 [details] [diff] [review] Adds makefiles to include the SDK APIs located in addon-sdk/source in built firefox and the tests package We're not at review stage quite yet I think but I'd like to make sure I'm in the right direction here. Assume that the tree contains a copy of the git repo https://github.com/mozilla/addon-sdk at addon-sdk/source, these makefile changes should copy the API modules so they appear at resource://app/modules/commonjs and copy the test harness and test files into the built test package. It also makes "make jetpack-tests" run the test suite.

Comment on attachment 698180 [details] [diff] [review] Adds makefiles to include the SDK APIs located in addon-sdk/source in built firefox and the tests package Review of attachment 698180 [details] [diff] [review]: ----------------------------------------------------------------- ::: .hgignore @@ +52,5 @@ > ^python/psutil/.*\.pyd > ^python/psutil/build/ > + > +# Add-on SDK git files > +^addon-sdk/source/.git/ Perhaps ".git/" would be a more appropriate pattern? ::: addon-sdk/Makefile.in @@ +18,5 @@ > +libs:: > + $(PYTHON) $(topsrcdir)/config/nsinstall.py \ > + $(srcdir)/source/lib/toolkit/* $(COMMONJS_PATH) > + $(PYTHON) $(topsrcdir)/config/nsinstall.py \ > + $(srcdir)/source/lib/sdk/* $(COMMONJS_PATH) General rule: if you type "nsinstall.py", $(INSTALL) or any of the variants in a Makefile.in, you are doing it wrong. The preferred way to install files is to use the generic INSTALL_TARGETS variable. See /services/sync/Makefile.in for an example starting around line 83. @@ +38,5 @@ > + > +PKG_STAGE = $(DIST)/test-package-stage > + > +stage-tests-package:: $(TEST_FILES) > + $(INSTALL) $^ $(PKG_STAGE)/jetpack If only it were this simple. Unless things have changed in the last few months, there are pieces outside of the tree (buildbotconfig-custom) that contain patterns of which files to extract from test archives for purposes of running tests on buildbot test machines. You'll need to add the jetpack directory to these as well. Of course, this all depends on how these tests will run on buildbot. I have no clue what your intentions are. You should work that out before you commit the packaging changes, IMO. @@ +40,5 @@ > + > +stage-tests-package:: $(TEST_FILES) > + $(INSTALL) $^ $(PKG_STAGE)/jetpack > + > +endif I don't believe any of the above will actually do anything by itself so I'm not sure you need to surround in the ifdef. ::: browser/build.mk @@ +7,5 @@ > endif > > TIERS += app > > +tier_app_dirs += addon-sdk Any reason why this is app and not platform? ::: testing/testsuite-targets.mk @@ +423,5 @@ > $(NSINSTALL) $(DEPTH)/build/mobile/sutagent/android/watcher/Watcher.apk $(PKG_STAGE)/bin > $(NSINSTALL) $(DEPTH)/build/mobile/sutagent/android/fencp/FenCP.apk $(PKG_STAGE)/bin > $(NSINSTALL) $(DEPTH)/build/mobile/sutagent/android/ffxcp/FfxCP.apk $(PKG_STAGE)/bin > > +stage-jetpack:: make-stage-dir I would prefer this be a mach command and we omit the make target. This may sound weird, but the make targets for tests aren't actually required by anything besides local development (buildbot does its own thing). I know some will disagree with me. My opinion is that mach is a much richer interface for driving developer tasks than make targets/files and thus we shouldn't bother with supporting make unless we need to preserve muscle memory. I can help you author a mach command if https://developer.mozilla.org/en-US/docs/Developer_Guide/mach and the various mach_commands.py files scattered in the tree aren't enough.

(In reply to Gregory Szorc [:gps] from comment #5) > Comment on attachment 698180 [details] [diff] [review] > Adds makefiles to include the SDK APIs located in addon-sdk/source in built > firefox and the tests package > > Review of attachment 698180 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: .hgignore > @@ +52,5 @@ > > ^python/psutil/.*\.pyd > > ^python/psutil/build/ > > + > > +# Add-on SDK git files > > +^addon-sdk/source/.git/ > > Perhaps ".git/" would be a more appropriate pattern? If that's acceptable then happy to, just wanted to avoid forcing it on the whole tree. > @@ +38,5 @@ > > + > > +PKG_STAGE = $(DIST)/test-package-stage > > + > > +stage-tests-package:: $(TEST_FILES) > > + $(INSTALL) $^ $(PKG_STAGE)/jetpack > > If only it were this simple. Unless things have changed in the last few > months, there are pieces outside of the tree (buildbotconfig-custom) that > contain patterns of which files to extract from test archives for purposes > of running tests on buildbot test machines. You'll need to add the jetpack > directory to these as well. > > Of course, this all depends on how these tests will run on buildbot. I have > no clue what your intentions are. You should work that out before you commit > the packaging changes, IMO. As luck would have it buildbot already extracts the jetpack directory (it currently only contains one file) for the existing jetpack tests so I suspect we're good. But I'm working on getting the buildbot side working in bug 826933, I want to get that working on the larch branch before doing the final review here. > ::: browser/build.mk > @@ +7,5 @@ > > endif > > > > TIERS += app > > > > +tier_app_dirs += addon-sdk > > Any reason why this is app and not platform? Right now I think this isn't useful for b2g or fennec so I don't want to balloon their builds, but I expect it to become more widely shipped v.soon. > ::: testing/testsuite-targets.mk > @@ +423,5 @@ > > $(NSINSTALL) $(DEPTH)/build/mobile/sutagent/android/watcher/Watcher.apk $(PKG_STAGE)/bin > > $(NSINSTALL) $(DEPTH)/build/mobile/sutagent/android/fencp/FenCP.apk $(PKG_STAGE)/bin > > $(NSINSTALL) $(DEPTH)/build/mobile/sutagent/android/ffxcp/FfxCP.apk $(PKG_STAGE)/bin > > > > +stage-jetpack:: make-stage-dir > > I would prefer this be a mach command and we omit the make target. I'm inclined to consider doing both. One of the issues with enabled jetpack tests in the tree before has been that it is hard for devs to run them locally, so I want to make it as easy as possible and I know some stubborn developers (like me!) haven't even tried mach yet. Thanks for the quick feedback.

Update patch, includes a simple mach command (that cheats by running make jetpack-tests).

Comment on attachment 703702 [details] [diff] [review] Adds makefiles to include the SDK APIs located in addon-sdk/source in built firefox and the tests package Review of attachment 703702 [details] [diff] [review]: ----------------------------------------------------------------- ::: addon-sdk/mach_commands.py @@ +28,5 @@ > + @Command('jetpack-test', help='Runs the jetpack test suite.') > + def run_jetpack_test(self, **params): > + # We should probably have a utility function to ensure the tree is > + # ready to run tests. Until then, we just create the state dir (in > + # case the tree wasn't built with mach). Yes, yes we should. This has been on my TODO list for a while. @@ +31,5 @@ > + # ready to run tests. Until then, we just create the state dir (in > + # case the tree wasn't built with mach). > + self._ensure_state_subdir_exists('.') > + > + jetpack = self._spawn(JetpackRunner) You technically don't need JetpackRunner since MachCommandBase inherits from MozbuildObject and thus has self._run_make(). That being said, I highly encourage the high-level APIs for invoking test runners to *not* live inside a mach command method. The reason is that this would potentially constrain callers to go through mach. In theory, mach is just one of multiple drivers to the build system. Each driver would call into shared APIs for invoking test runners, building the tree, etc. This means you need to abstract things like input and output because the assumptions of terminal presence may not always be valid. But I digress. It is acceptable for JetpackRunner to be exist in the same file as the mach commands for now. I just wanted to communicate my long-term strategy of exposing core APIs for doing anything worthwhile and then hooking up mach + other drivers appropriately. i.e. "design APIs first, hook up to the terminal (mach) later."

Ok, releng aren't going to be able to get the testsuite stuff working yet so we're going to go ahead with landing this anyway. I have verified that you can run the tests from the built packages so I think we're all good. This is essentially the same patch as before only now we ship basically everywhere because lots of places depend on our promises module.

Not sure how but in bug 756542 we ended up putting the promise module in a slightly different place to where it will end up as part of the SDK modules. This patch updates all users to the new path, there is only really one add-on on AMO using this library right now so I think we're safe to do this but should it be a problem we can land a shim to still support the old path. Gavin, feel ok reviewing all of this? The only non-moves are just changing the Cu.import path.

Removed a line that shouldn't have been there.

Comment on attachment 708789 [details] [diff] [review] Updates existing users of the promise library to the new location and moves the current tests Seems fine (never liked "core.js" as the filename!). I assume you'll reach out to the relevant add-on author.

Comment on attachment 708803 [details] [diff] [review] Adds makefiles to include the SDK APIs located in addon-sdk/source in built firefox and the tests package Review of attachment 708803 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: addon-sdk/Makefile.in @@ +12,5 @@ > +TEST_DIRS += test > + > +COMMONJS_FILES = \ > + $(srcdir)/source/lib/toolkit \ > + $(srcdir)/source/lib/sdk \ You shouldn't need to prefix with $(srcdir) here because VPATH is defined these relative paths will automagically be found in VPATH. @@ +36,5 @@ > + > +PKG_STAGE = $(DIST)/test-package-stage > + > +stage-tests-package:: $(TEST_FILES) > + $(INSTALL) $^ $(PKG_STAGE)/jetpack I don't like stage-tests-package target existing here (IMO the *-package targets should only be in testsuite-targets.mk). However, the alternative is having the set of installed files living outside addon-sdk/, away from the source of truth. So, I think this is acceptable.

https://hg.mozilla.org/integration/mozilla-inbound/rev/729d655f29f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6692f7e521

https://hg.mozilla.org/mozilla-central/rev/729d655f29f6 https://hg.mozilla.org/mozilla-central/rev/fb6692f7e521 https://hg.mozilla.org/mozilla-central/rev/c1eda57437ca

https://hg.mozilla.org/mozilla-central/rev/729d655f29f6 https://hg.mozilla.org/mozilla-central/rev/fb6692f7e521 https://hg.mozilla.org/mozilla-central/rev/c1eda57437ca