This patch adds TEST_HARNESS_FILES to the moz.build sandbox. It's like EXPORTS except you can't assign to the value itself, you have to assign to a property. Stuff in it gets installed under $objdir/_tests. The one thing I need to nail down is the expression of copying files from other directories etc. One look at the mochitest Makefile shows that we copy crap from all over the place: http://hg.mozilla.org/mozilla-central/annotate/1507f021ac93/testing/mochitest/Makefile.in#l14 It would be nice to make that less awful in moz.build.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0) > The one thing I need to nail down is the expression of copying files from > other directories etc. One look at the mochitest Makefile shows that we copy > crap from all over the place: > http://hg.mozilla.org/mozilla-central/annotate/1507f021ac93/testing/ > mochitest/Makefile.in#l14 WDYT about adopting the same convention as LOCAL_INCLUDES uses? - Anything starting with '/' appends $(topsrcdir); - Everything else is current-directory relative. Also, maybe we could just make this HierarchicalStringList and have the sandbox verify that the root has no files in it?

(In reply to Nathan Froyd (:froydnj) from comment #2) > WDYT about adopting the same convention as LOCAL_INCLUDES uses? > > - Anything starting with '/' appends $(topsrcdir); > - Everything else is current-directory relative. This doesn't cover the "copy files from the objdir" use case, unfortunately. Maybe we can just invent some arbitrary syntax for that? > Also, maybe we could just make this HierarchicalStringList and have the > sandbox verify that the root has no files in it? Yeah, that's probably a smarter way to do it.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3) > (In reply to Nathan Froyd (:froydnj) from comment #2) > > WDYT about adopting the same convention as LOCAL_INCLUDES uses? > > > > - Anything starting with '/' appends $(topsrcdir); > > - Everything else is current-directory relative. > > This doesn't cover the "copy files from the objdir" use case, unfortunately. Ah, I must not be seeing those cases in the Makefile.in example you gave. I can certainly imagine there being some, though! > Maybe we can just invent some arbitrary syntax for that? I guess this is from arbitrary directories in the objdir? Presumably from the current directory, it'd just be a GeneratedFile or somesuch? Maybe we can make it a unicode character to discourage people from doing that. ;) Failing that, I think I vote for '!'.

Thinking about this some more: WDYT about something like: TEST_HARNESS_FILES += [ 'some/srcdir/file', ObjdirFile(...), ... ] Or perhaps: TEST_HARNESS_FILES += [ 'some/srcdir/file', 'dist/bin/objdir/file', ... ] TEST_HARNESS_FILES['dist/bin/objdir/file'].from_objdir = True I kind of like the second one for requiring less infrastructure, but I feel like eventually people are going to start writing: TEST_HARNESS_FILES += [ ... ] for f in TEST_HARNESS_FILES: if f.startswith('dist/bin/'): f.from_objdir = True or somesuch, in which case we might as well have gone with the first one.

Those are both a little ugly, I actually like your suggestion from comment 4 of just having a sigil character, like: TEST_HARNESS_FILES += [ 'srcdir_file', '/top/of/srcdir_file', '!objdir_file', '!/top/of/objdir_file', ] If you want to take this over feel free. :) I don't think the patch here provides much value as-is. If not I'll take a crack at it soon.

I also like the idea of a sigil to identify the objdir.

glandium pointed out bug 991983, which has the results of the last discussion we had about standardizing paths. I think the ideas there + the objdir sigil idea we discussed here would be a great start.

I started doing this today and ran into some difficulties straightening everything out. As Ted indicated, converting all of testing/mochitest/Makefile.in to whatever we do here would be nice, so... TEST_HARNESS_FILES worked OK to convert away all the INSTALL_TARGETS stuff in that file: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#62 http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#68 http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#86 http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#96 I didn't try to deal with the weird situation we have with automation.py getting built multiple times for each testsuite: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#99 (and some testsuites, like xpcshell, just copying in automation.py from someplace else:) http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#46 But the rest of the stuff in that file is setting up things for test packaging, which is very different than what TEST_HARNESS_FILES is trying to do. TEST_HARNESS_FILES is concerned about moving things from the srcdir into their proper position in the objdir. (There are sometimes files from the objdir: http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#39 http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/Makefile.in#24 and it's easy to textually represent those in moz.build, but I haven't convinced myself that installing them in libs:: or similar would happen at the correct time.) Setting up test packaging info is almost the same, but we're primarily concerned with mapping files (oftentimes whole directories) from the objdir to places in the packaging directory hierarchy[1]. (Occasionally files from the srcdir are involved here:) http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#167 http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#184 I think this is sufficiently different to warrant another moz.build methodology (PACKAGED_FILES?), particularly since it'd be reasonable to provide for some/directory/**-style wild-carding in that context, which is something that most of the rest of moz.build has eschewed thus far. Does that sound wildly off-target? [1] Someday, we should do that all in Python, and then we wouldn't need all these stage-package targets, we could just generate the package directly, in Python, using the zip or tar or whatever modules.

