grpc-labview icon indicating copy to clipboard operation
grpc-labview copied to clipboard

Specifying CLFN paths on diagram reduces usability

Open gregr-ni opened this issue 2 years ago • 12 comments

When Call Library Function nodes use a path from the diagram, LV builds can't know the location of the library and automatically include it in the built output. This is mitigated in the MSDK service case by explicit post build code in the template projects, but it will be a constant source of issue for general gRPC usage.

Currently the libraries ship in the locations below (relative to the LV directory) and Get Server DLL Path.vi produces the correct path for each platform.

  • vi.lib\gRPC\LabVIEW gRPC Library\Libraries\Linux\liblabview_grpc_server.so
  • vi.lib\gRPC\LabVIEW gRPC Library\Libraries\LinuxRT\liblabview_grpc_server.so
  • vi.lib\gRPC\LabVIEW gRPC Library\Libraries\Win32\labview_grpc_server.dll
  • vi.lib\gRPC\LabVIEW gRPC Library\Libraries\Win64\labview_grpc_server.dll

Instead the libraries should ship in the locations below, Get Server DLL Path.vi should be deleted, and all CLFNs should be configured to reference the equivalent of <vilib>:\gRPC\LabVIEW gRPC Library\Libraries\labview_grpc_server*.*.

  • Targets\NI\RT\Linux\vi.lib\gRPC\LabVIEW gRPC Library\Libraries\liblabview_grpc_server64.so (edited to add "lib" prefix)
  • vi.lib\gRPC\LabVIEW gRPC Library\Libraries\labview_grpc_server32.dll
  • vi.lib\gRPC\LabVIEW gRPC Library\Libraries\labview_grpc_server64.dll
  • vi.lib\gRPC\LabVIEW gRPC Library\Libraries\liblabview_grpc_server64.so

Note: When first configuring the CLFN with the wildcards, the VI may claim the function can't be found. However, if you save and reload the VI in this state, it fixes itself. This appears to be a bug in LV. I observed it in LV 2021, not sure about other versions.

gregr-ni avatar Sep 15 '22 16:09 gregr-ni

We had issues with DLLs in vi.lib if CLF included vi.lib as part of the path when deploying those VIs to Linux RT. We found that putting the DLL in LabVIEW/resource worked better and then the CLF can just specify labview_grpc_server.* for the path. It makes the installation a little trickier because You want to install the 64-bit version in the LV 64-bit resource folder and 32-bit version for LV 32-bit, etc. this would better match where other DLLs like lvanlys, lvalarms, ni_httpClient, skylineAlarms, etc. all go. The recommended location on Linux for shared libraries is /usr/lib/x86_64-linux-gnu and even though CLF path says labview_grpc_server.* it will still find the Linux shared library at /usr/lib/x86_64-linux-gnu/liblabview_grpc_server.so

NIbbuchanan avatar Sep 16 '22 16:09 NIbbuchanan

@eyesonvis I don't believe this directly affects scripted code. All the calls to the dll are in the shipped libraries. Old scripted code using the new libraries would get the new behavior.

@NIbbuchanan I was thinking we would support the lib prefix but wasn't finding the code. I now remember that is done at a lower level than the wildcarding code I was looking at. I edited the issue to put lib back for the Linux binaries.

We might take this to a separate channel but I'd like to hear more about the RT issues. Were you putting the RT binary under Targets as I showed or somewhere else? What I showed is expected to work.

gregr-ni avatar Sep 16 '22 19:09 gregr-ni

I think the problem we ran into with DLLs in vi.lib is that the CLF doesn't use <vi.lib>\DLL Path, but instead C:\Program Files\National Instruments\LabVIEW 2021\vi.lib\DLL Path. Our build process syncs down a build of LV from perforce and uses that LV which isn't at C:\Program Files\National Instruments\LabVIEW 2021\ and so we would have build errors because it couldn't find the DLL...it's been awhile, but I think this was the problem and since we've switched to putting all our DLLs in LabVIEW/resource and not having a path in the CLF, everything works and RT deployment and rtexes also just work once we put the shared libraries/DLLs in the default search locations expected by LV. Hope this helps.

NIbbuchanan avatar Sep 16 '22 20:09 NIbbuchanan

@eyesonvis - This is also related to https://github.com/ni/grpc-labview/issues/75 in case you want to link them together.

