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

CMakeLists: Make grpc-device buildable on NILRT 11 #1082

Merged
merged 18 commits into from
Sep 11, 2024

Conversation

rajendra-desai-ni
Copy link
Contributor

@rajendra-desai-ni rajendra-desai-ni commented Aug 21, 2024

AB#2640878

What does this Pull Request accomplish?

NILRT 11 will ship with grpc >1.60 and protobuf >v25.2. grpc-device doesn't compile with these toolchain versions, throwing errors about undefined symbols. Downgrading grpc back to 1.51 (with python3-grpcio and protobuf recipes) successfully works around these issues. But for security and general currency reasons, we cannot afford to ship NILRT 11 with these downgrades.
Current CMakeLists.txt is limited to some of the bitbake functionalities which makes it difficult to build with new changes.

Changes in this commit will make sure NILRT 11 compiles grpc-device with the latest/upgraded grpc version without affecting the existing build process.

Why should this Pull Request be merged?

This patchset includes necessary changes to enable building on NILRT, within OpenEmbedded.

What testing has been done?

  • Able to build ni-grpc-device using cmake with new changes
  • Able to start ni-grpc-device-server on windows host
  • Able to build nilrt 11 (including ni-grpc-device) successfully with the changes

Implementation

  1. refactor toolchain link logic:
    Deprecate the CMAKE_CROSSCOMPILING variable, in favor of USE_SUBMODULE_LIBS cmake option. Refactor the linking
    logic to be a consolidation of all the linking actions from across the file, and to better support builds in generic linux
    environments.
  2. fixup utf8cpp library link:
    The utf8cpp cmake library namespace is incorrectly identified as 'utf8cpp', instead of the proper 'utf8cpp:utf8cpp'. As a result,
    cmake does not link the utf8.h header and compilation fails.
  3. parameterize python3 venv:
    Create a USE_PYTHON_VIRTUALENV cmake option. When asserted, it will add the bespoke venv to the toolchain. Otherwise,
    the cmake config will use the system python environment.
  4. link the device server to grpc_gpr:
    ni_grpc_device_server target depends on symbols from grpc gpr.so, namely gpr_log. Add grpc_gpr to link libraries for
    ni_grpc_device_server.
  5. add abseil_sync dep to server target:
    ni_grpc_device_server uses symbology from libabsl_synchronization library. Add a library dependency to reflect that
    relationship.
  6. add utf8cpp dep to IntegrationTestsRunner:
    The IntegrationTestsRunner depends on utf8.h header indirectly, via its access to the device server source.
  7. fill out target lib deps:
    Shove ni_grpc_device_server library dependencies into a variable, so that it can be easily passed along to the test targets.
  8. suppress protobuf installation in SM:
    Set protobuf_INSTALL=OFF, which suppresses the protobuf installation codepaths - that we don't want to use anyway and
    which cause the failure.
  9. add necessary gRPC dep to ni_grpc_device_server:
    ni_grpc_device_server must be linked against libgrpc, as well as the grpccpp libs.
  10. fixup venv codegen deps:
    Give the codegen targets a dependency on the python virtualenv via the all_codegen_dependencies variable.
  11. Enforce consistent runtime library settings in case of MSVC:
    This is needed to fix Linker issues while building the binaries
  12. Do a clean cmake build everytime:
    It is preferable to do a clean build to ensure all object files are deleted and then rebuilt from scratch, which can help
    resolve inconsistencies and errors.

NILRT 11 will ship with grpc >1.60 and protobuf >v25.2.
grpc-device doesn't compile with these toolchain versions,
throwing errors about undefined symbols. Downgrading grpc
back to 1.51 (with python3-grpcio and protobuf recipes)
successfully works around these issues. But for security and
general currency reasons, we cannot afford to ship NILRT 11
with these downgrades.
Current CMakeLists.txt is limited to some of the bitbake
functionalities which makes it difficult to build with new changes.

