1471995 - Consider not giving special treatment to the first GENERATED_FILES output

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Michael Shal [:mshal]]

When GENERATED_FILES was first implemented, it only supported one output. The file_generate.py action opened this file for the underlying script as FileAvoidWrite and gave the script the file handle to write to.

Now that GENERATED_FILES supports multiple outputs, it is not really practical to have the file_generate action pass in file handles for all outputs, but it still passes in the file handle for the first output for consistency with the original implementation. This leads to some confusing workarounds, like using a stub file for the first output, and opening subsequent outputs directly.

I think we should remove the special handling of the first output in file_generate.py and the GENERATED_FILES scripts that it invokes. (The stub file from bug 1454912 can still be named from the first output though, as that's an implementation detail for the make backend).

Comment 1

[Nathan Froyd [:froydnj]]

I agree that the extensions GENERATED_FILES has received makes the special-first-file thing a little weird.

Are you proposing:

a) remove the output stream argument entirely; or
b) pass in multiple output streams (via a list or something); or
c) something else?

Comment 2

[Michael Shal [:mshal]]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> I agree that the extensions GENERATED_FILES has received makes the
> special-first-file thing a little weird.
> 
> Are you proposing:
> 
> a) remove the output stream argument entirely; or
> b) pass in multiple output streams (via a list or something); or
> c) something else?

My current thought is to remove the output_file argument from file_generate.py, so that it doesn't do a FileAvoidWrite(args.output_file) and pass in the file handle to the underlying script. I haven't looked at all the GENERATED_FILES invocations in detail yet, so I'm not sure how straightforward this would be.

A list could probably work, but we do have some places where it might be hard to pass down a file handles list to the place that actually writes the files (xpidllex.py/xpidlyacc.py might be tricky, as will the node outputs in bug 1461714, which is what ultimately led me to filing this.)

I'm certainly open to other suggestions or reasons why it won't work.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Michael Shal [:mshal] from comment #2)
> My current thought is to remove the output_file argument from
> file_generate.py, so that it doesn't do a FileAvoidWrite(args.output_file)
> and pass in the file handle to the underlying script. I haven't looked at
> all the GENERATED_FILES invocations in detail yet, so I'm not sure how
> straightforward this would be.

What does "pass in the file handle to the underlying script" mean?  Are you thinking something like /dev/fd/$N on Linux?

> A list could probably work, but we do have some places where it might be
> hard to pass down a file handles list to the place that actually writes the
> files (xpidllex.py/xpidlyacc.py might be tricky, as will the node outputs in
> bug 1461714, which is what ultimately led me to filing this.)
> 
> I'm certainly open to other suggestions or reasons why it won't work.

My thought was that it would be nice to handle the FileAvoidWrite business in file_generate.py, rather than relying on every script to handle that itself.  But maybe that's not a compelling reason, and we should just let scripts handle all the I/O themselves.

Comment 4

[Michael Shal [:mshal]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Michael Shal [:mshal] from comment #2)
> > My current thought is to remove the output_file argument from
> > file_generate.py, so that it doesn't do a FileAvoidWrite(args.output_file)
> > and pass in the file handle to the underlying script. I haven't looked at
> > all the GENERATED_FILES invocations in detail yet, so I'm not sure how
> > straightforward this would be.
> 
> What does "pass in the file handle to the underlying script" mean?  Are you
> thinking something like /dev/fd/$N on Linux?

Ahh my wording here was ambiguous. I mean to not do:

        with FileAvoidWrite(args.output_file, mode='rb') as output:
            ret = module.__dict__[method](output, *args.additional_arguments, **kwargs)

(which does FileAvoidWrite and passes in a file handle)

I didn't mean to suggest "pass in the file handle to the underlying script" as the alternative.

> > A list could probably work, but we do have some places where it might be
> > hard to pass down a file handles list to the place that actually writes the
> > files (xpidllex.py/xpidlyacc.py might be tricky, as will the node outputs in
> > bug 1461714, which is what ultimately led me to filing this.)
> > 
> > I'm certainly open to other suggestions or reasons why it won't work.
> 
> My thought was that it would be nice to handle the FileAvoidWrite business
> in file_generate.py, rather than relying on every script to handle that
> itself.  But maybe that's not a compelling reason, and we should just let
> scripts handle all the I/O themselves.

Yeah, I think what I'm suggesting is to do this in file_generate.py:

ret = module.__dict__[method](*args.additional_arguments, **kwargs)

And rely on the script to write to the correct outputs.

If that results in too much duplication between listing the outputs in the moz.build file and the script, we could try to expand the "output_file" argument to be a list. This would still let the underlying script do the file IO to accommodate things like the node integration, where files are written via javascript instead of python.

Comment 5

[Michael Shal [:mshal]]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1461714#c60 and the node.stub entry in GeneratedFile.required_for_compile