1686265 - FOG only really needs two GeneratedFile directives

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task, P1)

Description

[Chris H-C :chutten]

So it turns out that GeneratedFile in moz.build can take multiple names (a word which here means files we are generating). We can tidy up moz.build to only have one GeneratedFile for metrics and another for pings, and do away with the whole nonsense of having to have three to call each of rust, cpp, and js in order : |

In other words, instead of

GeneratedFile('metrics.rs', ...)
GeneratedFile('GleanMetrics.h', ...)
GeneratedFile('GleanJSMetricsLookup.h', ...)

(each having the same ...)
We could have:

GeneratedFile('metrics.rs', 'GleanMetrics.h', 'GleanJSMetricsLookup.h', ...)

Now, in the single name case the output we're given is a fd-alike (a FileAvoidWrite to be precise) to the specific name we're generating. In the multi-name case I'm not sure that we are given fd-likes for each of these in order or what, so there's some possibility we might need to create the FileAvoidWrites ourselves a la make-stl-wrappers.py after finding our output dir in the objdir.

Even if we have to do that ourselves, this'll still be a nice tidying up as we'll only have one entry point, and run_glean_parser.py will be free to run rust, cpp, and js in whatever order it wants. It also frees us up to split the deps from bug 1686140 if we want to avoid rebuilding pings headers when metrics change (and vice versa) which could be beneficial as we gradually increase the number of people who have to include them.

Comment 1

[Mitchell Hentges [:mhentges] 🦀]

Hmm, it looks like the first argument to a GeneratedFile entrypoint is always the first name's associated file.
E.g.:

GeneratedFile(
    "GleanMetrics.h",
    "GleanFoo.h",
    "GleanBar.h",
    script="build_scripts/glean_parser_ext/run_glean_parser.py",
    entry_point="cpp_metrics",
    ...
)
# ---
def cpp_metrics(output_fd, input...):
    # output_fd is "toolkit/components/glean/GleanMetrics.h"

Also note that os.environ["OBJDIR"] points to the objdir, so combining the Glean generation scripts into one GeneratedFile could be viable.

Comment 2

[Chris H-C :chutten]

Also note that fixing this makes it so that glinter nits no longer print three times each.

Comment 3

[Chris H-C :chutten]

Attachment: Bug 1686265 - Simplify FOG GeneratedFile statements r?janerik!,mhentges!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a6ee5271b80
Simplify FOG GeneratedFile statements r=janerik,mhentges

Comment 5

[Alexandru Michis [:malexandru]]

Backed out changeset 0a6ee5271b80 (Bug 1686265) for causing bustages in GleanJSPingsLookup.h
Backout link: https://hg.mozilla.org/integration/autoland/rev/bcffc84bd8a3eef6ae20930a836aee420a1fc5d6
Push with failures, failure log.

Comment 6

[Chris H-C :chutten]

Hm. Maybe we do need all those GeneratedFile directives after all.

ni?mhentges,glandium - Am I misunderstanding how to use GeneratedFile with multiple names? I thought each name in the list would trigger the file generation, but it seems as though if it's not the first item in the names list, the build can get lost.

Comment 7

[Mitchell Hentges [:mhentges] 🦀]

I'll take a closer look at this in a bit, but just curious - are you able to repro the issue locally?

Comment 8

[Chris H-C :chutten]

I wasn't during my testing, but maybe I never tested the landing code with a clobber. Lemme try.

...annnd it fails locally. Well poo, I should've caught this before review. Looks like it's wanting to copy GleanJSPingsLookup.h over to dist but it doesn't know how to make it. Which is weird because OBJDIR/faster/Makefile has a rule

$(TOPOBJDIR)/toolkit/components/glean/GleanJSPingsLookup.h: $(TOPOBJDIR)/toolkit/components/glean/api/src/$(MDDEPDIR)/pings.rs.stub ;

OBJDIR/backend.mk is silent on it, though. (But I don't even know what I'm looking for)

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Chris H-C :chutten from comment #6)

Hm. Maybe we do need all those GeneratedFile directives after all.

ni?mhentges,glandium - Am I misunderstanding how to use GeneratedFile with multiple names? I thought each name in the list would trigger the file generation, but it seems as though if it's not the first item in the names list, the build can get lost.

I think you are confused, because this is confusing. The first file name is special, generally a stub name, like in this example. I think there are other examples in the tree that are more clear, perhaps the Node example.

I can help with this as needed.

Comment 10

[Chris H-C :chutten]

(In reply to Nick Alexander :nalexander [he/him] from comment #9)

(In reply to Chris H-C :chutten from comment #6)

Hm. Maybe we do need all those GeneratedFile directives after all.

ni?mhentges,glandium - Am I misunderstanding how to use GeneratedFile with multiple names? I thought each name in the list would trigger the file generation, but it seems as though if it's not the first item in the names list, the build can get lost.

I think you are confused, because this is confusing. The first file name is special, generally a stub name, like in this example. I think there are other examples in the tree that are more clear, perhaps the Node example.

I can help with this as needed.

In this particular case the first name is just fine, it's the others that make doesn't know how to, uh, make.

I did some backend searchfoxing to find out some of the ways the names list is different between first and "all the rest" names, but I must've missed the part where it doesn't tell make how to build these files.

If I have to have one GeneratedFile per header for mozbuild to understand things, then I guess the duplication is just something we get to live with (this bug standing as documentation for why). I was hoping for something a little prettier, but codegen is difficult and no doubt my case is a weird new corner that I can be thankful is supported at the small cost of repetition.

Comment 11

[Chris H-C :chutten]

Looks like we'll have to keep it like this. Ah well.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1686265 - Don't re-parse the same glean files multiple times, r=chutten!

This reduces the number of times we re-parse the same glean file, and means we
only use 2 GeneratedFiles directives to generate the relevant files for Glean.

This will also make errors generated by the validator only print once.

The problem in the original patch appears to be caused by the use of the rust
file ('api/src/metrics.rs'), as the first argument to the GeneratedFile. This
messed with the location of things like generated deps files for the build
script, and the path of the stub file, which were placed alongside the
metrics.rs file, and not in the directory the build system was expecting.

This issue appears to be fixed if the arguments are re-ordered, such that a
header file appears first instead.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a308cf1af5a
Don't re-parse the same glean files multiple times, r=chutten

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4a308cf1af5a