Closed
Bug 972168
Opened 9 years ago
Closed 9 years ago
Install support files to server root for mochitest
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla30
People
(Reporter: mt, Assigned: mt)
References
Details
Attachments
(2 files, 6 obsolete files)
1.54 KB,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
8.22 KB,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
I'm building code that depends content that is served from the /.well-known directory on a web server (see RFC 5985). Unfortunately, mochitest doesn't permit the installation of support files in that location, since everything is placed in a directory that mirrors the source directory. I don't much care how, but I need a way to control the placement of files relative to the root. Bug 901990 seems to have added the necessary support; I can submit a patch that does what I want, but what about the syntax? Ted suggests perhaps, [DEFAULT] test-path = .well-known/idp-proxy support-files = idp.html idp-proxy.js I'd prefer something that allows for files to be placed in multiple locations, e.g., [DEFAULT] support-files = /.well-known/idp-proxy/idp.html /.well-known/idp-proxy/idp-proxy.js checkaudio.js Also, this needs fixing (it doesn't include .*): http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#173
Assignee | ||
Comment 1•9 years ago
|
||
This might address the most egregious issue. I don't know what level of tolerance we have for make functions across the various build platforms we support, so I started a try run that should hit most platforms: https://tbpl.mozilla.org/?tree=Try&rev=3cbaeda2f748 Unfortunately, selecting files starting with '.' is tricky. The provided glob '.[!.]*' should work; '.[^.]*' didn't work with python's glob package. I'll ask for review if it works. Next up: fix mochitest.ini processing so that I can place the files without Makefile.in hacks.
Assignee | ||
Comment 2•9 years ago
|
||
OK, screwed that push up (didn't qref, I shouldn't be working in mercurial). Updated attempt: https://tbpl.mozilla.org/?tree=Try&rev=b82329696f36
Comment 3•9 years ago
|
||
CCing some people that might have opinions. I actually prefer Martin's proposal from comment 0 to my own proposal, I think you changed something from the original proposal. :) Specifying that support-files with a leading slash get installed to that path under the tests/ directory seems reasonable.
Comment 4•9 years ago
|
||
Also, I was just hacking on the moz.build code for handling manifests a few days ago, so the code you'll need to change for the manifest part is right here: http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#475
Updated•9 years ago
|
Assignee: nobody → martin.thomson
Comment 5•9 years ago
|
||
I know there are a few bugs around supporting additional layouts of support files. It's a hard problem to solve everyone's needs. Changes to the core code are influenced by the scope of the problem in the wild. For the request of installing files in the "server root," can't you just create a temporary directory as part of the test and install files there? What I'm trying to say is "convince me this is a common pattern that needs special attention."
Comment 6•9 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3) > CCing some people that might have opinions. I actually prefer Martin's > proposal from comment 0 to my own proposal, I think you changed something > from the original proposal. :) Specifying that support-files with a leading > slash get installed to that path under the tests/ directory seems reasonable. That could be reasonable. Do tests today know about their paths under the tests/ directory? I think the layout of the tests directory is mostly an implementation detail and do have concerns about this leaking.
Comment 7•9 years ago
|
||
Yes, there's a lot of tests that use absolute paths. Not really sure how support-files = /.well-known/idp-proxy/idp.html works, though. Where should idp.html live in the source tree?
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #5) > For the request of installing files in the "server root," can't you just > create a temporary directory as part of the test and install files there? > What I'm trying to say is "convince me this is a common pattern that needs > special attention." If I could understand how the first sentence is enacted, perhaps I would be less enthusiastic about this change. This RFC 5785 thing is starting to be used a lot more. (In reply to :Ms2ger from comment #7) > support-files = /.well-known/idp-proxy/idp.html > > works, though. Where should idp.html live in the source tree? In dirname(manifest)/idp.html at least according to my patch.
Assignee | ||
Comment 9•9 years ago
|
||
A less kludgy way to fix the copying of files.
Attachment #8375316 -
Attachment is obsolete: true
Attachment #8375699 -
Flags: review?(ted)
Assignee | ||
Comment 10•9 years ago
|
||
Adding the ability to add support-files to the server root.
Attachment #8375700 -
Flags: review?(ted)
Assignee | ||
Comment 11•9 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=d49018132f80 (This is a shotgun run for other less wonderful reasons.)
Updated•9 years ago
|
Attachment #8375699 -
Flags: review?(ted) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8375700 [details] [diff] [review] 0002-Bug-972168-Adding-root-relative-destination-paths-to.patch Review of attachment 8375700 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the review delay, this looks sensible. Conveniently, I have need of this for moving some other mochitests into manifests. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +436,4 @@ > return sub > > def _process_test_manifest(self, sandbox, info, manifest_path): > + flavor, install_root, install_target, filter_inactive = info nitpicky, but "target" has special meaning in the build world, so I'd probably call this "install_subdir" or something like that. @@ +485,4 @@ > if '*' in pattern and thing == 'support-files': > obj.pattern_installs.append( > (manifest_dir, pattern, out_dir)) > + elif pattern[0] == '/': This could probably stand a brief comment. Also, can you add a short blurb about this to the in-tree docs here? http://mxr.mozilla.org/mozilla-central/source/build/docs/test_manifests.rst#172
Attachment #8375700 -
Flags: review?(ted) → review+
Comment 13•9 years ago
|
||
Oh, I forgot to mention, it would also be good to add a unit test for this behavior, something like this: http://hg.mozilla.org/mozilla-central/annotate/bb030d47c946/python/mozbuild/mozbuild/test/frontend/test_emitter.py#l311 These tests use data files like: http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/test/frontend/data/test-manifest-just-support/ You can run those tests with `mach python python/mozbuild/mozbuild/test/frontend/test_emitter.py`.
Assignee | ||
Comment 14•9 years ago
|
||
Rebased. Carrying forward r+ from ted.
Attachment #8375699 -
Attachment is obsolete: true
Attachment #8378483 -
Flags: review+
Assignee | ||
Comment 15•9 years ago
|
||
Rebase was knarly, so I'm going to request another review. Besides, this time I have tests. Documentation in next patch.
Attachment #8375700 -
Attachment is obsolete: true
Attachment #8378484 -
Flags: review?(ted)
Assignee | ||
Comment 16•9 years ago
|
||
Documentation changes for the above change.
Attachment #8378486 -
Flags: review?(ted)
Comment 17•9 years ago
|
||
Comment on attachment 8378486 [details] [diff] [review] 0003-Bug-972168-Documentation-for-support-files-change.patch Review of attachment 8378486 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Just fold this into the other patch for landing.
Attachment #8378486 -
Flags: review?(ted) → review+
Updated•9 years ago
|
Attachment #8378484 -
Flags: review?(ted) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Carrying forward r+ from ted.
Attachment #8378483 -
Attachment is obsolete: true
Attachment #8378604 -
Flags: review+
Assignee | ||
Comment 19•9 years ago
|
||
Carrying forward r+ from ted on merged patch.
Attachment #8378484 -
Attachment is obsolete: true
Attachment #8378486 -
Attachment is obsolete: true
Attachment #8378605 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c75ed0867630 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b08710ab4e2
Flags: in-testsuite+
Keywords: checkin-needed
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c75ed0867630 https://hg.mozilla.org/mozilla-central/rev/7b08710ab4e2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•