Closed Bug 976733 Opened 10 years ago Closed 10 years ago

Add TEST_HARNESS_FILES support to moz.build

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: ted, Assigned: ted)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 9 obsolete files)

part 3 - convert testing/mozbase/ to use TEST_HARNESS_FILES
7.39 KB, patch
gps
: review+
Details | Diff | Splinter Review
part 4 - convert testing/{xpcshell,profiles}/ to use TEST_HARNESS_FILES
2.93 KB, patch
Details | Diff | Splinter Review
part 1 - add walking functions to HierarchicalStringList
22.63 KB, patch
mshal
: review+
Details | Diff | Splinter Review
part 2 - add TEST_HARNESS_FILES and convert testing/mochitest/ to use it
24.84 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
part 1a - add test for EXTRA_JS{_PP,}_MODULES output
4.78 KB, patch
mshal
: review+
Details | Diff | Splinter Review 
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?
Flags: needinfo?(ted)
(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.
Flags: needinfo?(ted)
(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.
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
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).
Flags: needinfo?(ted)
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.
Attachment #8473819 - Flags: feedback?(ted)
Attachment #8473819 - Flags: feedback?(gps)
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.
Attachment #8381649 - Attachment is obsolete: true
Attachment #8473820 - Flags: feedback?(ted)
Attachment #8473820 - Flags: feedback?(gps)
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.
Attachment #8473819 - Flags: feedback?(gps) → feedback+
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.
Attachment #8473820 - Flags: feedback?(gps) → feedback+
(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.
Flags: needinfo?(gps)
(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
Flags: needinfo?(gps)
(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.
Flags: needinfo?(gps)
(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.
Flags: needinfo?(gps)
Blocks: 1059740
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.
Attachment #8473814 - Attachment is obsolete: true
Attachment #8481348 - Flags: review?(mh+mozilla)
Attachment #8481348 - Flags: feedback?(gps)
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.
Attachment #8473819 - Attachment is obsolete: true
Attachment #8473819 - Flags: feedback?(ted)
Attachment #8481351 - Flags: review?(gps)
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.
Attachment #8473820 - Attachment is obsolete: true
Attachment #8473820 - Flags: feedback?(ted)
Attachment #8481353 - Flags: review?(gps)
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.
Attachment #8481348 - Flags: feedback?(gps) → feedback+
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.
Attachment #8481351 - Flags: review?(gps) → feedback+
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
Attachment #8481353 - Flags: review?(gps) → feedback+
Updated with review comments.
Attachment #8481351 - Attachment is obsolete: true
Attachment #8484394 - Flags: review?(gps)
Updated with review comments.
Attachment #8481353 - Attachment is obsolete: true
Attachment #8484395 - Flags: review?(gps)
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?
Attachment #8484397 - Flags: review?(gps)
Blocks: 991983
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.
Attachment #8481348 - Attachment is obsolete: true
Attachment #8481348 - Flags: review?(mh+mozilla)
Attachment #8485112 - Flags: review?(mh+mozilla)
Attachment #8485112 - Flags: feedback+
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.
Attachment #8484397 - Flags: review?(gps)
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.
Attachment #8484394 - Flags: review?(gps) → review+
Attachment #8484395 - Flags: review?(gps) → review+
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.
Attachment #8485112 - Flags: review?(mh+mozilla) → review?(mshal)
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!
Flags: needinfo?(nfroyd)
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.
Attachment #8485112 - Attachment is obsolete: true
Attachment #8485112 - Flags: review?(mshal)
Attachment #8487211 - Flags: review?(mshal)
Realized some of the tests weren't quite right here, with some of the review
comments that have been made.  Let's fix those.
Attachment #8484394 - Attachment is obsolete: true
Attachment #8487213 - Flags: review+
(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.
Flags: needinfo?(nfroyd)
Blocks: 1065434
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.
Attachment #8487211 - Flags: review?(mshal) → review+
I suppose tests should be reviewed too...
Attachment #8489500 - Flags: review?(mshal)
Attachment #8489500 - Flags: review?(mshal) → review+
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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: