SDL-0234 Proxy Library RPC Generation

[vladmu]

Fixes #1298

This PR is [ready] for review.

Risk

This PR makes [no] API changes.

Testing Plan

• I have run the unit tests with this PR

Unit Tests

Covered by unit tests and PyLint.

Summary

According to [SDL-0234] Proxy Library RPC Generation proposal, this PR adds the Python generator script based on XML Parser from smartdevicelink/rpc_spec linked to the repository as Git submodule. Based on answers provided by PMs in the related issue #1298 all customizations are postponed at this stage, the generated code follows the naming convention described here #1298 (comment), requirements of initializers described here #1298 (comment) and common behavior of other code parts, defined from the existing source code. The generated code is not a full replacement of existing RPC classes and doesn't guarantee the build and existing unit tests will be passed after the full replacement. Meantime the generator includes the rich "custom mapping" tool which allows customizing classes over the generation process if needed.

Changelog

Enhancements

• Added generator/rpc_spec submodule pointed to develop branch of smartdevicelink/rpc_spec repository
• Created Proxy RPC Generator based on Python 3.5 and linked with parser from the generator/rpc_spec submodule
• Added README.md with usage description

CLA

• I have signed the CLA

[kshala-ford]

@vladmu Thank you for all the work on implementing the RPC generator scripts. I ran the generator and used a diff tool to compare. It looks like many of the initializers are missing. In rare cases the classes contain outstanding methods like SDLAppInfo has a class method + (instancetype)currentAppInfo; Also the unit tests of sdl_ios itself are showing quite some errors.

Another example where the developers manually deprecated a parameter:

-@property (nullable, strong, nonatomic) SDLSyncMsgVersion *rpcSpecVersion __deprecated_msg(("Use maxRPCSpecVersion instead"));
+@property (nullable, strong, nonatomic) SDLSyncMsgVersion *rpcSpecVersion;

Question: When creating new RPC files how are these files going to be added to the Xcode project? I doubt there is a way to add files programmatically to the pbxproj file.

@joeljfischer I know this PR isn't ready for review but I believe this proposal implementation needs attention. Some details like the deprecation or the custom methods like currentAppInfo could be covered by the mapping, though it adds a lot of complication to contribute code to the project (previously you could just work on the code but now code is scattered, either it's manual source, based on mapping or generated). There's a possibility to create extensions per RPC class adding all the initializers. However, that seem to ruin the idea of generating code if you still need to create code manually...

[vladmu]

Thank you for all the work on implementing the RPC generator scripts. I ran the generator and used a diff tool to compare. It looks like many of the initializers are missing. In rare cases the classes contain outstanding methods like SDLAppInfo has a class method + (instancetype)currentAppInfo; Also the unit tests of sdl_ios itself are showing quite some errors.

@kshala-ford I pointed in the PR Summary regarding this. The effort for mapping all classes is more than simple manual coding, as we need to customize almost all RPCs. We agreed with PMs to postpone the mapping for the current stage.

When creating new RPC files how are these files going to be added to the Xcode project?

I believe it should be done manually by the person who generates the particular RPC and tries to commit this code into the repository.

[kshala-ford]

@vladmu Good point. I should have had a closer look at the issue. Let's continue such discussions in the issue.

[vladmu]

@kshala-ford @theresalech respecting the committee decided to remove the following section from the proposal downsides:

3. The generator could produce bad RPC classes, however, the library authors will ensure that this is not the case and that the first version of the generated code matches what already exists, and the unit tests will continue to pass.

It would be appreciated if you could summarize and define action points for us here regarding what we should do in this PR to follow the SDLC decision. As soon as Ford approves this PR, the status will be moved to "ready for review".

[theresalech]

Hi @vladmu - mappings for exceptions/corner cases should be removed from this PR. Please reference PM feedback provided on the Java Suite PR, and let us know if you have any additional questions. Thank you!

[joeljfischer]

I think the action item would look like this:

• Ensure that corner case mapping is only performed for general cases. e.g. All enum names starting with a number need to be prefixed with a _, but we don't want to fix any individual enums or enum values. We only want to apply a corner case mapping if it can be applied to all functions and/or structs and/or enums, etc.

[vladmu]

@theresalech @joeljfischer thank you for your answers and clarification. We are going to follow that now. @kshala-ford @mrapitis the PR is ready for Ford review, please take a look.

[kshala-ford]

I ran the scripts to generate all classes and manually reviewed the resulted code. I'm not looking for replacing existing RPCs but wanted to review and understand how the scripts behave if new RPCs would be added. Here a few things I found:

1.
The script seem to use __deprecated attribute for any deprecation. For params it makes sense but there seem to be an error when using this attribute on classes. Example:

@interface SDLSetDisplayLayoutResponse : SDLRPCResponse __deprecated

This results in an Xcode parser error. The error is Expected identifier or '(' at the next method in the class. It's better to use

__attribute__ ((deprecated))
@interface SDLSetDisplayLayoutResponse : SDLRPCResponse

@joeljfischer please confirm if you're OK with this approach.

2.
Don't think it's required to fix immediately but it's an idea to address later on. The inline documentation is kind of confusing for versioned items. For example the doc for SetDisplayLayout doesn't show the history. The RPC exist since 3.0 but was deprecated with 6.0. This is not shown in the documentation.

/**
 * This RPC is deprecated. Use Show RPC to change layout.
 *
 * @deprecated
 * @since SDL 6.0.0
 */
@interface SDLSetDisplayLayout : SDLRPCRequest

3.
I found occurrences of HMI and Hmi. Should the generator generate as is from the mobile API or should we add a rule to always uppercase to HMI ?

4.
Another HMI vs. Hmi: The enum HmiZoneCapabilities was generated as SDLEnum SDLHmiZoneCapabilities but the files were called SDLHMIZoneCapabilities.h and SDLHMIZoneCapabilities.m. In the .m file it does #import "SDLHmiZoneCapabilities.h" which can't be found obviously.

I believe the reason was that I test with --overwrite and the existing file was called SDLHMIZoneCapabilities.h. Due to my APFS volume being case-insensitive the scripts didn't successfully performed the overwrite.

5.
VR vs. Vr? One reason why I keep asking is because new RPCs can't be generated properly that use existing structs, or enums that have VR or HMI in the name. Those would need manual repair...

6.
SDLPredefinedWindow is a numeric enum not a string based enum. If the enum element values are numeric the enum should be a numerical enum. I know we don't overwrite existing code but in future new enums could be added that are also numeric.

See example output of a numeric enum

typedef NS_ENUM(NSUInteger, SDLPredefinedWindows){
    /// The default window is a main window pre-created on behalf of the app.
    SDLPredefinedWindowsDefaultWindow = 0,

    /// The primary widget of the app.
    SDLPredefinedWindowsPrimaryWidget = 1
};

7.
The struct TouchEvent has a parameter called id. This is a keyword in objc. The scripts should never use this keyword. A rule should be added if the param name is id the generated objc property should be called "${rpcName}Id" so in the example of touch event it should be touchEventId.

[joeljfischer]

1. The best way to do this is to follow what SDLOnLockScreenStatus does:

__deprecated
@interface SDLOnLockScreenStatus : SDLRPCNotification

2. I agree with Kujtim here. The documentation is pretty confusing. It's not a required fix immediately because we're not overwriting current RPCs, but an issue should at least be filed.

3/4/5. This is annoying. I think we should just generate straight from the RPC_Spec, but I can see both arguments. Whichever way it goes, it needs to align with Java_Suite.

6. Agreed.

7. I'm fine with that approach to keywords. We should define a list of keywords to do this approach with, and do the same for Java_Suite.

[o-mishch]

1. changed to approach suggested by @joeljfischer.
2. added @history, currently looks like as follow:

/**
 * This RPC is deprecated. Use Show RPC to change layout.
 *
 * @deprecated
 * @history SDL 3.0.0
 * @since SDL 6.0.0
 */
