1347440 - [Mortar] Integrate building rpc.cc into mozbuild instead of checking it into m-c

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, enhancement, P2)

Description

[Louis Chang [:louis]]

In order to build out rpc dynamic library for mortar project, we need rpc.cc file which is now manually built out from the old Makefile and checked into m-c. However, we should remove the Makefile once the mortar is released. Also, to avoid updating rpc.cc file every time while the PPAPI interface is modified, we should somehow build out the rpc.cc file automatically in the build time. Or find some other ways to get rid of these problems.

Comment 1

[Louis Chang [:louis]]

Attachment: [WIP] Bug 1347440 part 1 - Integrate building rpc.cc into mozbuild

Hi Mike,

I have a question with mozbuild could you help me out? Thanks.

I am now trying to build out a 'outrpc.cc' file by running a script file 'idl_gen_rpc.py' with mozbuild. And I have done the modification as the attached patch. But mozbuild generates the file in 'misc' time (after compile) instead of export time (before compile). It is weird that the result conflicts with the command's description [1]. And this will cause the compile error since we need to build a 'librpc.dylib' based on the host/rpc.cc and outrpc.cc.

Note:
I can separately build out outrpc.cc with mozbuild. Then build out librpc.dylib based on outrpc.cc generated in first build.

[1] http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#993-999

Comment 2

[Louis Chang [:louis]]

Attachment: [WIP] Bug 1347440 part 2 - Modify generator files to integrate building rpc.cc into mozbuild

The patch includes the modifications to generator files.

Comment 3

[Mike Hommey [:glandium]]

You need to add the generated source:

SOURCES += [ '!outrpc.cc' ]

Comment 4

[Louis Chang [:louis]]

Attachment: [WIP] Bug 1347440 part 1 - Integrate building rpc.cc into mozbuild

Updated patch based on comment 3.

Thanks Mike, I can now locally generate outrpc.cc and build out librpc.dylib in one time.

Comment 5

[Louis Chang [:louis]]

Test the patches on tryserver.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=712efec30e3eab4b16cf4fae8298c9d245beaf77

Comment 6

[Louis Chang [:louis]]

Test the patches on tryserver with allowing compilers warning.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f310f74b9c8f85cde25a35990c08aba9d9e3ca4f

Comment 7

[Louis Chang [:louis]]

Attachment: [WIP] Bug 1347440 part 3 - Allow compiler warnings and ignore bad implicit conversion CTOR

Test on try server with the attached patch which allow compiler warnings and also ignore bad implicit conversion constructor. It seems that only on Winodws platform will build failed now. I will keep looking into it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=004f8af6b54e0ac4b5ef15276b7eef0b7c81ea67&selectedJob=89426601

Comment 8

[Louis Chang [:louis]]

Attachment: [WIP] Bug 1347440 part 4 - Include mozglue and avoid redefinition problems on Windows

With the patch, we can pass the try server on Windows as well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a828b04d56e28fc717577b1c370bee8574157ba6

Comment 9

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 1 - Generate outrpc.cc with mozbuild

Hi Mike,

Could you review the patch for me? Thanks.
We suppose to generate outrpc.cc with moz.build by executing idl_gen_rpc.py script. The patch contains the related modifications.

Comment 10

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 2 - Modify generator files for generating outrpc.cc by mozbuild

Hi Peter,

Could you review the patch for me? Thanks.

The patch contains the modifications of generators files so we are able to leverage mozbuild to generate outrpc.cc.

One thing to mention in the patch is that mozbuild will somehow execute the script under objxxx/browser/extensions/mortar so I have to change the srcroot of working directory to "../../../../browser/extensions/mortar/ppapi/api"

Comment 11

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 3 - Fix compile and link errors of building rpc dynamic libaray

Hi Peter,

Could you review the patch for me? Thanks.

The patch fixes the compile errors such as bad implicit conversion constructor and repeated typedef on Windows 64bits (one in rpc.h another in outrpc.cc). Also to keep third-party code updated from upstream we allow the compilers warnings. And we have to include mozglue on Windows 64bits as well to fix the link error.

