1254682 - Improve checking of generated FinalTargetFiles

Status: NEW

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

In the emitter, we allow FinalTargetFiles-based variables to install generated files if the current moz.build file specifies those files in either GENERATED_FILES or CONFIGURE_SUBST_FILES. If the file isn't in one of those two variables, the emitter raises a SandboxValidationError(), which is helpful because we fail early if there's a mistake in the moz.build file.

Bug 1242632 extends this to also allow installing files from other directories, which is required to convert a few Makefiles to moz.build files. However, the emit_from_context() function doesn't know the set of files generated by other moz.build files, so we just blindly let any generated files with a '/' in the name through to the backend. This isn't ideal because a mistake in the moz.build file won't be caught until the build actually gets around to trying to install the file.

We might be able to move the generated_files set used internally in emit_from_context() to somewhere more global (maybe a member of TreeMetadataEmitter?). Though if the check is still in emit_from_context(), we have to rely on the moz.build file that generates something to be processed before another moz.build file installs it as a FinalTargetFiles.

If we want/need to be order-independent, I think we'd have to move the check out of emit_from_context() until after all moz.build files are processed.

Or maybe there's a simpler solution? Thoughts?

Comment 1

[Michael Shal [:mshal]]

I know how to click buttons.

Comment 2

[Michael Shal [:mshal]]

Attachment: MozReview Request: Bug 1254682 - Improve checking of FinalTargetFiles; r?gps

For each moz.build file that we process, we can keep track of the
generated files that it depends on, as well as those that it generates.
After all moz.build files are processed, we can validate the inputs to
ensure that some other moz.build file generates them. This allows us to
still fail early if a moz.build file depends on a generated file that
won't actually exist, but also gives us some flexibility to implement
some of the edge case rules that we have in Makefiles.

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

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8733085 [details]
MozReview Request: Bug 1254682 - Improve checking of FinalTargetFiles; r?gps

https://reviewboard.mozilla.org/r/41537/#review37979

I like the approach.

However, what scares me a bit about this is the emitter is making assumptions about output paths. e.g. you are defining the output paths of binaries. In theory, this is the domain of the backend, not the emitter. To do this correctly would require some abstraction. Although that feels like a lot of overhead.

If we go with the current patch, I'd insist on glandium or ted signing off on it.

::: python/mozbuild/mozbuild/frontend/emitter.py:198
(Diff revision 1)
> +
> +    def _check_generated_files(self):
> +        for mozbuild, files in self._generated_inputs.iteritems():
> +            error_files = files.difference(self._generated_outputs)
> +            if error_files:
> +                raise Exception('Objdir file(s) listed in %s not generated by the build system: %s' % (

Why isn't this be a SandboxValidationError?

Comment 4

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #3)
> However, what scares me a bit about this is the emitter is making
> assumptions about output paths. e.g. you are defining the output paths of
> binaries. In theory, this is the domain of the backend, not the emitter. To
> do this correctly would require some abstraction. Although that feels like a
> lot of overhead.

Hmm, I'm not sure how this would be implemented in the emitter without doing a path-level check like this patch does. Can you elaborate on your thoughts there?

It might be possible to move this check to the mozbuild backend files (eg: recursivemake.py), or leave it up to the actual backend itself (make). The latter is essentially what we currently do for things that depend on objdir files not generated by the current mozbuild file, but it's just hacky in that it looks for a '/' in the path and skips the error check, leaving it for make to fail.

This patch is an attempt to make everything fail early, but we could also go with the approach of just leaving everything up to the actual backend by basically removing this check. The disadvantage there for the RecursiveMake backend is that it will fail much later in the build, rather than up-front during config.status. I'm not sure what would be involved in moving it to something like recursivemake.py, but I could give that a try.

> 
> If we go with the current patch, I'd insist on glandium or ted signing off
> on it.

ni'd - what do you guys think?

> 
> ::: python/mozbuild/mozbuild/frontend/emitter.py:198
> (Diff revision 1)
> > +
> > +    def _check_generated_files(self):
> > +        for mozbuild, files in self._generated_inputs.iteritems():
> > +            error_files = files.difference(self._generated_outputs)
> > +            if error_files:
> > +                raise Exception('Objdir file(s) listed in %s not generated by the build system: %s' % (
> 
> Why isn't this be a SandboxValidationError?

Mostly because we don't have the mozbuild context during this check. There is probably a way to get it from the contexts dict we have, though. I'll see if it's easy to change.

Comment 5

[Mike Hommey [:glandium]]

Before looking into this too deeply, I'd like to question the premise. Why do we have install rules from generated files from other directories? Said otherwise: why can't we define the install rules in the same moz.build the generated files are defined? It seems to me it would be more valuable to fix whatever prevents it.

Comment 6

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Before looking into this too deeply, I'd like to question the premise. Why
> do we have install rules from generated files from other directories? Said
> otherwise: why can't we define the install rules in the same moz.build the
> generated files are defined? It seems to me it would be more valuable to fix
> whatever prevents it.

That's a good question. I think the main offender is probably modules/libmar/test/, which installs some nss libraries and ../tool/signmar into an xpcshell test directory.

For js/xpconnect/tests/ we can just add XPIDL_MODULE.xpt to the local generated_files. I think the only other tricky ones are where we preprocess a file into the objdir and also install it somewhere, but I think it could be made to work.

In any case, this doesn't block bug 1252931 like I originally suspected, so I may just revisit it later.

Comment 7

[Michael Shal [:mshal]]

Unassigning for now.