__deprecated
@interface SDLSetDisplayLayout : SDLRPCRequest

3/4/5. Currently generator generate as is from the mobile API. Let me know if you would like to hardcode any other approach, e.g. capitalize Vr to VR and Hmi to HMI.
For cases of overriding I added removing file first and then writing/creating it.

6. Added new template sdl_ios/generator/templates/enums/template_numeric.h which going to be used for enums which has attribute value, e.g. PredefinedWindow

7. Added approach suggested by @kshala-ford

[theresalech]

@kshala-ford @mrapitis please let us know once Ford has approved this PR and it is ready for Livio review, thanks!

[mrapitis]

@theresalech Ford has approved of this PR.

[vladmu]

@theresalech I marked the PR as ready for Livio review in the description. Please take a look.

[joeljfischer]

This was getting too long, so review part 1

[joeljfischer]

This was getting too long, so review part 1

[o-mishch]

@joeljfischer The PR is ready for review.

[o-mishch]

@joeljfischer The PR is ready for review.

[joeljfischer]

@o-mishch @vladmu Alongside the Java Suite and JavaScript Suite the iOS library generator will need to avoid using reserved keywords. See the PR description by @bilal-alsharifi (I have copied the current version below) for a list of rules that should be followed when avoiding these reserved keywords. Here is the PR: smartdevicelink/rpc_spec#232

This PR adds a list of keywords that RPC Generators in all platforms (iOS, Java Suite, JavaScript Suite) should avoid using by following these rules:
- If the generator needs to create a variable or method and the name was one of the names in the reserved keyword, the generator should append the suffix `Param` to it. For example `getFunctionID`  > `getFunctionIDParam`
- Any empty line in the reserved keywords file should be ignored
- Any line in the reserved keywords file that starts with one or more `#` symbols then `space` should be considered a comment and ignored (like `#<space>` or `##<space>` or `###<space>` …). Note that some reserved keywords have a `#` without a `<space>` so it can't just check for `#`.
- Reserved keywords matching should be case insensitive
- Although the reserved keywords list has a section for each platform, The generator for each platform should avoid using any of the keywords whether it belongs to that platform or another one.

Let me know if there are any questions.

[joeljfischer]

Enums don't need to modify their object based on the reserved keywords unless the entire value (remember we prefix with SDL and the enum type) matches the reserved keyword.

[o-mishch]

so if keyword would be SDLDisplayTypeCID then it should be renamed as follow?

//DisplayType.h
extern SDLDisplayType const SDLDisplayTypeCidParam;
//DisplayType.m
SDLDisplayType const SDLDisplayTypeCidParam = @"CID_PARAM";

[joeljfischer]

If the keyword is CID, it would be:

//DisplayType.h
extern SDLDisplayType const SDLDisplayTypeCid;
//DisplayType.m
SDLDisplayType const SDLDisplayTypeCidParam = @"CID";

If the keyword were SDLDisplayTypeCID it would be:

//DisplayType.h
extern SDLDisplayType const SDLDisplayTypeCidParam;
//DisplayType.m
SDLDisplayType const SDLDisplayTypeCidParam = @"CID";

The string value MUST remain as the constant (and having a string be a keyword is fine in any language anyway), or else SDL will fail.

[o-mishch]

done in 8d53846

[joeljfischer]

This doesn't seem to be completely fixed. SDLImageType's static value: SDLImageTypeStatic is being turned into SDLImageTypeStaticParam. This shouldn't happen because SDLImageTypeStatic is not a keyword even though static is.

[joeljfischer]

Same for SDLAppHMIDefaultParam, that should be SDLAppHMIDefault.

[joeljfischer]

Same for SDLResultSuccessParam. It should be SDLResultSuccess.

[o-mishch]

changed to following regex:

r'^(sdl)?({}){}$'.format(item_name.casefold(), name.casefold())

in e131215

[o-mishch]

@joeljfischer The PR is ready for review.