We currently generate .xpt files into a staging directory then install them into $(FINAL_TARGET)/components during libs. We should just generate them directly into their final destination. This is blocked on $(FINAL_TARGET) being available to moz.build files because we need to add the file references to the install manifest.

Now that the mozbuild backend knows about FINAL_TARGET, we are able to install generated xpt files into their final location. This saves us from copying xpt files into their final location on every build. The interaction with FINAL_TARGET is a bit hacky. But I think it works OK. I may or may not roll some interfaces.manifest work into this patch so we can remove the silly buildlist.py rule out of rules.mk. https://tbpl.mozilla.org/?tree=Try&rev=6ab5dca3a211

Reftests didn't like this patch :(

This fixes the issue with test_necko.xpt by adding an explicit INSTALL_TARGET to the necko Makefile.in. Just another incremental hack to support XPI_NAME. Did I mention how much I hate XPI_NAME? https://tbpl.mozilla.org/?tree=Try&rev=ee9175eb2f6e

Comment on attachment 822494 [details] [diff] [review] Generate xpt files into final location Review of attachment 822494 [details] [diff] [review]: ----------------------------------------------------------------- ::: Makefile.in @@ +65,5 @@ > js-config-status: > $(call SUBMAKE,backend.RecursiveMakeBackend.built,js/src,1) > endif > > +install_manifests := bin idl include public private sdk xpi-stage xp*t*-stage. xpi is something else. ::: js/xpconnect/tests/idl/Makefile.in @@ +5,5 @@ > > include $(topsrcdir)/config/rules.mk > > componentdir = js/xpconnect/tests/components > +libs:: $(FINAL_TARGET)/components/$(MODULE).xpt Not really awesome to install test xpts but i guess we can't do much better for now. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +863,5 @@ > + dist = mozpath.join(self.environment.topobjdir, 'dist') > + if path.startswith(dist): > + path = path[len(dist)+1:] > + > + offset = path.index('/') prefix, subpath = path.split('/', 1) Note this fails if there is no '/' in the path, but on the other hand your code doesn't handle that well either. At least this version would fail with noise. @@ +869,5 @@ > + > + key = 'dist_%s' % prefix > + > + manifest = self._install_manifests[key] > + manifest.add_optional_exists(path[offset+1:]) manifest.add_optional_exists(subpath) @@ +988,5 @@ > + try: > + os.makedirs(d) > + except OSError as e: > + if e.errno != errno.EEXIST: > + raise I think that should be handled by dependencies on $(call mkdir_deps,...) instead. ::: python/mozbuild/mozbuild/frontend/data.py @@ +68,5 @@ > > + # We strip 'dist' because most consumers don't care about that part. > + install_target = sandbox['FINAL_TARGET'] > + offset = install_target.find('/') > + self.install_target = install_target[offset+1:] Does FINAL_TARGET really always start with dist and nothing else significant? I'd feel better if you didn't strip this and used depth instead of dist when writing the xpt rules. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +95,5 @@ > """ > + # We only want to emit an InstallationTarget if one of the consulted > + # variables is defined. Later on, we look up FINAL_TARGET, which has > + # the side-effect of populating it. So, we need to do this lookup > + # early. Shouldn't we have and use a sandbox.has/hasattr/contains/whatever, then ?

(In reply to Mike Hommey [:glandium] from comment #4) > Comment on attachment 822494 [details] [diff] [review] > Generate xpt files into final location > > Review of attachment 822494 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: Makefile.in > @@ +65,5 @@ > > js-config-status: > > $(call SUBMAKE,backend.RecursiveMakeBackend.built,js/src,1) > > endif > > > > +install_manifests := bin idl include public private sdk xpi-stage > > xp*t*-stage. xpi is something else. Actually, it is that one. We install WorkerTest.xpt into dist/xpi-stage/worker/components, so we need to track dist/xpi-stage.

Rebased

Note that this doesn't build yet.

In particular, the patches cause IOError: [Errno 2] No such file or directory: 'js/src/_build_manifests/install/dist_xpi-stage' and I'm way out of my depth to fix that.

I stumbled across this and thought I'd take a quick stab at updating the patches. The only major difference is that this part: > ::: js/xpconnect/tests/idl/Makefile.in > @@ +5,5 @@ > > > > include $(topsrcdir)/config/rules.mk > > > > componentdir = js/xpconnect/tests/components > > +libs:: $(FINAL_TARGET)/components/$(MODULE).xpt > > Not really awesome to install test xpts but i guess we can't do much better > for now. can be better now that TEST_HARNESS_FILES is a thing (although I did have to make that support absolute objdir paths).

/r/1187 - Bug 923080 - Generate xpt files into final location Pull down this commit: hg pull review -r ea85f8ae3dd3a27facb96e51569876fadb857b74

Comment on attachment 8532821 [details] MozReview Request: bz://923080/bokeefe Looks like I broke xpcshell... I'll figure that out and re-request review.

/r/1187 - Bug 923080 - Generate xpt files into final location Pull down this commit: hg pull review -r d99f51d63c96fc7da7d8792fd739e6bf8668b892

(In reply to Brian O'Keefe [:bokeefe] from comment #13) > Comment on attachment 8532821 [details] > MozReview Request: bz://923080/bokeefe > > Looks like I broke xpcshell... I'll figure that out and re-request review. So, it turns out that js/xpconnect/tests/idl stopped being traversed during libs. I had to arrange for TEST_HARNESS_FILES to cause the libs tier not to be skipped when it emits the install targets for files in the objdir. Green try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=33926258bd16

https://reviewboard.mozilla.org/r/1185/#review715 ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > + key = 'dist_%s' % prefix This should probably fail if the path is not under dist. ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > + path = path[len(dist)+1:] use mozpath.relpath ::: netwerk/test/httpserver/Makefile.in (Diff revision 2) > +INSTALL_TARGETS += reftest_xpt You should land this after bug 948278 and drop this hunk. ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > + self._may_skip['libs'].remove(backend_file.relobjdir) Ouch. This shouldn't be needed, and the reason why you need it is because TEST_HARNESS_FILES is not listed as a 'libs' variable in context.py. Change that there instead. ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > + need_libs = False You're not using this variable. ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > 'xpt/.mkdir.done'): I think you can remove xpt/.mkdir.done here. ::: js/xpconnect/tests/idl/moz.build (Diff revision 2) > +TEST_HARNESS_FILES.xpcshell.js.xpconnect.tests.components.native += [ Please add a comment that this relies on xpctest.xpt ending up in dist/bin/components during export, and TEST_HARNESS_FILES being processed after that. ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > - '$(idl_xpt_dir)/%s.xpt: %s' % (module, ' '.join(idl_deps)), > + xpt_path = '%s/%s/components/%s.xpt' % (self.environment.topobjdir, install_target, module) Use '$(DEPTH)' instead of self.environment.topobjdir? ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > - '', > + rule.add_dependencies('$(dist_idl_dir)/%s.idl' % dep for dep in deps) If we have all the idl dependencies, at this point we might as well point to the actual source instead of the $(dist_idl_dir) file, which we actually have no dependencies in place to ensure things are copied there before things happen here. ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > + '@echo "$(notdir $@)"', $(notdir $@) can be written as $(@F) ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > + for d in set(os.path.dirname(p) for p in xpt_files): I don't think this should be done here. Instead, each target xpt file should depend on mkdir_deps for their directory. ::: python/mozbuild/mozbuild/frontend/emitter.py (Diff revision 2) > + if context.get('FINAL_TARGET') or context.get('XPI_NAME') or \ any(k in context for k in ('FINAL_TARGET', ...)) ::: config/makefiles/xpidl/Makefile.in (Diff revision 2) > - $(dist_include_dir) $(idl_xpt_dir) $(idl_deps_dir) > + $(dist_include_dir) $(dir $@) $(idl_deps_dir) $(dir $@) can be written as $(@D) While here, you might as well make things smarter, by having a generic target here and only have dependencies emitted later. something like: %.xpt: @echo "$(@F)" $(PYTHON_PATH) $(PLY_INCLUDE) ... $(@D) $(idl_deps_dir) $(basename $(notdir $@ $(filter %.idl,$^))) ::: python/mozbuild/mozbuild/backend/recursivemake.py (Diff revision 2) > +from StringIO import StringIO from io import StringIO (if that works)

/r/1187 - Bug 923080 - Generate xpt files into final location Pull down this commit: hg pull review -r e2dc0fe0adc225a1b8a9feb08c60ffcb024725e5

https://reviewboard.mozilla.org/r/1185/#review879 > You should land this after bug 948278 and drop this hunk. I did this, and then wondered why I broke reftests in the try push. Obviously, not including the patch from that bug breaks it. > from io import StringIO (if that works) When I tried that, I got 0:37.71 File "c:\mozilla-src\mozilla\python\mozbuild\mozbuild\makeutil.py", line 143, in dump 0:37.71 fh.write('\n') 0:37.71 TypeError: unicode argument expected, got 'str' so I just left it as-is. > Ouch. This shouldn't be needed, and the reason why you need it is because TEST_HARNESS_FILES is not listed as a 'libs' variable in context.py. Change that there instead. I was afraid doing that would cause all of the TEST_HARNESS_FILES directories to be recursed during libs, but it seems that's not the case. I compared before and after versions of root.mk, and they looked the same.

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=02c72a723e9a (Reftests are broken because this needs bug 948278 first)

https://reviewboard.mozilla.org/r/1185/#review999 ::: python/mozbuild/mozbuild/frontend/context.py (Diff revision 3) > - """, None), > + """, 'libs'), Come to think of it, what you did in the previous iteration was actually theoretically better. BUT, most TEST_HARNESS_FILES directories have a Makefile.in anyways, so they'd be recursed anyways. I think it's fine for now. Please file a followup to make the objdir_files processing not use backend.mk.

(In reply to Mike Hommey [:glandium] from comment #20) > https://reviewboard.mozilla.org/r/1185/#review999 > > ::: python/mozbuild/mozbuild/frontend/context.py > (Diff revision 3) > > - """, None), > > + """, 'libs'), > > Come to think of it, what you did in the previous iteration was actually > theoretically better. BUT, most TEST_HARNESS_FILES directories have a > Makefile.in anyways, so they'd be recursed anyways. I think it's fine for > now. Please file a followup to make the objdir_files processing not use > backend.mk. Bug 1113622 covers fixing the objdir_files situation.