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

ASSIGNED
Assigned to

Status

()

Core
Plug-ins
P2
major
ASSIGNED
7 months ago
2 months ago

People

(Reporter: louis, Assigned: louis, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pdfium-release])

Attachments

(4 attachments, 11 obsolete attachments)

1.80 KB, patch
louis
: review?
peterv
Details | Diff | Splinter Review
3.01 KB, patch
louis
: review+
Details | Diff | Splinter Review
2.26 MB, patch
louis
: review+
Details | Diff | Splinter Review
1.33 KB, patch
Details | Diff | Splinter Review
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.

Updated

7 months ago
Blocks: 1264551

Updated

7 months ago
Whiteboard: [pdfium-release]
(Assignee)

Updated

7 months ago
No longer blocks: 1309444, 1323120
No longer depends on: 1313295
(Assignee)

Updated

7 months ago
Assignee: nobody → lochang
Status: NEW → ASSIGNED
Summary: [jsplugins][Discussion] Generate rpc.cc at build time instead of checking it into m-c → [jsplugins] Integrate building rpc.cc into mozbuild instead of checking it into m-c
(Assignee)

Comment 1

7 months ago
Created attachment 8854789 [details] [diff] [review]
[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
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 2

7 months ago
Created attachment 8854790 [details] [diff] [review]
[WIP] Bug 1347440 part 2 - Modify generator files to integrate building rpc.cc into mozbuild

The patch includes the modifications to generator files.
You need to add the generated source:

SOURCES += [ '!outrpc.cc' ]
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

7 months ago
Attachment #8854790 - Attachment description: Bug 1347440 part 2 - Modify generator files to integrate building rpc.cc into mozbuild → [WIP] Bug 1347440 part 2 - Modify generator files to integrate building rpc.cc into mozbuild
(Assignee)

Comment 4

7 months ago
Created attachment 8855222 [details] [diff] [review]
[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.
Attachment #8854789 - Attachment is obsolete: true
(Assignee)

Comment 5

7 months ago
Test the patches on tryserver.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=712efec30e3eab4b16cf4fae8298c9d245beaf77
(Assignee)

Comment 6

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

Comment 7

7 months ago
Created attachment 8855659 [details] [diff] [review]
[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
(Assignee)

Updated

7 months ago
Attachment #8855659 - Attachment description: Bug 1347440 part 3 - Allow compiler warnings and ignore bad implicit conversion CTOR → [WIP] Bug 1347440 part 3 - Allow compiler warnings and ignore bad implicit conversion CTOR
(Assignee)

Comment 8

7 months ago
Created attachment 8856832 [details] [diff] [review]
[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
(Assignee)

Updated

7 months ago
Attachment #8856832 - Attachment description: Bug 1347440 part 4 - Include mozglue and avoid redefinition problems on Windows → [WIP] Bug 1347440 part 4 - Include mozglue and avoid redefinition problems on Windows
(Assignee)

Comment 9

7 months ago
Created attachment 8857347 [details] [diff] [review]
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.
Attachment #8855222 - Attachment is obsolete: true
Attachment #8857347 - Flags: review?(mh+mozilla)
Created attachment 8857349 [details] [diff] [review]
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"
Attachment #8854790 - Attachment is obsolete: true
Attachment #8857349 - Flags: review?(peterv)
Created attachment 8857405 [details] [diff] [review]
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.
Attachment #8855659 - Attachment is obsolete: true
Attachment #8857405 - Flags: review?(peterv)
Created attachment 8857406 [details] [diff] [review]
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.
Attachment #8856832 - Attachment is obsolete: true
Attachment #8857406 - Flags: review?(peterv)
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?
Attachment #8857347 - Flags: review?(mh+mozilla)
(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?
Flags: needinfo?(mh+mozilla)

Comment 15

6 months ago
Created attachment 8859876 [details] [diff] [review]
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.
Created attachment 8860332 [details] [diff] [review]
Bug 1347440 part 2 - Modify generator files for generating outrpc.cc by mozbuild

Update patch based on Bruce suggestion.
Attachment #8857349 - Attachment is obsolete: true
Attachment #8857349 - Flags: review?(peterv)
Attachment #8860332 - Flags: review?(peterv)
Created attachment 8860333 [details] [diff] [review]
Bug 1347440 part 3 - Fix compile and link errors of building rpc dynamic libaray

Update patch based on Bruce suggestion.
Attachment #8857405 - Attachment is obsolete: true
Attachment #8857405 - Flags: review?(peterv)
Attachment #8860333 - Flags: review?(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

:-(
Attachment #8860332 - Flags: review?(peterv) → review+
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?
Attachment #8857406 - Flags: review?(peterv) → review+
(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?
Flags: needinfo?(mh+mozilla)
Created attachment 8866201 [details] [diff] [review]
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+.
Attachment #8859876 - Attachment is obsolete: true
Attachment #8860332 - Attachment is obsolete: true
Attachment #8866201 - Flags: review+
Created attachment 8866205 [details] [diff] [review]
Bug 1347440 part 4 - Remove Makefile and rpc.cc in mortar. r=peterv

Carry over r+.
Attachment #8857406 - Attachment is obsolete: true
Attachment #8866205 - Flags: review+
(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
(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?
Flags: needinfo?(peterv)
Flags: needinfo?(mh+mozilla)
(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?
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.
Flags: needinfo?(mh+mozilla)
Created attachment 8868830 [details] [diff] [review]
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.
Attachment #8857347 - Attachment is obsolete: true

Updated

4 months ago
Severity: normal → major
Priority: -- → P2
Summary: [jsplugins] Integrate building rpc.cc into mozbuild instead of checking it into m-c → [Mortar] Integrate building rpc.cc into mozbuild instead of checking it into m-c
You need to log in before you can comment on or make changes to this bug.