Changes in this commit will make sure NILRT 11 compiles grpc-device
with the latest/upgraded grpc version without affecting the existing
build process.

Changes:
1. refactor toolchain link logic
   - Deprecate the CMAKE_CROSSCOMPILING variable, in favor of
     USE_SUBMODULE_LIBS cmake option. Refactor the linking logic to
     be a consolidation of all the linking actions from across the
     file, and to better support builds in generic linux environments.
2. fixup utf8cpp library link
   - The utf8cpp cmake library namespace is incorrectly identified
     as 'utf8cpp', instead of the proper 'utf8cpp:utf8cpp'. As a
     result, cmake does not link the utf8.h header and compilation
     fails.
3. parameterize python3 venv
   - Create a USE_PYTHON_VIRTUALENV cmake option. When asserted, it
     will add the bespoke venv to the toolchain. Otherwise, the
     cmake config will use the system python environment.
4. link the device server to grpc_gpr
   - ni_grpc_device_server target depends on symbols from  grpc
     gpr.so, namely gpr_log. Add grpc_gpr to link libraries for
     ni_grpc_device_server.
5. add abseil_sync dep to server target
   - ni_grpc_device_server uses symbology from libabsl_synchronization
     library. Add a library dependency to reflect that relationship.
6. add utf8cpp dep to IntegrationTestsRunner
   - The IntegrationTestsRunner depends on utf8.h header indirectly,
     via its access to the device server source.
7. fill out target lib deps
   - Shove ni_grpc_device_server library dependencies into a variable,
     so that it can be easily passed along to the test targets.
8. suppress protobuf installation in SM
   - Set protobuf_INSTALL=OFF, which suppresses the protobuf
     installation codepaths - that we don't want to use anyway and
     which cause the failure.
9. add necessary gRPC dep to ni_grpc_device_server
   - ni_grpc_device_server must be linked against libgrpc, as well as
     the grpccpp libs.
10.fixup venv codegen deps
   - Give the codegen targets a dependency on the python virtualenv
     via the all_codegen_dependencies variable.

Signed-off-by: Rajendra Desai <rajendra.desai@ni.com>
Copy link
Collaborator

@astarche astarche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me. I appreciate the thoroughness and clarity.

I would like @reckenro and @maxxboehme to review if possible.

It looks like you have some errors in the windows build.

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@reckenro reckenro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me besides sorting out the build failures

…d fixed a small typo

Signed-off-by: Rajendra Desai <rajendra.desai@ni.com>
Signed-off-by: Rajendra Desai <rajendra.desai@ni.com>
MSVC_RUNTIME_LIBRARY property) which is supported on cmake version
>3.15 in case of MSVC compiler

Signed-off-by: Rajendra Desai <rajendra.desai@ni.com>
Copy link
Collaborator

@maxxboehme maxxboehme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. Will wait for final review when windows build has been fixed.

CMakeLists.txt Show resolved Hide resolved
@rajendra-desai-ni
Copy link
Contributor Author

I tried doing a windows build locally with the existing changes to debug the pipeline failures that I am getting for Windows build, but I am not able to reproduce those linker errors that are being reported in the pipeline logs. I am able to build and run the executables (IntegrationTestsRunner.exe, ni_grpc_device_server.exe, SystemTestsRunner.exe, UnitTestsRunner.exe) successfully.
The cmake version on my system is 3.27.6, not sure if this is the reason why it is working on my machine. But the CMAKE_CXX_FLAGS that has been set should have worked.

@maxxboehme @astarche @reckenro @bkeryan: Is there anything more I can try out? Or am I missing something?

@reckenro
Copy link
Collaborator

reckenro commented Aug 26, 2024

