-
Notifications
You must be signed in to change notification settings - Fork 26
[RSDK-10385] Windows build system improvements and rust_utils workarounds #402
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
Conversation
| @@ -1,3 +1,9 @@ | |||
| #ifdef _WIN32 | |||
| #define NOMINMAX | |||
| #include <winsock2.h> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this. I'm surprised the gRPC headers don't handle this for themselves, but without this this file doesn't compile.
The right way to do this would be with a global config precompiled header that got pulled into every .cpp we have, but I don't want to spend time on that right now, so I will file a follow-up ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/viam/sdk/CMakeLists.txt
Outdated
| rpc/dial.cpp | ||
| rpc/server.cpp | ||
| rpc/private/viam_grpc_channel.cpp | ||
| rpc/private/viam_rust_utils_stubs.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of doing an #if WIN32 in the cpp file, what if we did this at the CMake level:
if (NOT WIN32)
target_link_libraries(viamsdk
PRIVATE viam_rust_utils
)
else()
target_sources(viamsdk PRIVATE rust_utils_stubs.cpp)
endif()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll give that a try, and as long as it works (which I expect it will) I'll merge with that.
lia-viam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks fine assuming the main tests pass. Conan is failing but it's only as broken as it already was on main (which seems to have gotten worse recently) so not a blocker for merging.
Probably want to look into github CI for the conan package at some point however
__cplusplusis set right on Windows builds.viam_rust_utilson Windows.viam_rust_utilsentry points toaborton Windows.viamapilibrary name on Windows and apply/WHOLEARCHIVEstd::invoke_result_tinstead ofresult_ofin C++17, where the latter is deprecated