There has been talk of a generic file copying feature before. See bug 853594, comment 5 in particular. As for this bug, remember that objdir/_tests is a giant hack to make creating the tests zip simpler and to make running tests easier. In the ideal world, we avoid this directory tree and its 19000+ files entirely. Instead, we run tests directly from the source directory (or from the objdir if test files are created dynamically) and we create the tests archive dynamically, likely via Python-native APIs. This avoids a ton of I/O, which can be a massive build slowdown, especially on Windows. Now, eliminating _tests is not a reasonable first step. The test harnesses will complain loudly because they assume things all exist in _tests. However, we could start to think about being more efficient with the population of _tests. For example, for developers who never package tests and never run mochitests, we don't necessarily need to install mochitest files into _tests. I'm not sure if those comments add any value to what you are doing. But this overall context wasn't captured in this bug and I figured you may benefit... somehow.

froydnj: I would like this to target both the "copy to _tests" step as well as the "make test package" step. I know they don't currently match 100%, but I think we ought to be able to align them without too many problems. I don't think it'd be harmful to copy extra stuff to _tests that we only need for the test package, for example. I'm not sure if we'll ever get to gps' ideal world above, since it'd require a lot of test harness and test hacking, but we could certainly get to a point where we eliminate the test-package-stage nonsense and just build the zip file directly from Python (or build a set of smaller zips or something neater).

This path actually comes from gps's work in bug 853594. All I did was rebase, and I'll put gps's name on it before commit, but we should get this into the tree somehow.

This is Ted's patch with a lot more logic in recursivemake.py to handle things from the objdir and to handle the packaging steps as well. glandium was complaining last night on IRC about some of this stuff; arguably it should be moved to the directories which compile the necessary components. Maybe with .for_mochitest or .for_xpcshell flags on things once programs/libraries/plugins/etc. are moved into moz.build. That feels like boiling the ocean to me, so I didn't do it that way. I'm not completely happy with the objdir path mechanism, nor am I thrilled about how directories get copied over, but I wanted to at least get the work out there and get feedback on it.

To provide a little extra flavor, here's another Makefile.in converted to use the new mechanism. I didn't '$VCS rm' the Makefile.in because I am lazy.

Also, FTR, I felt that keeping the stage-package stuff separate was right and good; there's no sense in copying even more (large binaries!) into _tests, which will just sit there until somebody gets rid of _tests. The mechanism I may have chosen may not be right and good, of course...

