Minor usability updates to gRPC code generator

[jplotzke]

This pull request has several minor "quality of life" improvements to the client/server generator:

• VIs required to run the server are moved to the generated server lvlib public scope. This allows incorporation of the gRPC service into a larger application. (Fixes Generated server lvlib should not make methods private that are needed to start the service #450)

  • In addition, all UEs and RPC methods, along with their associated controls, are also moved back to public scope for backwards-compatibility. (Fixes Type Definitions and Accessors for RPC Methods need to be made Public again #453)

• Added icon text to "Rich to Flat" and "Flat to Rich" VIs (for both client and server generated VIs)
  

• The generated enum converter VIs ("LV enum to proto enum" and vise-versa) used a CLFN to labview_grpc_server.dll to convert LV enum values to/from proto enum values. As all other server DLL calls are wrapped in grpc-lvsupport.lvlib (and also that TranslateGrpcError.vi is in private scope of that library), I created two wrapper functions in grpc-lvsupport.lvlib for these calls. The client template VIs are subsequently updated to use these wrapper VIs instead of the direct CLFN. (Fixes Generated Enum Flat/Rich converters should not call DLL in UI thread #447)

• In the spirit of the above update, I created a new method in the LV Support lvlib to wrap the DeserializeReflectionInfo DLL call. This was previously called from Register Descriptors.vi in the template server lvlib. I then updated Register Descriptors.vi to call the newly created Register Reflection Descriptors.vi.

  • In my opinion, all calls to labview_grpc_server.dll should take place from the LV Support lvlib. This way, the DLL and associated VIs are packaged/versioned together and there's much less risk of breaking changes between versions. With the change above, I believe that all labview_grpc_server.dll calls are removed from the template server and client VIs and now are only called from the grpc-lvsupport.lvlib VIs.

• I updated the Oneof template so that the set value template (and only the set value template) no longer requires the lvclass input (now set to recommended). This fixes Don't set input terminals unnecessarily to required #417.
  

  • I also updated the generator to add the field name text to the oneof set/get/has accessor VI's icon text. In most cases, the field name is truncated in the icon, but I believe it looks nicer than no text.
  • I also updated the oneof template lvclass icon text to say "ONE OF"... just to make it look a bit nicer/cleaner

• Removed the name for the new local application context that's created by the main generation application. When using this generator as a subVI (to call it automatically for multiple services), calling it on the 2nd time resulted in an error that the application context name already exists. Not passing any name to this function ensures that a unique name is created every time, allowing it to run multiple times programmatically.
  

• Updated label for "Generate" control to "Generate Server or Client?" for better naming when using generator tool as a subVI.
  

• Standardized on lvclass input/output naming for FP controls/indicators throughout LV Servicer and server templates.

  • Servicer.lvclass now uses gRPC server in/out
  • ServiceBase.lvclass, ServiceName Template.lvclass, and Service Template.lvlib now use gRPC service in/out

[jplotzke]

@jasonmreding - I see the build_release_artifacts step still failing. Did I cause this when I updated the vipb files which seems to inadvertently updated the Package_LabVIEW_Version tag from 19.0 to 19.0 (64-bit).

grpc-labview/labview source/gRPC lv Support/build spec/LabVIEW gRPC Library.vipb

Line 11 in 53c8347

<Package_LabVIEW_Version>19.0 (64-bit)</Package_LabVIEW_Version>

Should I revert back my VIPB updates and then manually edit the file to update the version and dependencies?

[jasonmreding]

@jasonmreding - I see the build_release_artifacts step still failing. Did I cause this when I updated the vipb files which seems to inadvertently updated the Package_LabVIEW_Version tag from 19.0 to 19.0 (64-bit).

grpc-labview/labview source/gRPC lv Support/build spec/LabVIEW gRPC Library.vipb

Line 11 in 53c8347

<Package_LabVIEW_Version>19.0 (64-bit)</Package_LabVIEW_Version>

Should I revert back my VIPB updates and then manually edit the file to update the version and dependencies?

I suspect that is it. I know the self hosted runner only has LV 2019 32 bit installed. IIRC VIPM will automatically modify the LV version for the package if you don't have the version of LV it specifies installed. I think VIPM displays a warning message about this though it may not be obvious what the impact of that message is. You should just be able to edit the xml in a text editor to revert the LV version back to its original form.

[jplotzke]

@jasonmreding - Argh, still failing - this time for There was an error recalling the build specification (invalid build signature).

I opened each of the VIPB files in VIPM, built each package to dry-run the build, then re-saved. Then opened each VIPB file and manually edited the LV version back to 32-bit.

Hoping this one helps. Appreciate the patience. :)

[jplotzke]

@jasonmreding - Argh, still failing - this time for There was an error recalling the build specification (invalid build signature).

I opened each of the VIPB files in VIPM, built each package to dry-run the build, then re-saved. Then opened each VIPB file and manually edited the LV version back to 32-bit.

Hoping this one helps. Appreciate the patience. :)

Ok, happened again with same error. Odd since I'm able to run the BuildGRPCPackages.vi on my machine (although with LV 2019 64-bit) and it runs ok and generates all packages. My only guess is that opening up the VIPB files changed a GUID/ID in the file that's different between 32-bit and 64-bit and therefore VIPM is complaining on the build machine because something doesn't line up?

I can try installing LV 2019 32-bit and see if I can repro this issue later.

[jasonmreding]

@jasonmreding - Argh, still failing - this time for There was an error recalling the build specification (invalid build signature).
I opened each of the VIPB files in VIPM, built each package to dry-run the build, then re-saved. Then opened each VIPB file and manually edited the LV version back to 32-bit.
Hoping this one helps. Appreciate the patience. :)