Comment 12

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 4 - Remove Makefile and rpc.cc in mortar

Hi Peter,

Could you review the patch for me? Thanks.

Based on the part 1-3 patches, we can successfully generate outrpc.cc and build out rpc dynamic library with mozbuild. So I guess we are able to remove rpc.cc and Makefile now.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8857347 [details] [diff] [review]
Bug 1347440 part 1 - Generate outrpc.cc with mozbuild

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

::: browser/extensions/mortar/moz.build
@@ +6,4 @@
>  
>  XPCSHELL_TESTS_MANIFESTS += ['test/unit/xpcshell.ini']
>  
> +EXPORTS += [

I'm pretty sure you want this to be SOURCES, not EXPORTS.

@@ +13,2 @@
>  SOURCES += [
>      'host/rpc.cc',

Doesn't outrpc.cc replace this? Why not call outrpc.cc rpc.cc, btw?

Comment 14

[Louis Chang [:louis]]

(In reply to Mike Hommey [:glandium] (VAC: Apr 20-May 4) from comment #13)
> Comment on attachment 8857347 [details] [diff] [review]
> Bug 1347440 part 1 - Generate outrpc.cc with mozbuild
> 
> Review of attachment 8857347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/mortar/moz.build
> @@ +6,4 @@
> >  
> >  XPCSHELL_TESTS_MANIFESTS += ['test/unit/xpcshell.ini']
> >  
> > +EXPORTS += [
> 
> I'm pretty sure you want this to be SOURCES, not EXPORTS.
> 
> @@ +13,2 @@
> >  SOURCES += [
> >      'host/rpc.cc',
> 
> Doesn't outrpc.cc replace this? Why not call outrpc.cc rpc.cc, btw?

Sorry for the confusing naming, but the rpc.cc we are going to replace is ppapi/out/rpc.cc not host/rpc.cc. In addition, host/rpc.cc is written by us and it includes "outrpc.cc". So that's why I only add host/rpc.cc to SOURCES (will include outrpc.cc) and add outrpc.cc to EXPORTS.
If I only add outrpc.cc to SOURCES, codes in host/rpc.cc may missed and the shared library we built out might be wrong, right?

Comment 15

[Bruce Sun [:brsun]]

Attachment: remove_hardcode_path.patch

Hi Louis,

I based on your patches and do a little modification, and it seems rpc.so still can be built without hardcoding the path in scripts on my Ubuntu machine. Hope it helps.

Comment 16

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 2 - Modify generator files for generating outrpc.cc by mozbuild

Update patch based on Bruce suggestion.

Comment 17

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 3 - Fix compile and link errors of building rpc dynamic libaray

Update patch based on Bruce suggestion.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 8860332 [details] [diff] [review]
Bug 1347440 part 2 - Modify generator files for generating outrpc.cc by mozbuild

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

::: browser/extensions/mortar/host/rpc.cc
@@ +2,4 @@
>   * 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/. */
>  
> +#include "outrpc.cc"

We need a better name. ppapi_rpc.cc?

::: browser/extensions/mortar/ppapi/generators/idl_gen_rpc.py
@@ +622,5 @@
>      outfile.Write('/* ' + include.GetName() + ' */')
>      GenerateHeader(outfile, include, releases)
>  
> +def main(outfile):
> +  outfile.Write = outfile.write

:-(

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 8860333 [details] [diff] [review]
Bug 1347440 part 3 - Fix compile and link errors of building rpc dynamic libaray

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

::: browser/extensions/mortar/moz.build
@@ +32,4 @@
>  rpc = GENERATED_FILES['outrpc.cc']
>  rpc.script = 'ppapi/generators/idl_gen_rpc.py'
>  
> +GeckoSharedLibrary('rpc')

Why do we need to change this and what does it mean?

Comment 20

[Mike Hommey [:glandium]]

(In reply to Louis Chang [:louis] from comment #14)
> (In reply to Mike Hommey [:glandium] (VAC: Apr 20-May 4) from comment #13)
> > Comment on attachment 8857347 [details] [diff] [review]
> > Bug 1347440 part 1 - Generate outrpc.cc with mozbuild
> > 
> > Review of attachment 8857347 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/extensions/mortar/moz.build
> > @@ +6,4 @@
> > >  
> > >  XPCSHELL_TESTS_MANIFESTS += ['test/unit/xpcshell.ini']
> > >  
> > > +EXPORTS += [
> > 
> > I'm pretty sure you want this to be SOURCES, not EXPORTS.
> > 
> > @@ +13,2 @@
> > >  SOURCES += [
> > >      'host/rpc.cc',
> > 
> > Doesn't outrpc.cc replace this? Why not call outrpc.cc rpc.cc, btw?
> 
> Sorry for the confusing naming, but the rpc.cc we are going to replace is
> ppapi/out/rpc.cc not host/rpc.cc. In addition, host/rpc.cc is written by us
> and it includes "outrpc.cc". So that's why I only add host/rpc.cc to SOURCES
> (will include outrpc.cc) and add outrpc.cc to EXPORTS.

EXPORTS is the wrong way to do it. But why do you need host/rpc.cc including outrpc.cc in the first place?

Comment 21

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 2 - Modify generator files for generating ppapi_rpc.cc by mozbuild. r=peterv

Rename outrpc.cc to ppapi_rpc.cc.
Carry over r+.

Comment 22

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 4 - Remove Makefile and rpc.cc in mortar. r=peterv

Carry over r+.

Comment 23

[Louis Chang [:louis]]

(In reply to Peter Van der Beken [:peterv] from comment #19)
> ::: browser/extensions/mortar/moz.build
> @@ +32,4 @@
> >  rpc = GENERATED_FILES['outrpc.cc']
> >  rpc.script = 'ppapi/generators/idl_gen_rpc.py'
> >  
> > +GeckoSharedLibrary('rpc')
> 
> Why do we need to change this and what does it mean?

The command will build rpc with mozglue library and we need the some functions such as xmalloc in the library on Windows.

Try server error:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86b988d6222ba3ce2f4b8a307dc1c68a8fce9641&selectedJob=97934493

Comment 24

[Louis Chang [:louis]]

(In reply to Mike Hommey [:glandium] from comment #20)
> EXPORTS is the wrong way to do it. But why do you need host/rpc.cc including
> outrpc.cc in the first place?

Basically, host/rpc.cc will need some typedef and functions in generated ppapi_rpc.cc to compile. And it might be much more complicated to separate them clearly. So I guess that's why we originally include it directly. Maybe Peter have more information about this issue?

Comment 25

[Louis Chang [:louis]]

(In reply to Louis Chang [:louis] from comment #24)
> Basically, host/rpc.cc will need some typedef and functions in generated
> ppapi_rpc.cc to compile. And it might be much more complicated to separate
> them clearly. So I guess that's why we originally include it directly. Maybe
> Peter have more information about this issue?

Or maybe we can change the name of ppapi_rpc.cc to ppapi_rpc.h if it is acceptable to you?

Comment 26

[Mike Hommey [:glandium]]

Then you need something like this in a Makefile.in:

rpc.$(OBJ_SUFFIX): ppapi_rpc.cc

There is no reliable moz.build way to do this.

Comment 27

[Louis Chang [:louis]]

Attachment: Bug 1347440 part 1 - Generate ppapi_rpc.cc with mozbuild

Fixed the patch based on glandium's suggestion.
But I will send the review request until we make sure whether rpc and pdfium should build alone as dynamic libraries or build with libxul.

Comment 28

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm closing this bug as WONTFIX per:

"The Mortar experiment has concluded. Mozilla does not consider the PDF use case justifies the burden of implementing and maintaining PDFium and a Pepper API implementation in Gecko."

Source: https://wiki.mozilla.org/Mortar_Project