[Accepted with Revisions] SDL 0234 - Proxy Library RPC Generation

[theresalech]

Hello SDL community,

The review of "SDL 0234 - Proxy Library RPC Generation" begins now and runs through May 28, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0234-proxy-rpc-generation.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#741

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

• Is the problem being addressed significant enough to warrant a change to SDL?
• Does this proposal fit well with the feel and direction of SDL?
• If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
  Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
[email protected]

[ghost]

First of all I support this proposal. Obviously this proposal is not mentioned in #742 or vice versa but if both proposal are accepted it should be noted that the JavaScript library should be included with the code generator.

1. The spec is not currently housed within the library. Therefore the spec will need to be fed into the script (i.e. the spec should not be auto-downloaded or stored in the library repository but should be an input to the script).

I guess we could have the repo rpc_spec added as a git submodule. This would be handy as it's clear what rpc spec version each library is based of.

2. This proposal is left intentionally ambiguous regarding implementation details of the scripts. I wish to leave implementation details up to the author(s) of the scripts.

I want to note that https://github.com/smartdevicelink/sdl_core/tree/master/tools/InterfaceGenerator exist. So we already have core doing what this proposal is suggesting. Having code generation for Core, iOS and Java we have 3 (in future also JavaScript we have 4) platforms of code generation.

3. The different platform scripts should be very similar outside of the actual code that's generated (e.g. file system code and XML parsing code should be nearly identical).

I believe I don't understand this point. Are you saying the XML parsing code is identical and the code that exports RPC classes is different per platform? I would suggest to have the parsing code to be part of the rpc_spec repository and this repository should be a git submodule. Otherwise we would have identical copies of source files in each repo adding unnecessary maintenance burden.

[joeljfischer]

| 1 / 3. I agree with this and would like to have it as a revision. That way we have an rpc version tagged and can reuse the XML parser and only need to create change the code generator.

[joeygrover]

Just for my own understanding, the idea is to have a separate repo that can generate code for various platforms. Then that code would be added to the projects? Or are you intending this parser be part of each library (or added as a dependency) that builds the RPC classes at compile time? Is this only used when updated RPC specs or every compile? Will the parser also create tests for the RPCs or will we manually write tests?

[joeljfischer]

I understand that the proposal is a bit confusing. The is the current iteration of the idea:

1. The rpc_spec repo will be pulled in as a submodule. The version of the rpc_spec will therefore be pinned.

2. The code to generate each library's RPC classes will be stored within the library github itself. After further investigation, I don't think there's very much code for parsing the XML at all. The majority of the code will be output, not parsing (e.g. the rpc_spec parser is quite short https://github.com/smartdevicelink/rpc_spec/blob/master/RpcParser/xmlParser.py).

3. The code generator will be run manually when we update the RPC specs. If it happens on every compile, it will likely slow down build speeds too much.

4. This version of the proposal does not create any unit tests for RPCs. Those will continue to be handwritten. In the future, we could add another proposal to generate unit tests for the generated RPCs, but generated unit tests for generated RPCs seems unlikely to be especially useful.

[nickschwab]

@joeljfischer

|1/3. Agreed.

|2. I'm concerned that we're stuffing unnecessary things into the libraries (sample code, RPC class generators) that are irrelevant for actual implementations. I think we should strongly consider making this its own repository.

|4. Agreed. Automatically generating unit tests for generated RPCs/classes goes against the principle of unit testing :) Unit tests should be manually created.

[joeljfischer]

|2. To just briefly touch on the sample code thing – that's a common practice in iOS library circles because it allows testing of the library while developing the library through running the example app which builds and links the library code. Every iOS library that has example code that I'm aware of does this. Otherwise you'd have to symlink or use submodules and it just gets much messier.

Regarding the RPC class generators. The main reason for not putting it into its own repository is:

a. Theres little to no code overlap, so it doesn't really gain us anything.

b. If the generator is in a different repository, there's "secret knowledge" about the library that someone needs to know to, for example, update an RPC.

c. If that repository was added as a submodule, it would contain unrelated RPC generators.

d. If the repository is not added as a submodule, then we would need to generate the code, then copy and paste in the new code whenever we want to generate updated code. This is less ideal than simply setting up the script to replace the existing files when the generator is run.

Based on all of these factors, I think it makes much more sense to have the RPC generator in each app library's repository.

EDIT: The only alternative I can think of to not have it in each individual repository would be to have it in the rpc_spec repository, since that already needs to be submoduled into each app library repo for this proposal. It still has the downsides of 2.c., but not 2.a., b, or d.

[nickschwab]

@joeljfischer I can get behind putting the RPC generators in each library's repository for the reasons you mentioned.

--

I'm still against putting sample projects into the library repositories for two main reasons, but my opinion about this should have no impact to this particular proposal. Perhaps this should be decided on a per-library basis to accommodate the best practices of creating modules for each target platform/language.

1. They don't provide real ongoing value to app developers after the library has been integrated into an application.
2. It creates a bloated dependency which developers would have to should "clean up" prior to pushing their app to production, rather than providing a clean plug-and-play solution.

[theresalech]

The Steering Committee voted to approve this proposal with the following revision: there will be parsing code in each repository, and the rpc spec will be a submodule that pins which rpc spec will be used in each version of the library.

[theresalech]

@joeljfischer please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and update issues in the respective repositories for implementation. Thanks!

[theresalech]

Proposal has been updated to include requested revisions, and implementation issues have been entered:
iOS
Java Suite

[theresalech]

Additional impacted platforms:
Java Script - based on creation of JavaScript Library via proposal 0235
RPC Spec - based on accepted revisions to proposal 0234