1242632 - Remove TESTING_FILES and TestingFiles from moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

The only use of TESTING_FILES is to provide a base for TESTING_JS_MODULES, but we can just use TEST_HARNESS_FILES for that.

Comment 1

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1242632-Remove-TESTING_FILES-from-moz.build.patch

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8711898 [details] [diff] [review]
0001-Bug-1242632-Remove-TESTING_FILES-from-moz.build.patch

Review of attachment 8711898 [details] [diff] [review]:
-----------------------------------------------------------------

I'd consider this a regression. If anything, things should be moved the other way around, because TEST_HARNESS_FILES requires explicit support in the backend while TESTING_FILES doesn't.

Comment 3

[Michael Shal [:mshal]]

Ahh, I see. I tried to implement it that way instead, but I ran into two issues:

1) A few moz.build files use wildcards in TEST_HARNESS_FILES, which FinalTargetFiles doesn't yet support. Do we want to add support for that in FinalTargetFiles? Or does that open a can of worms for things like EXPORTS += ['**/*.h']? If that's something we want, I'll fix that first.

2) js/xpconnect/tests/idl/moz.build adds '!/dist/bin/components/xpctest.xpt' to TEST_HARNESS_FILES. I don't think it makes sense to put this in GENERATED_FILES, right? Is there some other list of generated files we can check to allow this?

Comment 4

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1242632-Remove-TESTING_FILES-from-moz.build.patch

This removes TESTING_FILES and replaces TEST_HARNESS_FILES with an implementation that uses FinalTargetFiles. I had to add support for wildcards in FinalTargetFiles, which I'm not certain we'll want to do globally.

So far I haven't found a better way to work-around the f.startswith('!/dist/bin/components') check added to emitter.py. I tried to add the .xpt file generated from each XPIDLModule into a global set, but AFAIK that would rely on the order that the moz.build files get parsed. Thoughts?

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8713771 [details] [diff] [review]
0001-Bug-1242632-Remove-TESTING_FILES-from-moz.build.patch

Actually, this breaks xpcshell tests...

Comment 6

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1242632-Remove-TESTING_FILES-from-moz.build.patch

Ahh, it seems this line is required _process_final_target_files():

self._no_skip['export'].add(backend_file.relobjdir)

Otherwise make doesn't recurse into js/xpconnect/tests/idl.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8715047 [details] [diff] [review]
0001-Bug-1242632-Remove-TESTING_FILES-from-moz.build.patch

Review of attachment 8715047 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1241,5 @@
>                  assert not isinstance(f, RenamedSourcePath)
>                  dest = mozpath.join(reltarget, path, f.target_basename)
>                  if not isinstance(f, ObjDirPath):
> +                    if f.is_wildcard:
> +                        install_manifest.add_pattern_symlink(f.srcdir, f, path)

This very much assumes that f is a SourcePath, which not isinstance(f, ObjDirPath) doesn't guarantee. Specifically, the AbsolutePath type would not work here, because f would start with a '%'.

@@ +1250,5 @@
>                      backend_file.write('%s_FILES += %s\n' % (
>                          target_var, self._pretty_path(f, backend_file)))
>                      have_objdir_files = True
>              if have_objdir_files:
> +                self._no_skip['export'].add(backend_file.relobjdir)

ouch, this could be its own bug.

::: python/mozbuild/mozbuild/frontend/context.py
@@ +446,5 @@
>  
> +    @property
> +    def is_wildcard(self):
> +        """Returns True if the path contains a wildcard character ('*'), False if not."""
> +        return self._is_wildcard

meh, I'm pretty sure we have plenty of '*' in instance_of_Path, we can live with a couple more (iow, I don't feel a terrible need for an extra property method just for that).

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +706,5 @@
>                                  % (var, path), context)
>                      else:
> +                        # !/dist/bin/components check is for TEST_HARNESS_FILES
> +                        # in js/xpconnect/tests/idl/moz.build
> +                        if f.target_basename not in generated_files and not f.startswith('!/dist/bin/components'):

You need to ensure the xpt exists before the _tests install manifest is processed, see the comment in js/xpconnect/tests/idl/moz.build.

Comment 8

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #7)
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +1241,5 @@
> >                  assert not isinstance(f, RenamedSourcePath)
> >                  dest = mozpath.join(reltarget, path, f.target_basename)
> >                  if not isinstance(f, ObjDirPath):
> > +                    if f.is_wildcard:
> > +                        install_manifest.add_pattern_symlink(f.srcdir, f, path)
> 
> This very much assumes that f is a SourcePath, which not isinstance(f,
> ObjDirPath) doesn't guarantee. Specifically, the AbsolutePath type would not
> work here, because f would start with a '%'.

Since it doesn't seem AbsolutePaths work with pattern symlinks even when the '%' is accounted for, I've chosen to raise an exception here if it's not a SourcePath.

> @@ +1250,5 @@
> >                      backend_file.write('%s_FILES += %s\n' % (
> >                          target_var, self._pretty_path(f, backend_file)))
> >                      have_objdir_files = True
> >              if have_objdir_files:
> > +                self._no_skip['export'].add(backend_file.relobjdir)
> 
> ouch, this could be its own bug.

This was fixed in bug 1252301.

> ::: python/mozbuild/mozbuild/frontend/context.py
> @@ +446,5 @@
> >  
> > +    @property
> > +    def is_wildcard(self):
> > +        """Returns True if the path contains a wildcard character ('*'), False if not."""
> > +        return self._is_wildcard
> 
> meh, I'm pretty sure we have plenty of '*' in instance_of_Path, we can live
> with a couple more (iow, I don't feel a terrible need for an extra property
> method just for that).

Done.

> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +706,5 @@
> >                                  % (var, path), context)
> >                      else:
> > +                        # !/dist/bin/components check is for TEST_HARNESS_FILES
> > +                        # in js/xpconnect/tests/idl/moz.build
> > +                        if f.target_basename not in generated_files and not f.startswith('!/dist/bin/components'):
> 
> You need to ensure the xpt exists before the _tests install manifest is
> processed, see the comment in js/xpconnect/tests/idl/moz.build.

I believe this will be addressed properly by bug 1253430.

Comment 9

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1242632 - Remove TESTING_FILES from moz.build; r?glandium

Review commit: https://reviewboard.mozilla.org/r/38327/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38327/

Comment 10

[Michael Shal [:mshal]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfa8067ed035

Comment 11

[Michael Shal [:mshal]]

Actually, it seems the check to avoid the SandboxValidationError is too targetted. Other Makefiles, like modules/libmar/tests/Makefile.in, install generated files from elsewhere in the tree into the tests directory. Maybe it's better to just ignore anything with a '/' in the name and assume it came from elsewhere? It won't fail prematurely (during moz.build processing) if the filename is wrong, but it will at least still fail in the build.

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 8726920 [details]
MozReview Request: Bug 1242632 - Remove TESTING_FILES from moz.build; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38327/diff/1-2/

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8726920 [details]
MozReview Request: Bug 1242632 - Remove TESTING_FILES from moz.build; r?glandium

https://reviewboard.mozilla.org/r/38327/#review35281

::: python/mozbuild/mozbuild/backend/recursivemake.py:1246
(Diff revision 2)
> +                            raise Exception("Wildcards are only supported in "
> +                                            "SourcePath objects in %s. Path is: %s" % (
> +                                                type(obj), f
> +                                            ))

The error message here will be weird. e.g.:

"Wildcards are only supported in SourcePath objects in AbsolutePath"

::: python/mozbuild/mozbuild/frontend/emitter.py:866
(Diff revision 2)
> -                        if f.target_basename not in generated_files:
> +                        # The '/' check is to allow installing files generated
> +                        # from other directories, which is done occasionally for
> +                        # tests.
> +                        if f.target_basename not in generated_files and '/' not in f:

I think we should avoid this. Followup?

Comment 14

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #13)
> ::: python/mozbuild/mozbuild/backend/recursivemake.py:1246
> (Diff revision 2)
> > +                            raise Exception("Wildcards are only supported in "
> > +                                            "SourcePath objects in %s. Path is: %s" % (
> > +                                                type(obj), f
> > +                                            ))
> 
> The error message here will be weird. e.g.:
> 
> "Wildcards are only supported in SourcePath objects in AbsolutePath"

It is printing type(obj) (not type(f)) so you have some indication of which variable it's complaining about. Eg, if you have:

TEST_HARNESS_FILES += ['%/home/mshal/foo*',]

you'd see:

Exception: Wildcards are only supported in SourcePath objects in <class 'mozbuild.frontend.data.TestHarnessFiles'>. Path is: %/home/mshal/foo*

> ::: python/mozbuild/mozbuild/frontend/emitter.py:866
> (Diff revision 2)
> > -                        if f.target_basename not in generated_files:
> > +                        # The '/' check is to allow installing files generated
> > +                        # from other directories, which is done occasionally for
> > +                        # tests.
> > +                        if f.target_basename not in generated_files and '/' not in f:
> 
> I think we should avoid this. Followup?

I filed bug 1254682 for this. Suggestions welcome!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb230d696c72

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb230d696c72