Closed Bug 1274518 Opened 8 years ago Closed 7 years ago

Add IPDL preprocessing support

Categories

(Core :: IPC, defect, P3)

Product:
Core
Component:
IPC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: gerard-majax, Assigned: seinlin)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-backlog)

Attachments

(1 file, 1 obsolete file)

Bug 1274518 - Allow to use preprocessor in IPDL files.
59 bytes, text/x-review-board-request
mshal
: review+
Details 
As reported in several bugs (1273998, 1159320), there seems to be no way to perform preprocessing on IPDL.

In bug 1273998, this is leading to "--disable-printing" being broken.
In bug 1159320, this is leading to "--disable-webrtc" being broken also.

As suggested by :glandium, it would be interesting to know if this is because there is something making this completely impossible or is it just that nobody implemented it.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(benjamin)
What do you mean by "preprocessing"? I think it's pretty clear that we don't want to support something like a C preprocessor on these files. The build cost of doing that is high.

I'm not convinced that we should support this at all (and we should probably remove a bunch of --disable-feature flags while we're at it). But if we do, it should be represented as a part of the file syntax itself.
Flags: needinfo?(benjamin)
Preprocessing as in the good old python preprocessor (in python/mozbuild/mozbuild/preprocessor.py, nowadays). The cost of preprocessing is really not high. Also consider this: *.idl files and *.webidl files *are* preprocessed.
It would also be nice to be able to #ifdef stuff like this for only building in a DEBUG/test build:

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#505
It would certainly be nicer if we added conditional support to the IPDL parser instead. But that could takes some time. Using the Python preprocessor seems okay to me for now, assuming someone is willing to do the work.
Flags: needinfo?(wmccloskey)
Whiteboard: btpp-backlog
Blocks: 1283135
OS: Gonk (Firefox OS) → All
Priority: -- → P3
Hardware: ARM → All
Summary: Preprocessing of IPDL files → Add IPDL preprocessing support
Assignee: nobody → seinlin.maung
When a IPDL file need to be preprocessed, just need to use PREPROCESSED_IPDL_SOURCES instead of IPDL_SOURCES.

eg:
PREPROCESSED_IPDL_SOURCES += [
   'IPDL file need to be preprocessed',
]
Comment on attachment 8928600 [details] [diff] [review]
Allow to use preprocessor in IPDL files.

:froydnj, Would you mind review this patch? Thanks!
Attachment #8928600 - Flags: review?(nfroyd)
Comment on attachment 8928600 [details] [diff] [review]
Allow to use preprocessor in IPDL files.

Obsoleted, as failed on try server.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e460b0fcc4591cbcf0c1b0877ac102313073047
Attachment #8928600 - Attachment is obsolete: true
Attachment #8928600 - Flags: review?(nfroyd)
One thing to keep in mind here is that this will break SearchFox's handling of IPDL files (at least foe files that use it), because that relies on a different IPDL parser. Maybe SearchFox can be updated to run on the preprocessed output.
To be clear, I don't think a lack of SearchFox support should block landing of this. I filed a bug for adding SearchFox support.
Comment on attachment 8928628 [details]
Bug 1274518 - Allow to use preprocessor in IPDL files.

https://reviewboard.mozilla.org/r/199864/#review205708

Thanks for this change!  Just one comment below, and then I'm going to reassign the review to the general build peer review queue.  Somebody else should be along shortly to review the patch.