Comment on attachment 8473819 [details] [diff] [review] part 2 - add TEST_HARNESS_FILES and convert testing/mochitest/ to use it Review of attachment 8473819 [details] [diff] [review]: ----------------------------------------------------------------- I like the non for_package bits of TEST_HARNESS_FILES. I'm not crazy about for_package. Convince me we need to tackle that here and now. (I want to see it improve too, but baby steps.) Given the new moz.build template foo being written in bug 1041941, I almost wonder if we should write the custom INSTALLS magic now and expose TEST_HARNESS_FILES as a template that magically maps to a generic install mechanism. glandium may have opinions. Feel free to call scope creep on me. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +888,5 @@ > namespace="", > action=handle_export) > > + def _process_test_harness_files(self, obj, files, backend_file): > + def resolve_file(f): This function needs to live somewhere reusable since we have other plans for it. @@ +911,5 @@ > + if path.startswith('for_package'): > + continue > + > + objdir_files = list(itertools.ifilter(is_objdir_file, e._strings)) > + srcdir_files = itertools.ifilterfalse(is_objdir_file, e._strings) Having to access a _ prefixed attribute from an "external" class is a sign you are doing something wrong. @@ +917,5 @@ > + for f in srcdir_files: > + source = resolve_file(f) > + dest = '%s/%s' % (path, mozpath.basename(source)) > + if not os.path.exists(source): > + raise Exception('File listed in TEST_HARNESS_FILES does not exist: %s' % source) We prefer to perform validation like this in emitter.py, where the code is exercised among all backends. Generally speaking, we want backends to be dumb data consumers. We haven't done a good job of enforcing that design intention. @@ +948,5 @@ > + tar_template = '(cd %(source)s && tar $(TAR_CREATE_FLAGS) - %(files)s) | (cd $(PKG_STAGE)/%(pkg_path)s && tar -xf -)' > + all_install_rule_names = [] > + for path, e in files.for_package.walk_descendants(): > + if not len(e._strings): > + continue Nit: len([]) is 0 which is False. |if len(x)| is equivalent to |if x| for container types. This only fails to hold if __nonzero__ does something funky. That almost never happens. See https://docs.python.org/2/reference/datamodel.html#object.__nonzero__. @@ +989,5 @@ > + # One command to rule them all. > + backend_file.write(""" > +stage-package: %s > + > +""" % ' '.join(all_install_rule_names + [nsinstall_dir_rule])) I would strongly prefer we use this opportunity to kill this tar pipe cruft. We have mozpack.copier now: we should use it. I would almost favor tackling the "for_package" foo in a follow-up bug. But seeing it all here is nice to see what we're dealing with. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +816,5 @@ > + ``TEST_HARNESS_FILES`` can also be used to install files for > + test packaging, by setting fields in the > + ``TEST_HARNESS_FILES.for_package`` field. Files from the objdir that > + should be part of the test package are indicated with a leading > + ``'!'``. Unlike many other lists of files in moz.build, directories I approve of adopting syntax for declaring paths in the objdir. But we do need to consider directory vs objdir relative paths. How about '!foo' is "relative to current directory" and '!/foo' is "relative to objdir root?" @@ +817,5 @@ > + test packaging, by setting fields in the > + ``TEST_HARNESS_FILES.for_package`` field. Files from the objdir that > + should be part of the test package are indicated with a leading > + ``'!'``. Unlike many other lists of files in moz.build, directories > + may be installed with ``TEST_HARNESS_FILES.for_package``. For example:: Having a HierarchicalStringList under a HierarchicalStringListWithFlagsFactory is very weird. TEST_HARNESS_FILES and TEST_HARNESS_FILES.for_package are very similar but do different things. The docs aren't very clear when one should be used over the other. You can also make the case that we should be using separate variables here. @@ +819,5 @@ > + should be part of the test package are indicated with a leading > + ``'!'``. Unlike many other lists of files in moz.build, directories > + may be installed with ``TEST_HARNESS_FILES.for_package``. For example:: > + TEST_HARNESS_FILES.for_package += [ > + '/build/pgo/certs/', For directories, elsewhere we're using the wildcard support in mozpack for matching files. Instead of recording directory copies, we store a wildcard like "/build/pgo/certs/**" and use the file matching code to expand that at run-time. We like using wildcards instead of directories because the mozpack.copier code doesn't know about directories and it's best to keep it that way. ::: testing/mochitest/moz.build @@ +178,5 @@ > +test_scripts = ['!dist/bin/%s' % s for s in test_scripts] > +test_libraries = ['!dist/bin/%s%s' % (l, CONFIG['DLL_SUFFIX']) > + for l in test_libraries] > +test_components = ['!dist/bin/components/%s' % c for c in test_components] > +test_plugins = ['!dist/plugins/%s' % p for p in test_plugins] I almost wonder if these properties should be declared on binaries, scripts, libraries, etc themselves, in the moz.build files that define them. Moving away from fragmented definitions is a goal of moz.build files after all.

Comment on attachment 8473820 [details] [diff] [review] part 3 - example conversion of testing/xpcshell/Makefile.in to moz.build Review of attachment 8473820 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/xpcshell/moz.build @@ +24,5 @@ > + 'head.js', > + 'moz-http2/', > + 'moz-spdy/', > + 'node-http2/', > + 'node-spdy/', I think I like wildcard file matching over directories.

