Closed Bug 972168 Opened 6 years ago Closed 6 years ago

Install support files to server root for mochitest

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: mt, Assigned: mt)

References

Details

Attachments

(2 files, 6 obsolete files)

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
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.
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
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.
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
Assignee: nobody → martin.thomson
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."
(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.
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?
(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.
A less kludgy way to fix the copying of files.
Attachment #8375316 - Attachment is obsolete: true
Attachment #8375699 - Flags: review?(ted)
Adding the ability to add support-files to the server root.
Attachment #8375700 - Flags: review?(ted)
Try run: https://tbpl.mozilla.org/?tree=Try&rev=d49018132f80

(This is a shotgun run for other less wonderful reasons.)
Attachment #8375699 - Flags: review?(ted) → review+
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+
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`.
Rebased.  Carrying forward r+ from ted.
Attachment #8375699 - Attachment is obsolete: true
Attachment #8378483 - Flags: review+
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)
Documentation changes for the above change.
Attachment #8378486 - Flags: review?(ted)
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+
Attachment #8378484 - Flags: review?(ted) → review+
Carrying forward r+ from ted.
Attachment #8378483 - Attachment is obsolete: true
Attachment #8378604 - Flags: review+
Carrying forward r+ from ted on merged patch.
Attachment #8378484 - Attachment is obsolete: true
Attachment #8378486 - Attachment is obsolete: true
Attachment #8378605 - Flags: review+
Keywords: checkin-needed
Blocks: 878941
https://hg.mozilla.org/mozilla-central/rev/c75ed0867630
https://hg.mozilla.org/mozilla-central/rev/7b08710ab4e2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.