NIbbuchanan avatar Sep 16 '22 20:09 NIbbuchanan

@NIbbuchanan The CLFN dialog always shows an absolute path but it will use symbolic paths when saving. If there was a problem, it was not that CLFN doesn't use symbolic paths. I still suspect the issue was not using an appropriate Targets/NI/RT vi.lib directory for the RT binaries.

The disadvantage of using the resource directory it that the appbuilder won't include the dll in its output. It doesn't include any dlls referenced only by name. That means that every customer using gRPC in a built binary would need to manually arrange for the dll to be copied. I think this is worth giving another shot.

gregr-ni avatar Sep 16 '22 20:09 gregr-ni

@gregr-ni I did some testing with dll paths and here's what I found: if I put the dll in vi.lib\gRPC\LabVIEW gRPC Library\Libraries\labview_grpc_server64.dll

  • Built windows exe and DLL copied to output folder and worked as expected
  • Running in an RT context did not work. I had put the shared library in Targets\NI\RT\Linux\vi.lib\gRPC\LabVIEW gRPC Library\Libraries\liblabview_grpc_server64.so as well as copying to target at /usr/lib/x86_64-linux-gnu
  • Create an rtexe and deploy and run as startup didn't work. Probably failed to load exe because it couldn't find dependencies
  • Made a copy of the LV folder (to simulate our build process that syncs LV from perforce) and removed DLL from original LV vi.lib. Everything worked fine and using alternate LabVIEW.exe correctly used DLL from it's vi.lib location

I then tried moving the DLL to LabVIEW/resource and specifying "labview_grpc_server.*" in the CLF path

  • Built windows exe didn't work since it was searching for DLL since it wasn't copied to build folder.
  • Running in an RT context worked.
  • Create rtexe and deploy and run as startup worked.
  • Made a copy of LV folder and use alternate LV.exe location worked

I looked at lvanlys and the CLF specified the full path to the DLL in the resource folder (i.e. C:\Program Files\National Instruments\LabVIEW 2021\resource\lvanlys.*) I tried specifying full path to DLL in resource folder i.e. " C:\Program Files\National Instruments\LabVIEW 2021\resource\labview_grpc_server.*

  • Built windows exe copied DLL to output folder and exe worked
  • Running in an RT context worked.
  • Create rtexe and deploy and run as startup worked.
  • Made a copy of LV folder and use alternate LV.exe location worked

So it looks like maybe we should put it in the LabVIEW/resource folder and specify the full path to the DLL in the CLF.

NIbbuchanan avatar Sep 19 '22 16:09 NIbbuchanan

Hello

I think I would be able to submit a PR that can address this if you would be interested?

Updating the build tooling to generate and handle building binaries that are automagically loaded with the CLFN wildcard tricks from the path is reasonably straightforward.

However, if my understanding is correct then library users will have code with the current binary path specification method already out in the wild so I feel like I would need to create scripting code which can find and update the relevant CLFNs in an existing library. Would that be the case?

Also the current method provides scope to inject a debug directory path into all the CLFNs. This could be retained but it would require extending the scripting to wrap each CLFN in a conditional disable structure so a DEBUG conditional flag would be set (via the LabVIEW Project) which would make the library work as it does at the moment. Is this debug functionally still required?

Finally - how do you want to handle the location of the feature_configs file? Should it still live in the folder alongside the binary file? This would mean that should a user want the same configs on a built/deployed application, they would have to manually install that file alongside the .dll or .so which would be fine I guess - just some additional docs would be required.

Cheers

John

j-medland avatar Jun 23 '24 00:06 j-medland

