972168 - Install support files to server root for mochitest

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Martin Thomson [:mt:]]

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

Comment 1

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-972168-Including-dot-files-in-the-test-bundle.patch

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.

Comment 2

[Martin Thomson [:mt:]]

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

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

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

Comment 5

[Gregory Szorc [:gps]]

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

[Gregory Szorc [:gps]]

(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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 8

[Martin Thomson [:mt:]]

(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.

Comment 9

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-972168-Including-dot-files-in-the-test-bundle.patch

A less kludgy way to fix the copying of files.

Comment 10

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-972168-Adding-root-relative-destination-paths-to.patch

Adding the ability to add support-files to the server root.

Comment 11

[Martin Thomson [:mt:]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=d49018132f80

(This is a shotgun run for other less wonderful reasons.)

Comment 12

[(not currently active) Ted Mielczarek]

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

Comment 13

[(not currently active) Ted Mielczarek]

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`.

Comment 14

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-972168-Including-dot-files-in-the-test-bundle.patch

Rebased.  Carrying forward r+ from ted.

Comment 15

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-972168-Adding-root-relative-destination-paths-to.patch

Rebase was knarly, so I'm going to request another review.  Besides, this time I have tests.  Documentation in next patch.

Comment 16

[Martin Thomson [:mt:]]

Attachment: 0003-Bug-972168-Documentation-for-support-files-change.patch

Documentation changes for the above change.

Comment 17

[(not currently active) Ted Mielczarek]

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.

Comment 18

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-972168-Including-dot-files-in-the-test-bundle.patch

Carrying forward r+ from ted.

Comment 19

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-972168-Adding-root-relative-destination-paths-to.patch

Carrying forward r+ from ted on merged patch.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c75ed0867630
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b08710ab4e2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c75ed0867630
https://hg.mozilla.org/mozilla-central/rev/7b08710ab4e2