Do you know if we have a concrete use case that would immediately use these changes?

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:660
(Diff revision 1)
>          topsrcdir = env.topsrcdir.replace(os.sep, '/')
>  
>          expected = [
>              "ALL_IPDLSRCS := %s/bar/bar.ipdl %s/bar/bar2.ipdlh %s/foo/foo.ipdl %s/foo/foo2.ipdlh" % tuple([topsrcdir] * 4),
>              "CPPSRCS := UnifiedProtocols0.cpp",
> -            "IPDLDIRS := %s/bar %s/foo" % (topsrcdir, topsrcdir),
> +            "IPDLDIRS := %s/ipc/ipdl %s/bar %s/foo" % (env.topobjdir, topsrcdir, topsrcdir),

We should have a test that contains `PREPROCESSED_IPDL_SOURCES` and ensures that the right things happen in processing `PREPROCESSED_IPDL_SOURCES`.
Attachment #8928628 - Flags: review?(nfroyd) → review?(core-build-config-reviews)
(In reply to :froydnj (on leave until 2018, ni? or email if necessary) from comment #12)
> Comment on attachment 8928628 [details]
> Bug 1274518 - Allow to use preprocessor in IPDL files.
> 
> https://reviewboard.mozilla.org/r/199864/#review205708
> 
> Thanks for this change!  Just one comment below, and then I'm going to
> reassign the review to the general build peer review queue.  Somebody else
> should be along shortly to review the patch.

:froydnj, Thanks for the review and feedback.
 
> Do you know if we have a concrete use case that would immediately use these
> changes?

Now, there is no urgent case needs to use this change, but I think this requirement does exist for a period of time. Once it is landed, feature owner can easily to have a build flag to enable/disable some feature as needed.
 
> ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:660
> 
> We should have a test that contains `PREPROCESSED_IPDL_SOURCES` and ensures
> that the right things happen in processing `PREPROCESSED_IPDL_SOURCES`.

Test case includes PREPROCESSED_IPDL_SOURCES is also updated to the patch.
Attachment #8928628 - Flags: review?(core-build-config-reviews) → review?(mshal)
Comment on attachment 8928628 [details]
Bug 1274518 - Allow to use preprocessor in IPDL files.

https://reviewboard.mozilla.org/r/199864/#review206590

This is looking pretty close! I think the main thing I'd like to see is to clean up the interface between the CommonBackend and RecursiveMakeBackend, and I think just adding a separate argument for the preprocessed sources instead of reaching into self._ipdls will do the trick.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1566
(Diff revision 2)
>                               unified_ipdl_cppsrcs_mapping):
>          # Write out a master list of all IPDL source files.
>          mk = Makefile()
>  
> -        mk.add_statement('ALL_IPDLSRCS := %s' % ' '.join(sorted_ipdl_sources))
> +        sorted_nonstatic_ipdl_files = list(sorted(self._ipdls.all_preprocessed_basenames()))
> +        mk.add_statement('nonstatic_ipdl_files := %s' % ' '.join(sorted_nonstatic_ipdl_files))

The nonstatic_ipdl_files does not appear to be used anywhere in the Makefile. I think this line can just be removed.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1568
(Diff revision 2)
>          mk = Makefile()
>  
> -        mk.add_statement('ALL_IPDLSRCS := %s' % ' '.join(sorted_ipdl_sources))
> +        sorted_nonstatic_ipdl_files = list(sorted(self._ipdls.all_preprocessed_basenames()))
> +        mk.add_statement('nonstatic_ipdl_files := %s' % ' '.join(sorted_nonstatic_ipdl_files))
> +
> +        for source in sorted(self._ipdls.all_preprocessed_sources()):

I think we should probably just add an argument to _handle_ipdl_sources for sorted_preprocessed_sources. Then we could avoid accessing self._ipdls in this method and just go based off the list of regular sources and the list of preprocessed sources.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1578
(Diff revision 2)
> +                '$(RM) $@',
> +                '$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) '
> +                    '$< -o $@)'
> +            ])
> +
> +        # Preprocessed ipdl files still need to be in the correct order.

Why do you need to do this ordering? I believe we just sort the inputs in order to get reproducible builds, but they would be just as reproducible if we do ALL_IPDLSRCS := %s %s % (sorted_ipdl_sources, sorted_preprocessed_ipdl_sources), and that would avoid the weird in-place replacement loop.

