1428775 - Make it possible to generate IDL files

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Patch showing the issue

Currently it doesn't seem possible to generate an IDL file via a script. I've attached a patch with a dummy script that "generates" the nsICrashReporter.idl file (to keep things simple in this case the file is simply copied over). Running ./mach build with the patch applied yields the following output:

 0:00.27 Adding make options from /home/gsvelto/projects/mozilla-central/.mozconfig
    AUTOCLOBBER=1
    MOZ_OBJDIR=/home/gsvelto/projects/build/firefox
    OBJDIR=/home/gsvelto/projects/build/firefox
    FOUND_MOZCONFIG=/home/gsvelto/projects/mozilla-central/.mozconfig
    export FOUND_MOZCONFIG
 0:00.27 /usr/bin/gmake -f client.mk -s
 0:00.58 Elapsed: 0.00s; From dist/public: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.59 Elapsed: 0.00s; From dist/xpi-stage: Kept 14 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.59 Elapsed: 0.00s; From dist/private: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.61 Traceback (most recent call last):
 0:00.61   File "/usr/lib64/python2.7/runpy.py", line 174, in _run_module_as_main
 0:00.61     "__main__", fname, loader, pkg_name)
 0:00.61   File "/usr/lib64/python2.7/runpy.py", line 72, in _run_code
 0:00.61     exec code in run_globals
 0:00.61   File "/home/gsvelto/projects/mozilla-central/python/mozbuild/mozbuild/action/process_install_manifest.py", line 113, in <module>
 0:00.61     main(sys.argv[1:])
 0:00.61   File "/home/gsvelto/projects/mozilla-central/python/mozbuild/mozbuild/action/process_install_manifest.py", line 100, in main
 0:00.61     defines=args.defines)
 0:00.61   File "/home/gsvelto/projects/mozilla-central/python/mozbuild/mozbuild/action/process_install_manifest.py", line 69, in process_manifest
 0:00.61     remove_empty_directories=remove_empty_directories)
 0:00.61   File "/home/gsvelto/projects/mozilla-central/python/mozbuild/mozpack/copier.py", line 431, in copy
 0:00.61     copy_results.append((destfile, f.copy(destfile, skip_if_older)))
 0:00.61   File "/home/gsvelto/projects/mozilla-central/python/mozbuild/mozpack/files.py", line 336, in copy
 0:00.61     raise ErrorMessage('Symlink target path does not exist: %s' % self.path)
 0:00.61 mozpack.errors.ErrorMessage: Symlink target path does not exist: /home/gsvelto/projects/mozilla-central/xpcom/system/!nsICrashReporter.idl
 0:00.61 gmake[3]: *** [Makefile:178: install-dist/idl] Error 1
 0:00.61 gmake[3]: *** Waiting for unfinished jobs....
 0:00.73 Elapsed: 0.14s; From _tests: Kept 1046 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:00.84 Elapsed: 0.25s; From dist/bin: Kept 2163 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.03 Elapsed: 0.45s; From dist/include: Kept 5454 existing; Added/updated 0; Removed 0 files and 0 directories.
 0:01.03 gmake[2]: *** [/home/gsvelto/projects/mozilla-central/config/recurse.mk:33: pre-export] Error 2
 0:01.03 gmake[1]: *** [/home/gsvelto/projects/mozilla-central/config/rules.mk:434: default] Error 2
 0:01.03 gmake: *** [client.mk:168: build] Error 2
 0:01.04 359 compiler warnings present.
 0:01.06 ccache (direct) hit rate: 0.0%; (preprocessed) hit rate: 0.0%; miss rate: 100.0%

Comment 1

[Nathan Froyd [:froydnj]]

OK, so a rough outline of steps for things to do here:

1. We need some list of the IDL files that are getting generated.  There are two ways to go about this:

a. Have a GENERATED_XPIDL_SOURCES mozbuild variable, similar to the existing XPIDL_SOURCES variable:

http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#1762

b. Extend XPIDL_SOURCES to understand that !path.idl names are generated sources.

I don't have strong opinions on the correct way forward here; option b) is probably slightly less work.  Option a) is consistent with what we do for e.g. WebIDL, whereas option b) is consistent with what we do for exported headers.

Assuming option b), we'd change XPIDL_SOURCES from a StrictOrderingOnAppendList to a ContextDerivedTypedList(Path, StrictOrderingOnAppendList).  That will magically start to parse !path.idl names.  (See the details in python/mozbuild/mozbuild/frontend/data.py:Path and subclasses if you are interested.)