Thanks for the update John! Very exciting to hear this may finally be fixed. A few comments:

  1. Since the change would affect low-level VIs and doesn't impact the connector pane, I feel like you can make the change without worrying about scripting code to replace existing VIs. For existing users, if they update to the new version with this change, their code will still be calling the same GRPC VIs in vi.lib, which under the hood now automagically call dll without needing path passed in, but user doesn't need to worry about that. If they have a built executable, it will still be using the old code since that was built into their executable along with the old dll. I would be interested to hear if there are real scenarios where this change would break people and if there are corner cases, I would be fine documenting the new behavior and let them sort it out instead of trying to make some scripting code to handle these uncommon use cases (let me know if there are common use cases where this approach would be a problem that I'm not aware of).
  2. For the feature_config.ini, I would have the DLL check for this file in the same location where the DLL/shared library is located. When the dll loads, it checks for the ini (i.e. only do this once) and apply any settings specified if the INI is found. Then you don't need any LV code to handle this and you can completely remove the Get Server DLL Path.vi since the DLL will take care of doing this only once and always look in the same folder where the DLL resides. Users can worry about copying this file to the same location as the DLL/shared library when building exe to distribute if they need special feature config settings...that's my vote.

NIbbuchanan avatar Jun 24 '24 16:06 NIbbuchanan

Thanks - I think that all makes sense.

In terms of determining the DLL Location - I have found a suitable cross-platform library that is permissively licenced (via the PG-13 "DO WHAT THE F*** YOU WANT TO PUBLIC LICENSE") that should simplify getting the shared-binary file path - which is otherwise a bit involved on Linux.

If you have no objections to adding this dependency then I will add it as a git submodule and update the build accordingly.

I think we can trigger the feature_config.ini read by adding a DLL_Main entry-point to the C++ code for Windows and an ____attribute__((constructor))'d function for Linux. Alternatively I could make an exported function which would be triggered from a CLFN Reserve Callback on the Create Server and Create Client CLFNs - but only the first call to said function would actually do anything. Did you have a preference on which approach to take?

j-medland avatar Jun 25 '24 13:06 j-medland

Sounds good, thanks for investigating this! I think the DLL_Main constructor makes the most sense. Thanks!

NIbbuchanan avatar Jun 25 '24 14:06 NIbbuchanan

I have been trying out some ideas and I think I am making good progress on narrowing in on something suitable.

I do have ~~a couple of~~ some questions:

  1. Is the example_client used for testing? I can see the source file and the CMakeLists.txt build and proto file related codegen but I cannot see any reference to it being launched in any of the test suites. If it is unused, could we consider removing it?

  2. Are we tied to such an old version of CMake? I see that github-runners ship with CMake 3.29 and whilst a Linux RT 2019 target is limited to 3.5-ish, I doubt anyone is actually going to dust off such a target when they can just cross-compile. I would propose moving to at least 3.15 so we can use the polished version of CMake's FetchContent commands which makes using external code a bit easier.

  3. As the binary names need to have the same name on all platforms (apart from the extensions), do you want to use the "lib" prefix on all platforms (unix style) or omit it (windows style)?

  4. As we are talking names - The"gRPC lv Support" directory contains the "grpc-labview" project and the "grpc-lvsupport" LabVIEW library which depends on the labview_grpc_server.* binary. We are sort-of stuck with "grpc-lvsupport.lvlib" if we don't want to break lots of other code but could we maybe rename the binary to either "<lib>grpc_labview" or maybe "<lib>grpc_lv_support" just to reduce the number of similar but different names flying around by 1?

j-medland avatar Jul 02 '24 22:07 j-medland

  1. and 2. I don't know anything about testing/building of grpc. I would ask someone on the team about these questions. I don't care if you want to delete unused code or update CMake version as long as everything strill works for applications/user code that uses the grpc library.

  2. I'm pretty sure LV automatically adds a "lib" to the beginning of shared libraries on linux, so the CLF can use "labview_grpc_server.*" for the name and if you put the DLL/shared library in the default search path for Windows and Linux, it will be found: Windows 64-bit: C:\Program Files\National Instruments\LabVIEW\resource\labview_grpc_server.dll Windows 32-bit: C:\Program Files (x86)\National Instruments\LabVIEW \resource\labview_grpc_server.dll Linux RT: /usr/local/natinst/lib/labview_grpc_server.so it may now be updated to /usr/local/natinst/LabVIEW/linux/labview_grpc_server.so (see how other driver shared files are named and where they are located). Note: You may also include a version in the name of the shared library and then you need to create a symbolic link file named labview_grpc_server.so that links to the right version (i.e. labview_grpc_server.24.0.so...again, see how other NI shared libraries work). Linux: Maybe check with @gregr-ni on this...not sure if it's different than Linux RT.

  3. Not sure it's worth it, but I don't have strong feelings on it. Maybe see what Greg Richardson or others think. Thanks!

braddbuchanan avatar Jul 08 '24 14:07 braddbuchanan