::: python/mozbuild/mozbuild/test/frontend/data/ipdl_sources/bar/moz.build:7
(Diff revision 2)
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +PREPROCESSED_IPDL_SOURCES += [

It looks like there is no corresponding frontend test for this. Were you intending to add one?

::: python/mozbuild/mozbuild/test/frontend/data/ipdl_sources/foo/moz.build:7
(Diff revision 2)
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +PREPROCESSED_IPDL_SOURCES += [

Similar here - no frontend test.
Attachment #8928628 - Flags: review?(mshal)
(In reply to Michael Shal [:mshal] from comment #15)

:mshal, Thanks for review. Comments are addressed and patch is also updated.

> ::: python/mozbuild/mozbuild/backend/recursivemake.py:1566
> 
> The nonstatic_ipdl_files does not appear to be used anywhere in the
> Makefile. I think this line can just be removed.

Removed.

> ::: python/mozbuild/mozbuild/backend/recursivemake.py:1568
> (Diff revision 2)
> 
> I think we should probably just add an argument to _handle_ipdl_sources for
> sorted_preprocessed_sources. Then we could avoid accessing self._ipdls in
> this method and just go based off the list of regular sources and the list
> of preprocessed sources.

Addressed as suggested.

> ::: python/mozbuild/mozbuild/backend/recursivemake.py:1578
> Why do you need to do this ordering? I believe we just sort the inputs in
> order to get reproducible builds, but they would be just as reproducible if
> we do ALL_IPDLSRCS := %s %s % (sorted_ipdl_sources,
> sorted_preprocessed_ipdl_sources), and that would avoid the weird in-place
> replacement loop.

ALL_IPDLSRCS passed to ipdl.py need to be in origin order, 
If the preprocessed one is out of order will cause error in ipdl.py.

> ::: python/mozbuild/mozbuild/test/frontend/data/ipdl_sources/bar/moz.build:7
> > +PREPROCESSED_IPDL_SOURCES += [
> 
> It looks like there is no corresponding frontend test for this. Were you
> intending to add one?
>

Test case is added.
Comment on attachment 8928628 [details]
Bug 1274518 - Allow to use preprocessor in IPDL files.

https://reviewboard.mozilla.org/r/199864/#review208544

::: python/mozbuild/mozbuild/backend/recursivemake.py:1560
(Diff revision 3)
>      def _handle_linked_rust_crates(self, obj, extern_crate_file):
>          backend_file = self._get_backend_file_for(obj)
>  
>          backend_file.write('RS_STATICLIB_CRATE_SRC := %s\n' % extern_crate_file)
>  
> -    def _handle_ipdl_sources(self, ipdl_dir, sorted_ipdl_sources,
> +    def _handle_ipdl_sources(self, ipdl_dir, sorted_ipdl_sources, sorted_nonstatic_ipdl_sources,

The compilation/database.py and backend/tup.py files also have _handle_ipdl_sources implementations, so they'll need the new argument as well. I don't think you need to update the body of those functions, but they should be called with the right arguments.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1567
(Diff revision 3)
>          # Write out a master list of all IPDL source files.
>          mk = Makefile()
>  
> +        for source in sorted_nonstatic_ipdl_sources:
> +            rule = mk.create_rule([source])
> +            rule.add_dependencies([source, '$(GLOBAL_DEPS)'])

Since 'source' is both the target and dependency, it gets pruned by makeutil.py:dependencies(). This means there's no first dependency in the preprocessor rule, so $< evaluates to nothing and the preprocessor invocation fails.

I think you probably want sorted_nonstatic_ipdl_sources to have the full path to the files, and then do mk.create_rule([basename of the path]) and rule.add_dependencies([full path to source, ...])

::: python/mozbuild/mozbuild/backend/recursivemake.py:1567
(Diff revision 3)
>          # Write out a master list of all IPDL source files.
>          mk = Makefile()
>  
> +        for source in sorted_nonstatic_ipdl_sources:
> +            rule = mk.create_rule([source])
> +            rule.add_dependencies([source, '$(GLOBAL_DEPS)'])

$(GLOBAL_DEPS) is defined in config.mk/rules.mk, but those are included after ipdlsrcs.mk. At the point where $(GLOBAL_DEPS) is evaluated in the Makefile, it is expanded immediately, and results in an empty string. I'm not sure if it will work, but you might try moving the include of ipdlsrcs.mk after the one for rules.mk in ipc/ipdl/Makefile.in

::: python/mozbuild/mozbuild/backend/recursivemake.py:1576
(Diff revision 3)
> +                    '$< -o $@)'
> +            ])
> +
> +        # Preprocessed ipdl files still need to be in the correct order.
> +        # Replace the preprocessed file into sorted_ipdl_sources in same order.
> +        for idx, ipdl in enumerate(sorted_ipdl_sources):

What error did you get from ipdl.py if these aren't sorted? I gave it a try and it seemed to work fine, though I had to make sure the preprocessed files weren't also listed in sorted_ipdl_sources. If possible, I'd still like to remove this for loop.

::: python/mozbuild/mozbuild/frontend/context.py:1742
(Diff revision 3)
>          """),
>  
> +    'PREPROCESSED_IPDL_SOURCES': (StrictOrderingOnAppendList, list,
> +        """Preprocessed IPDL source files.
> +
> +        These are ``.ipdl`` files that will be parsed and converted to

This description should mention that the files are preprocessed before being parsed & converted into .cpp files.
Attachment #8928628 - Flags: review?(core-build-config-reviews)
:mshal, Thanks for the review and comments. All comment are addressed. Would you mind review again?
(In reply to Kai-Zhen Li [:seinlin][:kli] from comment #21)
> :mshal, Thanks for the review and comments. All comment are addressed. Would
> you mind review again?

Sure! I should be able to get to it tomorrow.
Attachment #8928628 - Flags: review?(core-build-config-reviews)
Attachment #8928628 - Flags: review?(mshal)
Comment on attachment 8928628 [details]
Bug 1274518 - Allow to use preprocessor in IPDL files.

https://reviewboard.mozilla.org/r/199864/#review210294

Looks good! Just one nit to fix before landing, and if you don't mind filing the followup bug I'd appreciate it.

::: python/mozbuild/mozbuild/backend/common.py:192
(Diff revision 4)
> +        return self.sources
> +
> +    def all_preprocessed_sources(self):
> +        return self.preprocessed_sources
> +
> +    def all_preprocessed_basenames(self):

I think this function is now unused - you can just remove it.

::: python/mozbuild/mozbuild/backend/common.py:332
(Diff revision 4)
>              self._handle_idl_manager(self._idl_manager)
>              self._handle_generated_sources(mozpath.join(self.environment.topobjdir, 'dist/include/%s.h' % idl['root']) for idl in self._idl_manager.idls.values())
>  
>          self._handle_webidl_collection(self._webidls)
>  
> -        sorted_ipdl_sources = list(sorted(self._ipdl_sources))
> +        sorted_ipdl_sources = list(sorted(self._ipdls.all_sources()))

It doesn't really make sense to pass in the full list of sources, as well as the preprocessed & non-preprocessed ones. Since it look like only the tup backend is using sorted_ipdl_sources now, can you file a followup bug for this? Feel free to assign it to me since I'm actively working on that backend.
Attachment #8928628 - Flags: review?(mshal) → review+
Blocks: 1422682
(In reply to Michael Shal [:mshal] from comment #23)
> It doesn't really make sense to pass in the full list of sources, as well as
> the preprocessed & non-preprocessed ones. Since it look like only the tup
> backend is using sorted_ipdl_sources now, can you file a followup bug for
> this? Feel free to assign it to me since I'm actively working on that
> backend.

:mshal, Thanks for review. nit was addressed and follow up bug is filed, bug 1422682.
Attachment #8928628 - Flags: checkin+
Keywords: checkin-needed
In case of ipdl source need to be preprocessed, just need to use "PREPROCESSED_IPDL_SOURCES" in moz.build instead of "IPDL_SOURCES".
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Autoland can't push this until all pending issues in MozReview are marked as
> resolved.

:RayanVM, Thanks! I resoled pending in mozReview.
Comment on attachment 8928628 [details]
Bug 1274518 - Allow to use preprocessor in IPDL files.

https://reviewboard.mozilla.org/r/199864/#review211092

nits was addressed, carry r+
Attachment #8928628 - Flags: review+
Attachment #8928628 - Flags: review+
Attachment #8928628 - Flags: checkin+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cd86cbfcd98f
Allow to use preprocessor in IPDL files. r=mshal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cd86cbfcd98f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Thanks for taking care of this bug and landing that :)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: