Closed
Bug 1098537
Opened 10 years ago
Closed 10 years ago
detangle reftest's testing+addon build system
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 948278
People
(Reporter: froydnj, Assigned: glandium)
References
Details
Attachments
(6 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•10 years ago
|
||
We want to build httpd.js's module so we can include it in the reftest module.
It'd be nice if all the httpd.js knowledge was contained in one place. This
patch is taking a stab at doing that.
Attachment #8522454 -
Flags: feedback?(gps)
Reporter | ||
Comment 2•10 years ago
|
||
This patch might have belonged with part 1...anyway, we're going to need to
build a separate xpt for the reftest addon, and the jar manifest will need to
reflect that.
Attachment #8522455 -
Flags: feedback?(gps)
Reporter | ||
Comment 3•10 years ago
|
||
I ran across this limitation when I attempted to have the reftest addon have
both its normal manifest and the manifest from the httpd.js module. This patch
is the easiest, most straightforward way I could see to fix it. It may be
completely wrong in parts, particularly as relates to the jar.py bits, which I
have no familiarity with.
However, it does seem to work...although the chrome.manifest for the reftest
addon after this series of patches is not quite identical to what we had
before. I think the changes are harmless, but I haven't checked.
Attachment #8522456 -
Flags: feedback?(gps)
Reporter | ||
Comment 4•10 years ago
|
||
This patch is an explicit hack. I'd very much welcome better ways to do
this...but register_idl had this optional parameter that was just begging to be
used...
Anyway, the root problem is that the build system as it stands doesn't want IDL
files included in different xpt files. Which seems perfectly reasonable, but
interferes with what we're trying to do here.
Attachment #8522457 -
Flags: feedback?(gps)
Reporter | ||
Comment 5•10 years ago
|
||
This patch is the meat of the change; all the XPI_NAME-dependent bits are moved
to their own directory.
You can see some XXX comments and whatnot. This patch is also not ideal
insofar as the jar.mn files for reftest and reftest-addon are almost
identical. Either they should be refactored to share common bits,
or...something else. Feedback welcome.
I haven't tested this on try. However, reftests on my machine do appear to
work, so that's one positive thing going for this patch. We should ask dbaron
at some point whether this all works for him...or just wait for a bugreport and
fix it then. ;)
Attachment #8522458 -
Flags: feedback?(gps)
Reporter | ||
Comment 6•10 years ago
|
||
This patch isn't really necessary, but it was a nice cleanup.
Reporter | ||
Comment 7•10 years ago
|
||
I should also point out that part 5 probably doesn't go far enough, as we're still installing the test harness's version of reftest from xpi-stage...which is undesirable as far as directory ordering goes. I don't have a good idea as to the plan of attack there.
Maybe it's possible we don't need the complete structure of the addon for testing, and we could explicitly list out the files needed there? More experimentation required on that front.
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Anyway, the root problem is that the build system as it stands doesn't want
> IDL
> files included in different xpt files. Which seems perfectly reasonable, but
> interferes with what we're trying to do here.
We could, I suppose, have the emitter maintain a list of files that are permitted to exist in multiple modules, and then add a boolean flag on XPIDLFile indicating whether the backend should permit duplicates of this file. That would be slightly cleaner...
Assignee | ||
Comment 9•10 years ago
|
||
How about this instead: make httpd.js go in its own, separate addon. Then whatever needs it can install it. That would kill all our problems related to multiple things wanting it.
Comment 10•10 years ago
|
||
I am totally okay with the suggestion in comment 9. If the IDL/XPT thing is a problem, I think making us always build and package the httpserver IDL with a Firefox build would be fine too.
Reporter | ||
Comment 11•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #9)
> How about this instead: make httpd.js go in its own, separate addon. Then
> whatever needs it can install it. That would kill all our problems related
> to multiple things wanting it.
Can you use things cross-addon like that? Or is it just that the httpserver addon would expose things via xpcom components, and then reftests (or whoever) could just pick them up?)
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #11)
> (In reply to Mike Hommey [:glandium] from comment #9)
> > How about this instead: make httpd.js go in its own, separate addon. Then
> > whatever needs it can install it. That would kill all our problems related
> > to multiple things wanting it.
>
> Can you use things cross-addon like that? Or is it just that the httpserver
> addon would expose things via xpcom components, and then reftests (or
> whoever) could just pick them up?)
Either is possible, however httpd.js works. IIRC it's an xpcom component.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 13•10 years ago
|
||
Note that one way to solve the IDL/XPT problem would be to make it a jsm module. But I don't know how it's being used and how much change that would mean for callers.
Assignee | ||
Comment 14•10 years ago
|
||
So, a quick look suggests that:
- it's already been made loadable but is still also an xpcom component
- only reftest.js is using the xpcom interface.
- there's already a loadable copy in resource://testing-common/httpd.js for mochitests. Not sure where it comes from.
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #14)
> Not sure where it comes from.
It simply comes from netwerk/test/httpserver/moz.build's TESTING_JS_MODULES.
So it looks to me like we should just add support for the --testing-modules-dir to reftest, like there is for xpcshell and mochitest, and then httpd.js doesn't even need to be an xpcom component at all anymore.
Reporter | ||
Comment 16•10 years ago
|
||
My hacks have inspired glandium to write up something cleaner, so I'm going to mark this bug as assigned to him. I think this also removes the need for DYNAMIC_FINAL_TARGET (yay!) in bug 1058051, so marking as blocker.
Assignee: nfroyd → mh+mozilla
Blocks: 1058051
Assignee | ||
Comment 17•10 years ago
|
||
Damn, turns out I already had a patch for this, and it's been sitting forever. I'll compare the patch there to my current WiP from here.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Updated•10 years ago
|
Attachment #8522454 -
Flags: feedback?(gps)
Reporter | ||
Updated•10 years ago
|
Attachment #8522455 -
Flags: feedback?(gps)
Reporter | ||
Updated•10 years ago
|
Attachment #8522456 -
Flags: feedback?(gps)
Reporter | ||
Updated•10 years ago
|
Attachment #8522457 -
Flags: feedback?(gps)
Reporter | ||
Updated•10 years ago
|
Attachment #8522458 -
Flags: feedback?(gps)
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•