Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SetRuntimeEnvironment call to library constructors #973

Conversation

ni-jfitzger
Copy link
Contributor

@ni-jfitzger ni-jfitzger commented Jul 20, 2023

What does this Pull Request accomplish?

Add an Library::SetRuntimeEnvironment call upon library load, before init is called.

(Based on #962, but tweaked the template code to be a little cleaner)

Implemented via the following changes:

  • Update library.cpp.mako to call SetRuntimeEnvironment if the function exists
  • Generate a version.h with some of the values found in version.rc, defined as string constants (couldn't figure out any other way to get access to the data easily)
  • Update CMakeLists.txt and generate

Why should this Pull Request be merged?

As developers, we want to know how much nigrpc_device_server is used and which versions.

What testing has been done?

  • Built ni_grpc_device_server.exe locally and called it with the nimi-python system tests in CEIP test mode.
    • Verified that the Runtime Environment data was logged correctly.
  • Manually launched the server and called it, opening and closing sessions, as well as the python process that I called it from, multiple times. Confirmed that data was only logged once. This makes sense because the service that loads the library is supposed to remain loaded for the lifetime of the server instance.

TODOs:

  1. Create unit tests.
    We have a separate internal work item for this. The current plan is to templatize the LibraryClasses so that a FakeSharedLibrary class can be used and add a little functionality to the LibraryClass to track whether SetRuntimeEnvironment has been called.

CMakeLists.txt Outdated Show resolved Hide resolved
@reckenro
Copy link
Collaborator

reckenro commented Jul 20, 2023

@ni-jfitzger , sorry I missed this on the metadata PR, but do we want this for NI-TClk too? If so, I think having a follow up PR to update its metadata and regen makes sense.

@ni-jfitzger
Copy link
Contributor Author

ni-jfitzger commented Jul 20, 2023

@ni-jfitzger , sorry I missed this on the metadata PR, but do we want this for NI-TClk too? If so, I think having a follow up PR to update its metadata and regen makes sense.

No need. NI-TClk doesn't have devices or sessions. It calls into other drivers using their session handles. So, it has no telemetry.

@ni-jfitzger
Copy link
Contributor Author

Some of the System Tests sure are flaky.

@ni-jfitzger ni-jfitzger merged commit 96f9297 into ni:main Jul 21, 2023
@ni-jfitzger ni-jfitzger deleted the add-SetRuntimeEnvironment-call-to-Library-Constructors branch July 21, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants