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

[theresalech]

Hello SDL community,

The review of "SDL-0234 Proxy Library RPC Generation - Revision" begins now and runs through November 19, 2019. A previous review of this revision occurred October 30 - November 5, 2019.

This will be a review of proposed revisions to a previously accepted but not yet implemented proposal, SDL 0234.

The pull request outlining the revisions under review is available here:

#849

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

#853

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
theresa@livio.io

[ghost]

There was an offline discussion between the PM and SDLC members regarding the RPC spec. Because the PR was marked "review ready" I wanted to wait for the review status before suggesting revisions:

1. The proposal should include "rpc" as well as JavaScript, iOS and Java Suite as an impacted platform.
2. the rpc_spec repository should include an XSD file that defines the MOBILE_API XML file and relationship of the XML elements. A PR already exist for the rpc_spec repository which includes a reverse engineered XSD file (see #203 SDL-0234 Proxy Library RPC Generation rpc_spec#202).
3. The parser code that parses the MOBILE_API should use the XSD file to verify the integrity of the XML.

[vladmu]

Hello SDL community,

We would also suggest extending point 2 of the Implementation Notes

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. However, the command-line switches should be identical between implementations.

by adding the following list of command-line switches:

• <input-file> - required, should point to MOBILE_API.xml
• <output-directory> - required, define the place where the generated output should be placed
• [<regex-pattern>] - optional, only elements matched with defined regex pattern will be parsed and generated, if present
• --help, -h - print the help information and exit
• --version, -v - print the version and exit
• --verbose - display additional details like logs etc.
• --enums, -e, --structs, -s, --functions, -f - only specified elements will be generated, if present
• --overwrite, -y - force overwriting of existing files in the output directory, ignore confirmation message
• --skip, -n - skip overwriting of existing files in the output directory, ignore confirmation message

It is good to have the specified list here as the point suggests it should be identical between implementations

[joeljfischer]

Just a few notes:

1. Do we want to specify how these scripts (the generator script and the parser script) interact? Does the generator script call the parser script and depend on them being in a certain location in the file system.

2. When you note the downside:

Also, when changing the submodule reference to a new commit of rpc_spec it can be possible that the parser scripts and the generator scripts are not compatible anymore. Breaking changes could appear. However, the same issue is applicable to the current agreed status where the parsing scripts is not compatible with the mobile API file structure.

Do we want to version the scripts such that the generator script requires the same or lesser version of parser script? I can see issues with this if the parser script needs to fix a bug (and becomes, e.g. 1.0.1) and the generator script stays at 1.0.0. Perhaps as long as the generator script is the same or greater major version, it should be parsable? This requires some level of backward compatibility. We could also say that they have to be the same major version.

[joeygrover]

3. One of the changes included is to use Core's python code as a base versus what is in the RPC spec repo already. If this is to be accepted I would ask that the code taken from the Core repo be refactored to be much more readable and have better documentation around it. The current state is hard to follow by someone just picking it up and makes for a tough time to continue maintenance when each of our major projects will now depend on it. Also I think a generator script should be created for the README.MD for the RPC spec repo as well so that we can use a single parser for all output instead of maintaining multiple. The output should obviously match what is there now at a minimum, but improvements welcomed. All of this would have been necessary for using the RPC spec python code as well (cleaner code, better docs, etc), just want to point it out.

4. Because each project will be dependant on this new setup, will Core be refactored to accept this parser rather than having it built in separately?

[ghost]

@joeljfischer

1. It's somewhat a development detail but it makes sense to specify the interface between parser and generator. So: Yes. The generator scripts are dependent on the parser scripts. The generator script should call the parser scripts.
2. I am afraid of inventing versioning to the scripting API (and technically there will be some sort of ... API offered by the parser and used by the generator) but it'll help keep parser and generators aligned. I like your suggestion We could also say that they have to be the same major version. If the parser changes the parsing output it requires a major version bump. Generators can compare their own version with the parsers version. The major version must match otherwise an error is returned. Minor version change may be overkill and not needed here. We want the generators to be up-to-date with the parser. Do you agree?

@joeygrover
3. a I also have a hard time to read the python scripts of SDL Core. A major refactor makes sense. We can either specify the structure right now or we keep this as development detail. Suggesting parser and generator should have one file per type (enum, struct, rpc). A script file specific for "param" related functions makes sense and should be imported in the struct and rpc scripts.
3. b Makes sense to have generator scripts in rpc_spec as well to export a README.MD. Isn't the current script doing exactly this? Anyways, we can state that rpc_spec should contain a generator script that generates the README.MD file using the parser scripts of the rpc_spec
4. Good point. In this case Core is also an impacted platform and the proposal should state that the scripts in sdl_core should be updated to include rpc_spec as a submodule and use the new script structure

[joeljfischer]

2. I agree with that. Minor version shouldn't matter in this case; we will just need to watch for breaking version changes in the parser -> generator API and update the version accordingly.

[vladmu]

Based on this comment of @theresalech smartdevicelink/sdl_javascript_suite#2 (comment) we also suggest to include the Python version into the proposal.

@kshala-ford @joeljfischer

2.. Let me add some thoughts regarding the versioning. Based on a Git documentation, the project includes a submodule by tracking the particular commit at the moment of adding the submodule. And this will always follow the same commit until someone updates the submodule and push the new reference into the project. So this avoids inconsistency between the parser and the generator until the maintainer will decide to update the submodule is required. And at the time of this decision, it is the responsibility of the maintainer to guarantee the generator works correctly with the parser in the submodule. No needs to match versions of the generator and parser in that case. In other words, in my opinion, this downside has the same importance as just pushing bugs into existing code and should be filtered via PR approval and the testing processes.

4.. Small question regarding Core support, If this parser will impact Core, is that mean this parser should include and support the extended functionality which already exists in the InterfaceGenerator, like 3 versions of the XML format (JSONRPC, SDLRPCv1, and SDLRPCv2) and existing SmartFactory for generators? The current version of MOBILE_API.xml works with SDLRPCv2 of the existing parser in Core, and because generators separated into particular repositories, they don't require SmartFactory right now.

[theresalech]

@vladmu the Steering Committee voted to return this proposal for revisions on 2019-11-05. I will be posting the full rationale shortly. Your comments will need to be discussed when the revised proposal is brought back into review.

[theresalech]

The Steering Committee voted to return this proposal for revisions. The author will make the following changes to the proposal: specify that the generator scripts are dependent on the parser scripts, and the generator script should call the parser scripts; require that the generator and parser be the same major version; specify that the python code taken from the Core repo be refactored to be more readable and have better documentation around it; state that rpc_spec should contain a generator script that generates the README.MD file using the parser scripts of the rpc_spec; include Core as an impacted platform, and state that the scripts in sdl_core should be updated to include rpc_spec as a submodule and use the new script structure. The biggest reason the proposal was returned for revisions instead of accepted with revisions is to make sure reviewers have the opportunity to consider the implications of Core now being an impacted platform.

[theresalech]

The author has made the Steering Committee-requested changes to the pull request, and it is now ready for re-review: #849. The Steering Committee will vote on this PR during the 2019-11-19 meeting.

[joeljfischer]

The updated proposal is a 👍 from me

[ghost]

Thank you Joel.

While the proposal was returned for revisions I was asked to add the following:

Python (is) defined as a strict requirement in the proposal for the generator script, but the version is not set. Because Python 2 support ends in 2020, in our opinion, version 3 is preferable.

To follow the SDLC process I have not added this item while the proposal was returned as we have not agreed to this revision. Now, back in review I would suggest to add the following item to the proposal:

13. Python 2 support ends in 2020. Therfore, all python scripts should be developed for Python 3. This includes scripts from the InterfaceBuilder that will be refactored.

I think this requirement makes sense and shouldn't be a big change to the proposal. My recommendation is to accept the proposal with the revision to add item 13. as described above.

[theresalech]

The Steering Committee voted to accept this proposal with the revision of developing python scripts for Python 3 instead of Python 2. While it was determined this wouldn't require a proposal revision, there was also discussion in the Steering Committee meeting about unit tests for this implementation within the JavaScript Suite Library as there currently aren't any. It was determined that these should be added for the Python script, and existing unit tests could be leveraged for existing RPC classes in the iOS and JavaSuite libraries.

[theresalech]

@kshala-ford Please advise when PR #849 has been updated to reflect the Python 3 revision. I'll then merge the PR so the proposal is up to date, and leave comments on the implementation issues for this proposal referencing these revisions. Thanks!

[theresalech]

Comments have been left on implementation issues to note the accepted revisions:
RPC
iOS
Java Suite
JavaScript Suite

[theresalech]

Core issue added, as this is now an impacted platform.