Ok, happened again with same error. Odd since I'm able to run the BuildGRPCPackages.vi on my machine (although with LV 2019 64-bit) and it runs ok and generates all packages. My only guess is that opening up the VIPB files changed a GUID/ID in the file that's different between 32-bit and 64-bit and therefore VIPM is complaining on the build machine because something doesn't line up?

I can try installing LV 2019 32-bit and see if I can repro this issue later.

Yeah, I'm not sure what is going on. The error information returned isn't all that helpful and there isn't much logging available to determine how far things progressed or even which package is reporting the problem. You could try reverting all changes to the .vipb files back to the original other than the package and package dependency version updates and see if that helps. The VIPM error mentions Source:: BDB9067948AA5DEB4C77B2EF2E3D67FD but I have no idea what that GUID is referring to.

I just tried opening the .vipb files on the self hosted runner with its version of VIPM, and I get the following error:

I copied the same files to my dev machine, and I was able to open them without error. I am using VIPM 2024.3 (build 2761) on my dev machine. The version on the self hosted runner is shown below:

So I don't know what is going on. Either there is a problem/bug with the different VIPM versions interoperating with each other or there is some weird caching/state issue with VIPM on the self hosted runner machine. As I was poking into this, the runner seems to have gone offline again so this might be a holding pattern until somebody can diagnose that again.

[jplotzke]

...
So I don't know what is going on. Either there is a problem/bug with the different VIPM versions interoperating with each other or there is some weird caching/state issue with VIPM on the self hosted runner machine. As I was poking into this, the runner seems to have gone offline again so this might be a holding pattern until somebody can diagnose that again.

Thanks. I've also reached out to JKI to see if they can shed any light for what an "invalid build signature" is and if they'd expect those GUIDs/ID values that change when I re-save the file to represent something specific or are they just random values that we can ignore.

[jplotzke]

...
So I don't know what is going on. Either there is a problem/bug with the different VIPM versions interoperating with each other or there is some weird caching/state issue with VIPM on the self hosted runner machine. As I was poking into this, the runner seems to have gone offline again so this might be a holding pattern until somebody can diagnose that again.

Thanks. I've also reached out to JKI to see if they can shed any light for what an "invalid build signature" is and if they'd expect those GUIDs/ID values that change when I re-save the file to represent something specific or are they just random values that we can ignore.

Good news - I set up a clean machine with LV 2019 32-bit and VIPM 2021.0 and I get the same error that you get on the build machine. So, I don't believe that this has anything to do with the runner.

[jplotzke]

@jasonmreding - I reverted back the VIPB files, and then opened them on a system with LV 2019 32-bit and set all version properties again. VIPM 2021 can now open these files.

I think the root cause is that these files can't be edited manually and the "ID" field must be a checksum of sort. In order to update these files, one must have LV 2019 32-bit installed so that VIPM doesn't change that field.

[BShermanator]

In addition to the "RegistergRPC Messages.vI" reverting back to public, there are other accessors and type definitions that need to be moved back to public. Outlined the changes in this issue: #453

[jplotzke]

In addition to the "RegistergRPC Messages.vI" reverting back to public, there are other accessors and type definitions that need to be moved back to public. Outlined the changes in this issue: #453

@jasonmreding - How about as a compromise, we move all of these items back to public scope, but issue a deprecation notice with the release notes that these will eventually be moved back protected? I do feel like some users moved up to v1.5.0.1 for the security updates (myself included) and then were surprised/not expecting the protected-scope items.

With this, v1.6.0.1 maintains backwards-compatibility with previous applications which use these VIs. We still leave the new Create Services.vi in place and use that in the generated example Run Server.vi and advise that that is the intended approach.

Then, once the base issues that cause people to use these VIs (like grabbing the UEs out of the class) are resolved, a v2.0.0 release is targeted which moves these back to protected-scope and issues a warning for breaking changes.

[jasonmreding]

In addition to the "RegistergRPC Messages.vI" reverting back to public, there are other accessors and type definitions that need to be moved back to public. Outlined the changes in this issue: #453

@jasonmreding - How about as a compromise, we move all of these items back to public scope, but issue a deprecation notice with the release notes that these will eventually be moved back protected? I do feel like some users moved up to v1.5.0.1 for the security updates (myself included) and then were surprised/not expecting the protected-scope items.