(In reply to Gregory Szorc [:gps] from comment #16) > Comment on attachment 8473819 [details] [diff] [review] > part 2 - add TEST_HARNESS_FILES and convert testing/mochitest/ to use it > > Review of attachment 8473819 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like the non for_package bits of TEST_HARNESS_FILES. I'm not crazy about > for_package. Convince me we need to tackle that here and now. (I want to see > it improve too, but baby steps.) I don't have strong feelings either way, just trying to clear out Makefile.ins at this point. > Given the new moz.build template foo being written in bug 1041941, I almost > wonder if we should write the custom INSTALLS magic now and expose > TEST_HARNESS_FILES as a template that magically maps to a generic install > mechanism. glandium may have opinions. Feel free to call scope creep on me. ESCOPECREEP, EDONTUNDERSTANDALLTHEMAGIC > @@ +911,5 @@ > > + if path.startswith('for_package'): > > + continue > > + > > + objdir_files = list(itertools.ifilter(is_objdir_file, e._strings)) > > + srcdir_files = itertools.ifilterfalse(is_objdir_file, e._strings) > > Having to access a _ prefixed attribute from an "external" class is a sign > you are doing something wrong. Yeah, part 1 did not come out the cleanest, having to touch _-prefixed things elsewhere. I think it should move to a more os.walk()-style model, which would eliminate the walk_strings/walk_descendants distinction... > @@ +989,5 @@ > > + # One command to rule them all. > > + backend_file.write(""" > > +stage-package: %s > > + > > +""" % ' '.join(all_install_rule_names + [nsinstall_dir_rule])) > > I would strongly prefer we use this opportunity to kill this tar pipe cruft. > We have mozpack.copier now: we should use it. I don't understand how one would use that in the stage-package stuff. Generating a 'packaged-tests' manifest and then just having that serve as the basis of the stage-package build step? > ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py > @@ +816,5 @@ > > + ``TEST_HARNESS_FILES`` can also be used to install files for > > + test packaging, by setting fields in the > > + ``TEST_HARNESS_FILES.for_package`` field. Files from the objdir that > > + should be part of the test package are indicated with a leading > > + ``'!'``. Unlike many other lists of files in moz.build, directories > > I approve of adopting syntax for declaring paths in the objdir. But we do > need to consider directory vs objdir relative paths. How about '!foo' is > "relative to current directory" and '!/foo' is "relative to objdir root?" Yeah, the resolve_file function adopted that convention, but I didn't use it very consistently. When starting to rework these patches, I stuck a resolve_path function in Context, which makes things much nicer. > @@ +817,5 @@ > > + test packaging, by setting fields in the > > + ``TEST_HARNESS_FILES.for_package`` field. Files from the objdir that > > + should be part of the test package are indicated with a leading > > + ``'!'``. Unlike many other lists of files in moz.build, directories > > + may be installed with ``TEST_HARNESS_FILES.for_package``. For example:: > > Having a HierarchicalStringList under a > HierarchicalStringListWithFlagsFactory is very weird. > > TEST_HARNESS_FILES and TEST_HARNESS_FILES.for_package are very similar but > do different things. The docs aren't very clear when one should be used over > the other. You can also make the case that we should be using separate > variables here. I wondered about that in comment 9, but it was suggested in comment 11 that it wouldn't be "too much work" to stuff them into a single concept. Having to try to write the documentation for TEST_HARNESS_FILES and your comments is suggesting that maybe I was on the right track in the first place. > ::: testing/mochitest/moz.build > @@ +178,5 @@ > > +test_scripts = ['!dist/bin/%s' % s for s in test_scripts] > > +test_libraries = ['!dist/bin/%s%s' % (l, CONFIG['DLL_SUFFIX']) > > + for l in test_libraries] > > +test_components = ['!dist/bin/components/%s' % c for c in test_components] > > +test_plugins = ['!dist/plugins/%s' % p for p in test_plugins] > > I almost wonder if these properties should be declared on binaries, scripts, > libraries, etc themselves, in the moz.build files that define them. Moving > away from fragmented definitions is a goal of moz.build files after all. I would be in favor of that, but I'm not sure how to make it happen in the current moz.build world.

...and I forgot the ni? to encourage gps to elaborate on some of his feedback.

(In reply to Nathan Froyd (:froydnj) from comment #18) > > I like the non for_package bits of TEST_HARNESS_FILES. I'm not crazy about > > for_package. Convince me we need to tackle that here and now. (I want to see > > it improve too, but baby steps.) > > I don't have strong feelings either way, just trying to clear out > Makefile.ins at this point. "Clearing out Makefile.ins" does not need to be an explicit goal, depending on what your objective is. Directory traversal now has the smarts to only traverse directories that are relevant, rather than the full set of directories. So, we can have 500 Makefile.in but only execute them once, at exactly the point we need to. I hate Makefile.in too. But try not to get too caught up on their existence. > > @@ +989,5 @@ > > > + # One command to rule them all. > > > + backend_file.write(""" > > > +stage-package: %s > > > + > > > +""" % ' '.join(all_install_rule_names + [nsinstall_dir_rule])) > > > > I would strongly prefer we use this opportunity to kill this tar pipe cruft. > > We have mozpack.copier now: we should use it. > > I don't understand how one would use that in the stage-package stuff. > Generating a 'packaged-tests' manifest and then just having that serve as > the basis of the stage-package build step? It's possible to construct an in-memory InstallManifest() object and/or FileCopier(). I propose writing a function somewhere that does this instead of tar pipe foo. See e.g. http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/process_install_manifest.py > Yeah, the resolve_file function adopted that convention, but I didn't use it > very consistently. When starting to rework these patches, I stuck a > resolve_path function in Context, which makes things much nicer. +1 > > ::: testing/mochitest/moz.build > > @@ +178,5 @@ > > > +test_scripts = ['!dist/bin/%s' % s for s in test_scripts] > > > +test_libraries = ['!dist/bin/%s%s' % (l, CONFIG['DLL_SUFFIX']) > > > + for l in test_libraries] > > > +test_components = ['!dist/bin/components/%s' % c for c in test_components] > > > +test_plugins = ['!dist/plugins/%s' % p for p in test_plugins] > > > > I almost wonder if these properties should be declared on binaries, scripts, > > libraries, etc themselves, in the moz.build files that define them. Moving > > away from fragmented definitions is a goal of moz.build files after all. > > I would be in favor of that, but I'm not sure how to make it happen in the > current moz.build world. You would change the relevant variables (SOURCES, etc) into one of the *WithFlagsFactory classes (e.g. StrictOrderingOnAppendListWithFlagsFactory) and you would declare attributes/flags that impact packaging. e.g. EXTRA_JS_MODULES['foo.jsm'].test_package_in_foo = True You would need to read these attributes/flags in emitter.py and pass them along to the backend to do something meaningful. I concede that's a bit complicated for someone not familiar with how the data flow works. It may help to read https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/buildsystem/mozbuild-files.html#mozbuild-files

(In reply to Gregory Szorc [:gps] from comment #20) > I hate Makefile.in too. But try not to get too caught up on their existence. Fair enough. Let's say that trying to deal with TEST_HARNESS_FILES and PACKAGED_TEST_FILES (as a strawman name) is intertwined enough that they are worth dealing with together. And getting to the point where we don't have to copy everything into _tests etc. is a worthy goal in and of itself. > > > @@ +989,5 @@ > > > > + # One command to rule them all. > > > > + backend_file.write(""" > > > > +stage-package: %s > > > > + > > > > +""" % ' '.join(all_install_rule_names + [nsinstall_dir_rule])) > > > > > > I would strongly prefer we use this opportunity to kill this tar pipe cruft. > > > We have mozpack.copier now: we should use it. > > > > I don't understand how one would use that in the stage-package stuff. > > Generating a 'packaged-tests' manifest and then just having that serve as > > the basis of the stage-package build step? > > It's possible to construct an in-memory InstallManifest() object and/or > FileCopier(). I propose writing a function somewhere that does this instead > of tar pipe foo. See e.g. > http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/ > action/process_install_manifest.py I feel like this is leaving out several steps, but that's probably based on my poor model of what/how manifests are used for. My model says that a manifest is constructed by a backend, then used at some later point in the build process to shuffle files around. I'm not quite sure this model is applicable to packaging stuff, as a number of the files that one wants to package won't exist at manifest construction time (i.e. the backend). I think your comment above is indicating that's correct, but I'm unsure of where you're proposing to store the manifest and/or invoke the shuffling step involved with the manifest; stage-package isn't going to permit invoking arbitrary python functions, unless you're proposing something like: stage-package: $(call py_action,package_stuff,objdir/dest/dir/path/ /build/pgo/certs/** /some/other/srcdir/to/copy/** !/objdir/path/to/thing !/other/objdir/path/to/thing ...) Apologies for being dense here. > > > ::: testing/mochitest/moz.build > > > @@ +178,5 @@ > > > > +test_scripts = ['!dist/bin/%s' % s for s in test_scripts] > > > > +test_libraries = ['!dist/bin/%s%s' % (l, CONFIG['DLL_SUFFIX']) > > > > + for l in test_libraries] > > > > +test_components = ['!dist/bin/components/%s' % c for c in test_components] > > > > +test_plugins = ['!dist/plugins/%s' % p for p in test_plugins] > > > > > > I almost wonder if these properties should be declared on binaries, scripts, > > > libraries, etc themselves, in the moz.build files that define them. Moving > > > away from fragmented definitions is a goal of moz.build files after all. > > > > I would be in favor of that, but I'm not sure how to make it happen in the > > current moz.build world. > > You would change the relevant variables (SOURCES, etc) into one of the > *WithFlagsFactory classes (e.g. StrictOrderingOnAppendListWithFlagsFactory) > and you would declare attributes/flags that impact packaging. e.g. > > EXTRA_JS_MODULES['foo.jsm'].test_package_in_foo = True > > You would need to read these attributes/flags in emitter.py and pass them > along to the backend to do something meaningful. I concede that's a bit > complicated for someone not familiar with how the data flow works. It may > help to read > https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/ > buildsystem/mozbuild-files.html#mozbuild-files I would prefer moving everything into moz.build, then moving everything into the directories into which they belong, in that order.

(In reply to Nathan Froyd (:froydnj) from comment #21) > > It's possible to construct an in-memory InstallManifest() object and/or > > FileCopier(). I propose writing a function somewhere that does this instead > > of tar pipe foo. See e.g. > > http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/ > > action/process_install_manifest.py > > I feel like this is leaving out several steps, but that's probably based on > my poor model of what/how manifests are used for. My model says that a > manifest is constructed by a backend, then used at some later point in the > build process to shuffle files around. I'm not quite sure this model is > applicable to packaging stuff, as a number of the files that one wants to > package won't exist at manifest construction time (i.e. the backend). What if I told you the copier/manifest code was originally written for packaging and was later expanded on to provide functionality to the build system :) > I think your comment above is indicating that's correct, but I'm unsure of > where you're proposing to store the manifest and/or invoke the shuffling > step involved with the manifest; stage-package isn't going to permit > invoking arbitrary python functions, unless you're proposing something like: > > stage-package: > $(call py_action,package_stuff,objdir/dest/dir/path/ /build/pgo/certs/** > /some/other/srcdir/to/copy/** !/objdir/path/to/thing > !/other/objdir/path/to/thing ...) I'm proposing that the stage-package target should invoke a Python script and that Python script should construct an InstallManifest(), feed that into a FileCopier(), and have FileCopier do the I/O using it's rsync-like mechanism. We can populate that InstallManifest any way you want. Some options: 1) Hard-coding patterns in the script itself (the "port it from Makefile.in" approach). This is easiest. 2) From moz.build via a written install manifest file. 3) From moz.build via a custom serialized format. e.g. write JSON with all the metadata and write custom code for converting it to an InstallManifest. In the case of test .ini manifests, we extract patterns from e.g. support-files and store them in an install manifest, which we write to disk. See https://hg.mozilla.org/mozilla-central/file/f9bfe115fee5/python/mozbuild/mozbuild/frontend/emitter.py#l877 and https://hg.mozilla.org/mozilla-central/file/f9bfe115fee5/python/mozbuild/mozbuild/backend/recursivemake.py#l1054 > > You would need to read these attributes/flags in emitter.py and pass them > > along to the backend to do something meaningful. I concede that's a bit > > complicated for someone not familiar with how the data flow works. It may > > help to read > > https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/ > > buildsystem/mozbuild-files.html#mozbuild-files > > I would prefer moving everything into moz.build, then moving everything into > the directories into which they belong, in that order. Fair enough.

Having to walk over elements and strings of HierarchicalStringList with an external recursive function is un-Pythonic and adds unnecessary obfuscation to several tasks. Add a walk() function to HierarchicalStringList, modeled on os.walk(), to handle these cases more directly. I went for the proxy object approach for the sequence of strings because I felt that it made client code easier: for path, strings in hsl.walk(): vs. something like: for path, strings, flags in hsl.walk(): I think having to call flags_for() makes it slightly more obvious when things are dealing with flags. Tests still have to touch _-prefixed things, but I don't think this is too bad. This is gps's code originally, so just asking him for f? on how things have been tweaked, glandium for the real review.

This version fixes things up for the new .walk() and dispenses with the .for_package bits. It's much shorter as a result. We can move the hypothetical TEST_PACKAGED_FILES to a new bug.

This patch moves testing/mozbase/ over to the new hotness as an example of how one can use pattern rules in TEST_HARNESS_FILES, which necessitates extending TEST_HARNESS_FILES to handle pattern rules.

Comment on attachment 8481348 [details] [diff] [review] part 1 - add walking functions to HierarchicalStringList Review of attachment 8481348 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a huge fan of accessing ._strings in test_emitter.py. But meh.

Comment on attachment 8481351 [details] [diff] [review] part 2 - add TEST_HARNESS_FILES and convert testing/mochitest/ to use it Review of attachment 8481351 [details] [diff] [review]: ----------------------------------------------------------------- r+ should be nearly automatic with comments addressed. ::: python/mozbuild/mozbuild/frontend/context.py @@ +158,5 @@ > + > + Paths may be relative to the current srcdir or objdir, or to the > + environment's topsrcdir or topobjdir. Different resolution contexts > + are denoted by characters at the beginning of the path: > + Nit: trailing whitespace @@ +162,5 @@ > + > + * '/' - relative to topsrcdir; > + * '!/' - relative to topobjdir; > + * '!' - relative to objdir; and > + * any other character - relative to srcdir. Thank you for implementing !/, even though TEST_HARNESS_FILES doesn't use it. @@ +168,5 @@ > + if path[0] == '!': > + if path[1] == '/': > + resolved = mozpath.join(self.config.topobjdir, path[2:]) > + else: > + resolved = mozpath.join(self.objdir, path[1:]) I'd feel slightly better if this were collapsed into a flag if elif elif... If nothing else, the unchecked index offset of path[1] would be fixed since |if path[0:2] == '!/'| works, even if path=''. @@ +179,5 @@ > + > + @staticmethod > + def is_objdir_path(path): > + return path[0] == '!' > + Trailing whitespace. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +511,5 @@ > + test_harness_files = context.get('TEST_HARNESS_FILES') > + if test_harness_files: > + if len(test_harness_files._strings): > + raise SandboxValidationError( > + 'Cannot install files to the root of TEST_HARNESS_FILES', context) You should never access _ prefixed attributes outside of the class implementation. Let's roll this check into an |if not path| inside the .walk() loop. @@ +518,5 @@ > + objdir_files = {} > + > + for path, strings in test_harness_files.walk(): > + in_srcdir = list(itertools.ifilterfalse(context.is_objdir_path, strings)) > + in_objdir = list(itertools.ifilter(context.is_objdir_path, strings)) itertools is one of those things that is cute, but can be abused. I think ifilterfalse and ifilter constitute abuse because the Python way of accomplishing this pattern is list comprehensions. e.g. in_srcdir = [p for p in strings if not context.is_objdir_path(p)] Swap out "[]" for "{}" to get a set comprehension. @@ +532,5 @@ > + for f in in_objdir: > + if f.startswith('!/'): > + raise SandboxValidationError( > + 'Topobjdir-relative file not allowed in TEST_HARNESS_FILES: %s' % f, context) > + objdir_files[path] = [f[1:] for f in in_objdir] I think all this code would be easier to read if you did everything inside a |for f in strings| loop and populated srcdir_files and objdir_files as you went.

Comment on attachment 8481353 [details] [diff] [review] part 3 - convert testing/mozbase/ to use TEST_HARNESS_FILES Review of attachment 8481353 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Will re-review once it is rebased. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +527,5 @@ > + if '*' in f: > + base = mozpath.dirname(resolved) > + if not os.path.exists(base): > + raise SandboxValidationError( > + 'Pattern listed in TEST_HARNESS_FILES does not exist: %s' % f, context) This assumes * exists at the end of the string. This assumption does not always hold, as foo/**/__init__.py is a valid pattern. Wildcards aren't reliable by definition. I say nix this check from here. If anything, the "check for empty match" code should be inside of the manifest processor (as an optional run-time behavior), likely at https://hg.mozilla.org/mozilla-central/file/5e9826980be5/python/mozbuild/mozpack/manifests.py#l326

Updated with review comments.

Updated with review comments.

For completeness, convert these two directories too. They are much less exciting than mochitest and mozbase, I guess since xpcshell normally runs everything out of the source directory?

Comment on attachment 8481348 [details] [diff] [review] part 1 - add walking functions to HierarchicalStringList Review of attachment 8481348 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +1214,5 @@ > source = mozpath.normpath(os.path.join(obj.srcdir, f)) > dest = mozpath.join(reltarget, mozpath.basename(f)) > install_manifest.add_symlink(source, dest) > > + children = files._children What's the missing dependency in this bug? Because I can find this code nowhere, not even in mercurial.

Comment on attachment 8484397 [details] [diff] [review] part 4 - convert testing/{xpcshell,profiles}/ to use TEST_HARNESS_FILES This patch has been bitrotted by bug 945222.

(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #32) > Comment on attachment 8481348 [details] [diff] [review] > part 1 - add walking functions to HierarchicalStringList > > Review of attachment 8481348 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: python/mozbuild/mozbuild/backend/recursivemake.py > @@ +1214,5 @@ > > source = mozpath.normpath(os.path.join(obj.srcdir, f)) > > dest = mozpath.join(reltarget, mozpath.basename(f)) > > install_manifest.add_symlink(source, dest) > > > > + children = files._children > > What's the missing dependency in this bug? Because I can find this code > nowhere, not even in mercurial. My mistake. This got mixed up with some FINAL_TARGET_FILES stuff that I haven't committed yet.

Having to walk over elements and strings of HierarchicalStringList with an external recursive function is un-Pythonic and adds unnecessary obfuscation to several tasks. Add a walk() function to HierarchicalStringList, modeled on os.walk(), to handle these cases more directly. Now with less patch queue bleed-over.

Comment on attachment 8484397 [details] [diff] [review] part 4 - convert testing/{xpcshell,profiles}/ to use TEST_HARNESS_FILES Review of attachment 8484397 [details] [diff] [review]: ----------------------------------------------------------------- If this has been bitrotted, it's so small as to not worth doing right now.

Comment on attachment 8484394 [details] [diff] [review] part 2 - add TEST_HARNESS_FILES and convert testing/mochitest/ to use it Review of attachment 8484394 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/context.py @@ +152,5 @@ > value = stored_type(value) > > return KeyedDefaultDict.__setitem__(self, key, value) > > + def resolve_path(self, path): glandium wrote some very similar code in bug 1062221. I gave r+ to his patches. But I don't want to delay this awesome patch from landing. Please file a follow-up to unify the path handling code.

Comment on attachment 8485112 [details] [diff] [review] part 1 - add walking functions to HierarchicalStringList glandium is on holiday. Reassigning this so this can land soonish.

pasting from IRC in case you missed it: [18:25:14] <mshal> froydnj: I tried to apply bug 976733, but it seems to be stripping off the last part of the paths now: https://pastebin.mozilla.org/6409086 [18:25:34] <mshal> froydnj: the patch didn't apply cleanly, so I may have hosed something up. Do you get that as well? Or am I missing another patch for it to work? pastebin is: $ diff obj-backup/dom/alarm/backend.mk obj-x86_64-unknown-linux-gnu/dom/alarm/backend.mk 4,6c4,6 < extra_js_modules__FILES := AlarmDB.jsm AlarmService.jsm < extra_js_modules__DEST = $(FINAL_TARGET)/modules/ < INSTALL_TARGETS += extra_js_modules_ --- > extra_js__FILES := AlarmDB.jsm AlarmService.jsm > extra_js__DEST = $(FINAL_TARGET)/ > INSTALL_TARGETS += extra_js_ Lots of other backend files have differences as well. Let me know if it's something I broke importing the patch or am missing!

Having to walk over elements and strings of HierarchicalStringList with an external recursive function is un-Pythonic and adds unnecessary obfuscation to several tasks. Add a walk() function to HierarchicalStringList, modeled on os.walk(), to handle these cases more directly. This version ought to be better, though I haven't run it through full diffs of before/after build files.

Realized some of the tests weren't quite right here, with some of the review comments that have been made. Let's fix those.

(In reply to Michael Shal [:mshal] from comment #39) > Lots of other backend files have differences as well. Let me know if it's > something I broke importing the patch or am missing! You are missing a competent patch author! Updated review in place, ideally better.

Comment on attachment 8487211 [details] [diff] [review] part 1 - add walking functions to HierarchicalStringList >- prefix = 'extra_js_%s' % namespace.replace('/', '_') >+ prefix = 'extra_js_%s' % path.replace('/', '_') > backend_file.write('%s_FILES := %s\n' >- % (prefix, ' '.join(element.get_strings()))) >- backend_file.write('%s_DEST = $(FINAL_TARGET)/%s\n' % (prefix, namespace)) >+ % (prefix, ' '.join(strings))) >+ backend_file.write('%s_DEST = $(FINAL_TARGET)/modules/%s\n' % (prefix, path)) Since the '/modules' part was missing from the previous version of this patch, I'd argue that we need a test or two for for the JavaScriptModules data types to ensure the strings are emitted correctly. r+ assuming test coverage. Although I think you've addressed it now, since this has the potential to be moving files around, please do a test try run (one platform should be fine) in addition to building.

I suppose tests should be reviewed too...

https://hg.mozilla.org/mozilla-central/rev/7960841511fb https://hg.mozilla.org/mozilla-central/rev/6efd021f12bc https://hg.mozilla.org/mozilla-central/rev/28c2dcce4126 https://hg.mozilla.org/mozilla-central/rev/6231d5a29cd0

Did you file a followup for dealing with the test package? Thanks for finishing this!

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #46) > Did you file a followup for dealing with the test package? Thanks for > finishing this! Good point! Bug 1070097.