1476973 - a few cleanups to xpidl support

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

I wrote these patches as part of trying to make generated XPIDL files work.  I
think they're useful on their own, and we might as well put them in the tree.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make XPIDL_SOURCES a list of SourcePath objects

PIDL_SOURCES would benefit from being more explicit about what sort of
values it contains; let's define it as a list of SourcePath objects, and
propagate those objects into XPIDLFile frontend objects as well.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - assert that XPIDL_SOURCES files all exist

We should have been checking for this from the beginning.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - rationalize the backend's IDLManager

The IDLManager in the moz.build backend is a bit weird.  It maintains a
bunch of per-IDL file information, some of which isn't used, and
attempts to map module names to information about what entities the
module needs to be built.  The former is done with Python dicts, and the
latter with Python tuples, both resulting in some contortions by the
clients of IDLManager to specify exactly what they need.

Let's clean this up, by making IDLManager to more clearly do two jobs:

1. Keep track of whether IDL files are globally unique; and
2. Map module names to the information needed to build them.

In the case of #2, we store everything as a straight Python object, so
we can use actual property accesses everywhere.  We also provide a
stems() function on IDLManager to make some client code more
straightforward.

Doing this makes IDLManager much more XPIDL module-centric, and paves
the way for the same change to be made in the frontend as well.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - emit XPIDLModule, rather than multiple XPIDLFile

PIDL files are logically grouped together into a module, but the
current model for the moz.build frontend is that we emit individual
XPIDLFile objects, and leave it to the backend to reconstruct
module-ness from those.  This setup causes a small amout of useless work
to be done (e.g. see XPIDLFile handling in
RecursiveMakeBackend.consume_object; such handling should only be done
once), and it would be cleaner to have the objects emitted reflect the
build system concepts as closely as possible.  To that end, let's emit
XPIDLModule objects instead, which fortunately requires relatively few
changes.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 5 - make xpidl variables simply expanded

These kind of variables are slightly more efficient in make.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8993377 [details] [diff] [review]
part 1 - make XPIDL_SOURCES a list of SourcePath objects

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