With this, v1.6.0.1 maintains backwards-compatibility with previous applications which use these VIs. We still leave the new Create Services.vi in place and use that in the generated example Run Server.vi and advise that that is the intended approach.

Then, once the base issues that cause people to use these VIs (like grabbing the UEs out of the class) are resolved, a v2.0.0 release is targeted which moves these back to protected-scope and issues a warning for breaking changes.

I'm OK moving the items back to public scope. I think the hope in moving them to protected was to minimize the public API surface area for backward compatibility for things that were deemed to be implementation details and not proper public APIs. However, it seems that ship has sailed in this case.

I'm not sure deprecation is really an option for the service initialization methods unless we are OK with requiring upcasting from Create Services.vi if you need to store off concrete type instances in your server implementation. The only other solution I can really think of is to fix the code gen so the service metadata is encapsulated with the service class implementation rather than the wrapper library which contains all of the services. This would allow you to add a Create Service.vi or Initialize Service.vi to the public API of each service class using the concrete wire type of the service class. This would require you to pass in the server instance into the service class method rather than the other way around as it is with the Create Services.vi.

I don't really see a path forward for keeping the event types protected unless we can come up with a default dispatching implementation that everybody is happy with. I'm not optimistic we will be able to do that. I was kind of hoping that code generating a custom Handle RPC member VI per event that must be overridden would be sufficient. I definitely think that is an improvement in terms of separating generated code from user written code. However, I suspect some users won't be happy with having N places where they need to feed into a centralized dispatching mechanism as opposed to say a single event structure with N cases or even a single event structure with one event case for all RPCs. If users want to choose something in between for a single service or combine dispatching across multiple services via a single event structure, then things are even more challenging.

[jasonmreding]

@kt-jplotzke The Register gRPC Methods.vi on ServiceBase.lvclass is still marked as protected which is breaking the generated server hierarchy. Also, I believe other changes will be made as a result of renaming the generated Run Service.vi to Run Server.vi. If you grep for that string, you will see some hits to it in Python test scripts. There are also some string constants to it in some LV test VI wrappers. If you want to revert that change and submit it in a follow on PR, I am fine with that. I'm also OK if you want to keep moving forward with the change. Just wanted to warn you it might take a few more iterations to find/fix all of the fall out from that. We should probably update some of the repo documentation as well because of it as a number of tutorials reference that VI by name.

[jplotzke]

@kt-jplotzke The Register gRPC Methods.vi on ServiceBase.lvclass is still marked as protected which is breaking the generated server hierarchy. Also, I believe other changes will be made as a result of renaming the generated Run Service.vi to Run Server.vi. If you grep for that string, you will see some hits to it in Python test scripts. There are also some string constants to it in some LV test VI wrappers. If you want to revert that change and submit it in a follow on PR, I am fine with that. I'm also OK if you want to keep moving forward with the change. Just wanted to warn you it might take a few more iterations to find/fix all of the fall out from that. We should probably update some of the repo documentation as well because of it as a number of tutorials reference that VI by name.

Thanks - Yeah, I see the Run Service.vi being called out in the ATS code. I don't mind pushing through just to get it all done now in this PR. I'll look into this more later today.

[jplotzke]

@kt-jplotzke The Register gRPC Methods.vi on ServiceBase.lvclass is still marked as protected which is breaking the generated server hierarchy. Also, I believe other changes will be made as a result of renaming the generated Run Service.vi to Run Server.vi. If you grep for that string, you will see some hits to it in Python test scripts. There are also some string constants to it in some LV test VI wrappers. If you want to revert that change and submit it in a follow on PR, I am fine with that. I'm also OK if you want to keep moving forward with the change. Just wanted to warn you it might take a few more iterations to find/fix all of the fall out from that. We should probably update some of the repo documentation as well because of it as a number of tutorials reference that VI by name.

@jasonmreding - Updates:

• Set Register gRPC Methods.vi in ServiceBase.lvclass and the ServiceName Template.lvclass back to protected scope to align with previous generated code. This shouldn't be called by the user and was inadvertently set when Register gRPC Messages.vi should be the VI in public scope.
  -Updated New_ATS code to call Run Server.vi instead of Run Service.vi. I got the ATS code running and am able to run all tests on my machine with passing results.
  -AutoTests were previously failing because of the mixed protected/public scope on Register gRPC Methods.vi. I am now able to run all AutoTests on my machine with passing results.
  -Updated docs to use Run Server.vi instead of Run Service.vi

Note that the examples still use the old nomenclature and would need to be re-generated to align with the documentation. This may affect the AutoTests execution. Do you want that as part of this PR?

[jasonmreding]

Note that the examples still use the old nomenclature and would need to be re-generated to align with the documentation. This may affect the AutoTests execution. Do you want that as part of this PR?

I think this PR has already sprawled on longer than what would be considered ideal. At this point, I would rather perform additional refactoring/cleanup in follow on PRs that are easier to review in isolation.