2. Changing things in #1 will mean that we have to change the way TreeMetadataEmitter._process_xpidl works:

http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#1247

since after step #1, it's not processing strings that have to be converted into absolute paths when constructing XPIDLFile objects, but objects that already know their path.  I think we want to make XPIDLFile objects hold those path-ish objects, so we have to touch:

a. _process_xpidl itself;
b. XPIDLFile.__init__; and
c. anything that touches XPIDLFile.

2.c is probably the biggest thing, leading to...

3. XPIDLFile objects are processed in the common backend:

http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/common.py#190

which will require touching XPIDLManager.register_idl.  We'll also need to modify any implementations of _handle_idl_manager in all the build backends.

4. For the recursivemake backend.

http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py

_handle_idl_manager is at:

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#1041

We would need to make sure the generated IDL file(s) get installed in the correct place and correctly listed in the Makefile rules.  $OBJDIR/config/makefiles/xpidl/Makefile has the code generated from this method; ideally it should be reasonably obvious where you want to insert references to the generated IDL.

We probably need some sort of Makefile rule to indicate that the XPIDL processing from config/makefiles/xpidl/ depends on the generated XPIDL files.  That is tricky, but once we have that and all of the above, everything should Just Work.

4a. We'd need to modify the tup backend and the compilation database backend for these generated files as well.  Ideally, we can get mshal or chmanchester to help on the tup backend.  We might be able to punt on making things work with the compilation database backend?

5. Tests for all of this would be most excellent.  There are probably some existing tests that can serve as a useful skeleton.

I have probably forgotten a step or two along the way.

Does that make everything clearer, more daunting, or some combination of both?

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Nathan would you have time to fix this or know something who could? I have limited experience modifying our build system so I can try following your instructions... however it's probably going to take me a lot longer than it would to somebody more familiar with it.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

I've had a look at what needs to be done trying to follow the steps in comment 1 and the process does indeed seem rather daunting. While I can probably pull it off if given enough time I'd really appreciate if someone who already knows the build system internals could take this instead.

Comment 4

[Nathan Froyd [:froydnj]]

Yes, I can take a look at this.  Apologies for taking so long for it to float to the top of the queue.

Comment 5

[Nathan Froyd [:froydnj]]

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

The next several patches show off what I have so far.  All of this builds on
bug 1459721, which made at least the definition process surprisingly easy.
Unfortunately, there are some significant roadblocks ahead...

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 2 - emit objdir paths for generated XPIDL_SOURCES

Thanks to the magic of Path objects, we weren't actually emitting objdir
paths for generated XPIDL_SOURCES before this.  This commit changes
that, and ensures that such files need to live in GENERATED_FILES as well.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add tests for recursivemake backend and generated XPIDL files

Just to wave our hands in the direction of ensuring that we're emitting all the
right information to the generated makefiles.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 4 - adjust XPIDL lex cache directory when testing

The XPIDL build process selects a cache directory for various artifacts
from the Python script, such as the generated ply lexer and parser.  The
chosen directory during normal builds lives under xpcom, which is
normally not a problem.  During test builds, however, xpcom/ does not
exist in the objdir, and the XPIDL build process complains.

Instead of trying to hack the test builds to make xpcom/ exist, it's
easier to simply choose the current directory during test builds.  We
have various precedents for `ifdef TEST_MOZBUILD` or similar stanzas,
and this change feels fully in line with existing code.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 5 - test building generated IDL files; r=build-peer XXX

Tests are green with this patch, but that greenness is completely bogus:

* .xpt files with generated IDLs in them need to depend on the generated IDL,
  see below;

* The test objdir doesn't actually recurse into the idl/ directory.  No code
  is generated for it, AFAICT.

* The XPIDL build process needs to have cross-directory dependencies to
  anywhere that might generate IDL files.

* The XPIDL build process, AFAICT, runs during the *pre-export* tier, while our
  generated files run during the export tier, i.e. after pre-export.  So that's
  a bit of a circular dependency...

* There are probably some other issues that haven't been unearthed yet because
  of the above blockers.

It might be easier to just have a generated-files tier that generated
everything, then run pre-export, and then run export.  That would solve the
ordering problems, at the cost of not having explicit dependencies between
everything.  But that could hurt parallelism, depending.

Comment 10

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

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.