::: python/mozbuild/mozbuild/frontend/context.py
@@ +1748,5 @@
>          see :ref:`jar_manifests`.
>          """),
>  
>      # IDL Generation.
> +    'XPIDL_SOURCES': (ContextDerivedTypedList(SourcePath,StrictOrderingOnAppendList), list,

Nit: this wants a space after a comma.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8993379 [details] [diff] [review]
part 3 - rationalize the backend's IDLManager

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

Nice refactor!

I left a few comments. But they can be ignored and don't block landing.

::: python/mozbuild/mozbuild/backend/common.py
@@ +69,5 @@
> +            self._stems.update(mozpath.splitext(mozpath.basename(idl))[0]
> +                               for idl in idls)
> +
> +        def stems(self):
> +            return iter(self._stems)

Nit: why return an iterator here? Are you trying to prevent modification of the original object? You could also `return set(self._stems)` to return an independent copy.

@@ +97,5 @@
> +        """Return an iterator of stems of the managed IDL files.
> +
> +        The stem of an IDL file is the basename of the file with no .idl extension.
> +        """
> +        return itertools.chain(*[m.stems() for m in self.modules.itervalues()])

This might want to be sorted so behavior is deterministic. Although I guess a lot (all?) consumers already sprinkle sorted() around everything XPIDL. So maybe it isn't worth it.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8993380 [details] [diff] [review]
part 4 - emit XPIDLModule, rather than multiple XPIDLFile

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

Since you are poking around XPIDL, there is tons of historical baggage you should know about. You may want to see bugs 1267337, 1266998, and 1266995 for future potential optimizations. I have no plans to finish those patches. By all means pick them up and run with them if you want! I'm particularly fond of deriving the module name from the source path, eliminating XPIDL_MODULE, and auto-populating .xpt references when they are pulled into the build so they don't need to be synchronized in package-manifest.in. These were all things we couldn't realistically do before moz.build files existed.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8993381 [details] [diff] [review]
part 5 - make xpidl variables simply expanded

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

Yup. `=` should generally be `:=` in make unless you absolutely care about lazy evaluation of nested expressions.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Gregory Szorc [:gps] from comment #7)
> ::: python/mozbuild/mozbuild/backend/common.py
> @@ +69,5 @@
> > +            self._stems.update(mozpath.splitext(mozpath.basename(idl))[0]
> > +                               for idl in idls)
> > +
> > +        def stems(self):
> > +            return iter(self._stems)
> 
> Nit: why return an iterator here? Are you trying to prevent modification of
> the original object? You could also `return set(self._stems)` to return an
> independent copy.

I was trying to prevent modification here, I think, but iterators are what the callers want anyway.  I guess ideally, `self._stems` would be turned into a `frozenset` at some point, and we wouldn't have to care about modification.

> @@ +97,5 @@
> > +        """Return an iterator of stems of the managed IDL files.
> > +
> > +        The stem of an IDL file is the basename of the file with no .idl extension.
> > +        """
> > +        return itertools.chain(*[m.stems() for m in self.modules.itervalues()])
> 
> This might want to be sorted so behavior is deterministic. Although I guess
> a lot (all?) consumers already sprinkle sorted() around everything XPIDL. So
> maybe it isn't worth it.

I think I looked at all the callers here and everybody called `sorted()`.  It would be nice to make the contracts a little stronger so we could call `sorted()` in the callee...

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Gregory Szorc [:gps] from comment #8)
> Since you are poking around XPIDL, there is tons of historical baggage you
> should know about. You may want to see bugs 1267337, 1266998, and 1266995
> for future potential optimizations. I have no plans to finish those patches.
> By all means pick them up and run with them if you want! I'm particularly
> fond of deriving the module name from the source path, eliminating
> XPIDL_MODULE, and auto-populating .xpt references when they are pulled into
> the build so they don't need to be synchronized in package-manifest.in.
> These were all things we couldn't realistically do before moz.build files
> existed.

Nice, I had patches for eliminating XPIDL_MODULE but I didn't feel like they were really justified yet.

Also note that we no longer package .xpt files, though we do generate them as a convenient intermediate format for other things.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6a7a63abc0
part 1 - make XPIDL_SOURCES a list of SourcePath objects; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/08370aca03f5
part 2 - assert that XPIDL_SOURCES files all exist; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/24bcc39a55f2
part 3 - rationalize the backend's IDLManager; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e5a88621d1
part 4 - emit XPIDLModule, rather than multiple XPIDLFile; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59a3967c1e4
part 5 - make xpidl variables simply expanded; r=gps

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for "build-linux64-tup/opt" bustages.

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d89e0d0f3b16250d643d6a1ff5a8b3d09fc55b8
Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e59a3967c1e48ed155805fb4e6d3c7a11f778985
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=189879958&repo=mozilla-inbound&lineNumber=1437

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Narcis Beleuzu [:NarcisB] from comment #13)
> Backed out for "build-linux64-tup/opt" bustages.
> 
> Backout link:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 6d89e0d0f3b16250d643d6a1ff5a8b3d09fc55b8
> Push link:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=e59a3967c1e48ed155805fb4e6d3c7a11f778985
> Log link:
> https://treeherder.mozilla.org/logviewer.html#?job_id=189879958&repo=mozilla-
> inbound&lineNumber=1437

build-linux64-tup/opt is a tier2 job, so I assumed failures there would not provoke a backout...

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1843ad25a8a
part 1 - make XPIDL_SOURCES a list of SourcePath objects; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/a15b3186d0bb
part 2 - assert that XPIDL_SOURCES files all exist; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0ab5e5a871
part 3 - rationalize the backend's IDLManager; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4400e353e30
part 4 - emit XPIDLModule, rather than multiple XPIDLFile; r=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d4bff67989
part 5 - make xpidl variables simply expanded; r=gps

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d1843ad25a8a
https://hg.mozilla.org/mozilla-central/rev/a15b3186d0bb
https://hg.mozilla.org/mozilla-central/rev/5a0ab5e5a871
https://hg.mozilla.org/mozilla-central/rev/c4400e353e30
https://hg.mozilla.org/mozilla-central/rev/b9d4bff67989