Closed Bug 1242632 Opened 4 years ago Closed 4 years ago

Remove TESTING_FILES and TestingFiles from moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

Details

Attachments

(2 files, 2 obsolete files)

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 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.
Attachment #8711898 - Flags: review?(gps) → review-
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?
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?
Attachment #8711898 - Attachment is obsolete: true
Attachment #8713771 - Flags: review?(mh+mozilla)
Comment on attachment 8713771 [details] [diff] [review]
0001-Bug-1242632-Remove-TESTING_FILES-from-moz.build.patch

Actually, this breaks xpcshell tests...
Attachment #8713771 - Flags: review?(mh+mozilla)
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.
Attachment #8713771 - Attachment is obsolete: true
Attachment #8715047 - Flags: review?(mh+mozilla)
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.
Attachment #8715047 - Flags: review?(mh+mozilla)
Depends on: 1252301
Depends on: 1253430
(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.
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 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 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?
Attachment #8726920 - Flags: review?(mh+mozilla) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/bb230d696c72
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1259174
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.