I tried doing a windows build locally with the existing changes to debug the pipeline failures that I am getting for Windows build, but I am not able to reproduce those linker errors that are being reported in the pipeline logs. I am able to build and run the executables (IntegrationTestsRunner.exe, ni_grpc_device_server.exe, SystemTestsRunner.exe, UnitTestsRunner.exe) successfully. The cmake version on my system is 3.27.6, not sure if this is the reason why it is working on my machine. But the CMAKE_CXX_FLAGS that has been set should have worked.

@maxxboehme @astarche @reckenro @bkeryan: Is there anything more I can try out? Or am I missing something?

@rajendra-desai-ni , this is my stab at a guess or at least where I'd look.

I'm guessing it has to do with the lines about
set_property(TARGET UnitTestsRunner PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>").

I say that because that seems to indicate statically linked according to the documentation while in the CMAKE < 3.15 case it's using set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /MDd") which the /MDd means dynamically linked.

That's where I'd look into at least. I say this all with little experience debugging the CMAKE builds and no idea what the desired settings are in this case.

@bkeryan
Copy link
Contributor

bkeryan commented Aug 26, 2024

I tried doing a windows build locally with the existing changes to debug the pipeline failures that I am getting for Windows build, but I am not able to reproduce those linker errors that are being reported in the pipeline logs. I am able to build and run the executables (IntegrationTestsRunner.exe, ni_grpc_device_server.exe, SystemTestsRunner.exe, UnitTestsRunner.exe) successfully. The cmake version on my system is 3.27.6, not sure if this is the reason why it is working on my machine. But the CMAKE_CXX_FLAGS that has been set should have worked.
@maxxboehme @astarche @reckenro @bkeryan: Is there anything more I can try out? Or am I missing something?

@rajendra-desai-ni , this is my stab at a guess or at least where I'd look.

I'm guessing it has to do with the lines about set_property(TARGET UnitTestsRunner PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>").

I say that because that seems to indicate statically linked according to the documentation while in the CMAKE < 3.15 case it's using set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /MDd") which the /MDd means dynamically linked.

That's where I'd look into at least. I say this all with little experience debugging the CMAKE builds and no idea what the desired settings are in this case.

It seems that all of the errors are about gtest.lib and gmock.lib. Perhaps the issue is related to gtest_force_shared_crt, which is supposed to prevent gtest from replacing /MD with /MT. Does this need to be set before you add the third_party/gtest dir?

@rajendra-desai-ni
Copy link
Contributor Author

I tried doing a windows build locally with the existing changes to debug the pipeline failures that I am getting for Windows build, but I am not able to reproduce those linker errors that are being reported in the pipeline logs. I am able to build and run the executables (IntegrationTestsRunner.exe, ni_grpc_device_server.exe, SystemTestsRunner.exe, UnitTestsRunner.exe) successfully. The cmake version on my system is 3.27.6, not sure if this is the reason why it is working on my machine. But the CMAKE_CXX_FLAGS that has been set should have worked.
@maxxboehme @astarche @reckenro @bkeryan: Is there anything more I can try out? Or am I missing something?

@rajendra-desai-ni , this is my stab at a guess or at least where I'd look.

I'm guessing it has to do with the lines about set_property(TARGET UnitTestsRunner PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>").

I say that because that seems to indicate statically linked according to the documentation while in the CMAKE < 3.15 case it's using set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /MDd") which the /MDd means dynamically linked.

That's where I'd look into at least. I say this all with little experience debugging the CMAKE builds and no idea what the desired settings are in this case.

@reckenro, Thanks for the reply, I tried doing a static linking to the project files since we are building the exe's, but even that doesn't seem to be working. It looks like the other workarounds doesn't seem to work as well :(

@rajendra-desai-ni
Copy link
Contributor Author

I tried doing a windows build locally with the existing changes to debug the pipeline failures that I am getting for Windows build, but I am not able to reproduce those linker errors that are being reported in the pipeline logs. I am able to build and run the executables (IntegrationTestsRunner.exe, ni_grpc_device_server.exe, SystemTestsRunner.exe, UnitTestsRunner.exe) successfully. The cmake version on my system is 3.27.6, not sure if this is the reason why it is working on my machine. But the CMAKE_CXX_FLAGS that has been set should have worked.
@maxxboehme @astarche @reckenro @bkeryan: Is there anything more I can try out? Or am I missing something?

@rajendra-desai-ni , this is my stab at a guess or at least where I'd look.
I'm guessing it has to do with the lines about set_property(TARGET UnitTestsRunner PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>").
I say that because that seems to indicate statically linked according to the documentation while in the CMAKE < 3.15 case it's using set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /MDd") which the /MDd means dynamically linked.
That's where I'd look into at least. I say this all with little experience debugging the CMAKE builds and no idea what the desired settings are in this case.

It seems that all of the errors are about gtest.lib and gmock.lib. Perhaps the issue is related to gtest_force_shared_crt, which is supposed to prevent gtest from replacing /MD with /MT. Does this need to be set before you add the third_party/gtest dir?

@bkeryan, I am not sure if setting gtest_force_shared_crt before adding the third_party/gtest dir would make any difference as we are anyways setting it before building the binaries or linking the files. Please correct me if I am wrong here.
I also tried other workarounds but nothing seems to be working. Please let me know if you have any more suggestions.

@bkeryan
Copy link
Contributor

bkeryan commented Aug 30, 2024

It seems that all of the errors are about gtest.lib and gmock.lib. Perhaps the issue is related to gtest_force_shared_crt, which is supposed to prevent gtest from replacing /MD with /MT. Does this need to be set before you add the third_party/gtest dir?

@bkeryan, I am not sure if setting gtest_force_shared_crt before adding the third_party/gtest dir would make any difference as we are anyways setting it before building the binaries or linking the files. Please correct me if I am wrong here. I also tried other workarounds but nothing seems to be working. Please let me know if you have any more suggestions.

@rajendra-desai-ni I don't know the answer. That's why I asked the question.

Did you try moving this variable above the add_subdirectory? The old code did it in this order:

  set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
  add_subdirectory(third_party/gtest ${CMAKE_CURRENT_BINARY_DIR}/gtest EXCLUDE_FROM_ALL)

I can't find this variable in your latest changes. Did you remove it?

Edit: see https://stackoverflow.com/questions/6891447/why-is-a-variable-value-not-available-after-add-subdirectory-ing-a-cmakelists-tx

@rajendra-desai-ni
Copy link
Contributor Author

@rajendra-desai-ni I don't know the answer. That's why I asked the question.

Did you try moving this variable above the add_subdirectory? The old code did it in this order:

  set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
  add_subdirectory(third_party/gtest ${CMAKE_CURRENT_BINARY_DIR}/gtest EXCLUDE_FROM_ALL)

I can't find this variable in your latest changes. Did you remove it?

Edit: see https://stackoverflow.com/questions/6891447/why-is-a-variable-value-not-available-after-add-subdirectory-ing-a-cmakelists-tx

@bkeryan: I was trying out other workarounds, so I had removed the code. I reverted those and tried your suggested changes and I am now able to get the windows pipeline building. Thanks for the help!!

@rajendra-desai-ni
Copy link
Contributor Author

@bkeryan @reckenro @maxxboehme @astarche : All the required checks are now successful. Please feel free to re-review the changes as there were many commits and comments to fix the failing windows pipeline.

@rajendra-desai-ni
Copy link
Contributor Author

@maxxboehme @astarche @reckenro @bkeryan : It looks like I have got all necessary approvals for this PR. Can one of you please merge this PR so that I can go ahead and enable the package from NILRT side? Or is this PR merge self-doable and if so, do I need to squash and merge?
Cc: @amstewart @prasanna-nib

@maxxboehme maxxboehme merged commit 0d1658e into ni:main Sep 11, 2024
9 of 11 checks passed
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.

6 participants