-
Notifications
You must be signed in to change notification settings - Fork 23
RSDK-10366 - rust utils for windows #416
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
Changes from 87 commits
e9d3ea1
0e3b8af
b968803
5d5bb2d
d9827e5
adff932
59fd83c
a8e1751
ff01f42
96652c8
46dad50
1d1ad1e
3268789
57e6d5d
cb01ea7
5cf9590
ff5a2bf
bc83e5b
674e729
952c305
7e38e1b
9eab8c5
d590eff
2d7df41
f299251
9e36c2f
02660a3
6728c57
38667a8
ae64216
1dd810f
15abea3
d6982be
11e56e9
b1e8526
e21ba06
c7490e2
758bacf
7e0953c
c98af91
72004be
412cbb9
73c5410
b4c4b84
de30df1
0b2512b
4c29471
add6916
d4cb253
89df7fb
ba316a3
f1ba0d3
07470b2
3ee79d5
6e77560
59ed94e
0f5812a
c66a2e0
51de5d8
a9b2337
7afa451
1bd47b2
d43865b
b3a339a
4b68fda
952702b
f1c1e4f
ec59387
5e15a93
e5a3a5a
b843938
861a35c
6306025
9de2913
2779b50
1e042cf
c6382e1
c4143af
165ca0c
d292417
9095b79
84b1874
bfc6faf
4ba53d5
7cb2dd7
3186390
3d56ccc
b710c7a
47aca71
b56ab5c
3693c60
9459e17
230b29d
cd7690b
fa063e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,39 @@ jobs: | |
github.event_name == 'schedule' && | ||
steps.git_info.outputs.current_commit == steps.last_successful_commit.outputs.commit-hash | ||
|
||
# TODO (RSDK-10666) Uncomment and fix | ||
#build_windows: | ||
#if: github.repository_owner == 'viamrobotics' | ||
#needs: [prepare] | ||
#runs-on: windows-latest | ||
#strategy: | ||
#fail-fast: false | ||
#matrix: | ||
#include: | ||
#- target: x86_64-windows | ||
#platform: windows_x86_64 | ||
#steps: | ||
#- name: Checkout Code | ||
#uses: actions/checkout@v4 | ||
|
||
#- name: Install dependencies | ||
#uses: crazy-max/ghaction-chocolatey@v3 | ||
#with: | ||
#args: install -y conan git | ||
|
||
#- name: Create package | ||
#shell: powershell | ||
#run: | | ||
#Import-Module $env:ChocolateyInstall\helpers\chocolateyProfile.psm1 | ||
#refreshenv | ||
#conan profile detect | ||
#conan create . ` | ||
#--build=missing ` | ||
#-o "&:shared=False" | ||
#env: | ||
#CONAN_USER_HOME: c:/cache | ||
#CONAN_USER_HOME_SHORT: c:/cache/conan_shortpaths | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure when we'll get around to fixing this so I don't know that I feel quite as strongly about leaving this in for future reference. Happy to just remove if preferred. We can definitely try and fix now, but unless someone has a very clear picture of what's wrong I think I'd prefer to do that in a separate PR so we can lock in the wins here now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think maybe just leave in the TODO/ticket number but the commented code can be removed, we can always consult this diff to see what you had written I believe |
||
|
||
build_macos: | ||
if: github.repository_owner == 'viamrobotics' | ||
needs: [prepare] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,54 @@ jobs: | |
uses: andymckay/[email protected] | ||
if: steps.release_exists.outputs.id != '' | ||
|
||
build_windows: | ||
if: github.repository_owner == 'viamrobotics' | ||
needs: [prepare] | ||
runs-on: windows-latest | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
include: | ||
- target: x86_64-windows | ||
platform: windows_x86_64 | ||
steps: | ||
- name: Checkout Code | ||
uses: actions/checkout@v4 | ||
with: | ||
ref: ${{ needs.prepare.outputs.sha }} | ||
|
||
- name: Install dependencies | ||
uses: crazy-max/ghaction-chocolatey@v3 | ||
with: | ||
args: install -y conan git | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still surprised you need this. Chocolatey is pre-installed on windows builders, from what I know. The tflite cpu build uses choco to install conan without doing anything special to install choco itself: https://github.com/viam-modules/mlmodel-tflite/blob/main/bin/setup.ps1#L5 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah you're probably right that it's not necessary! I kept it in just because we needed to do a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without knowing exactly what it does, it likely is innocuous until "crazy max"'s repo ends up being a vector for / victim of a supply chain attack. At least when run on windows, all it actually seems to do is call https://github.com/crazy-max/ghaction-chocolatey/blob/master/src/main.ts#L22 So, from a risk / reward ratio it looks to me like it shouldn't stay. However, I'm fine with leaving it in for now since I know there has been a lot of back and forth on this and testing is slow. If you end up leaving it in, could you please file a ticket to remove it and add a TODO here before merging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh that's a good point! this is reason enough to just make the change now imo, tested and it works. |
||
|
||
- name: Create package | ||
shell: powershell | ||
run: | | ||
Import-Module $env:ChocolateyInstall\helpers\chocolateyProfile.psm1 | ||
refreshenv | ||
conan profile detect | ||
conan install . --output-folder=build-conan --build=missing -o "&:shared=False" | ||
acmorrow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cmake . --preset conan-default | ||
cmake --build --preset=conan-release --target ALL_BUILD install -j 8 | ||
Comment on lines
+66
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious: but why direct invocation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reasons that weren't entirely clear, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to remember if we were in fact using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recall having issues with both, but there was so much back and forth and problem solving that it's hard for me to speak to with total confidence. at any rate I think this seems like a reasonable thing to revisit in the above-linked ticket. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe throw a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done! |
||
env: | ||
CONAN_USER_HOME: c:/cache | ||
CONAN_USER_HOME_SHORT: c:/cache/conan_shortpaths | ||
|
||
- name: Copy | ||
run: | | ||
cmake --install build-conan/build --prefix builds/viam-cpp-sdk-${{ matrix.platform }} | ||
|
||
- name: Create tar | ||
run: | | ||
tar -czvf builds/viam-cpp-sdk-${{ matrix.platform }}.tar.gz builds/viam-cpp-sdk-${{ matrix.platform }} | ||
|
||
- name: Upload artifacts | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: viam-cpp-sdk-${{ matrix.platform }}.tar.gz | ||
path: builds/viam-cpp-sdk-${{ matrix.platform }}.tar.gz | ||
|
||
build_macos: | ||
if: github.repository_owner == 'viamrobotics' | ||
needs: [prepare] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -266,14 +266,20 @@ if (viam_rust_utils_files) | |
${viam_rust_utils_file} | ||
ONLY_IF_DIFFERENT | ||
) | ||
elseif(NOT WIN32) # TODO(RSDK-10366): Currently, rust_utils is not published for windows, so don't even try downloading | ||
elseif(NOT WIN32 OR ${CMAKE_SYSTEM_PROCESSOR} STREQUAL "AMD64") | ||
set(lvru_system_name ${CMAKE_SYSTEM_NAME}) | ||
if (CMAKE_SYSTEM_NAME STREQUAL "Darwin") | ||
set(lvru_system_name "macosx") | ||
endif() | ||
|
||
set(lvru_system_processor ${CMAKE_SYSTEM_PROCESSOR}) | ||
if(WIN32) | ||
set(lvru_system_processor "x86_64") | ||
acmorrow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
endif() | ||
|
||
file( | ||
DOWNLOAD | ||
https://github.com/viamrobotics/rust-utils/releases/latest/download/${CMAKE_SHARED_LIBRARY_PREFIX}viam_rust_utils-${lvru_system_name}_${CMAKE_SYSTEM_PROCESSOR}${CMAKE_STATIC_LIBRARY_SUFFIX} | ||
https://github.com/viamrobotics/rust-utils/releases/latest/download/${CMAKE_SHARED_LIBRARY_PREFIX}viam_rust_utils-${lvru_system_name}_${lvru_system_processor}${CMAKE_STATIC_LIBRARY_SUFFIX} | ||
${viam_rust_utils_file} | ||
STATUS lvru_status | ||
) | ||
|
@@ -283,24 +289,21 @@ elseif(NOT WIN32) # TODO(RSDK-10366): Currently, rust_utils is not published for | |
if(NOT lvru_status_code EQUAL 0) | ||
message(FATAL_ERROR "No local viam_rust_utils found and failed to download: ${lvru_status_string}") | ||
endif() | ||
|
||
else() | ||
message(WARNING "Currently running on Windows with no rust-utils file. Module code should work as expected, but client code may fail unexpectedly.") | ||
endif() | ||
|
||
# TODO(RSDK-10366): Currently, rust_utils is not published for windows, so don't even declare the library | ||
if (NOT WIN32) | ||
add_library(viam_rust_utils SHARED IMPORTED) | ||
|
||
if (NOT WIN32 OR ${CMAKE_SYSTEM_PROCESSOR} STREQUAL "AMD64") | ||
add_library(viam_rust_utils STATIC IMPORTED) | ||
target_link_directories(viam_rust_utils | ||
INTERFACE | ||
"$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies: I've changed my mind here, I think you still do need |
||
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_LIBDIR}>" | ||
) | ||
|
||
set_property(TARGET viam_rust_utils PROPERTY IMPORTED_LOCATION ${viam_rust_utils_file}) | ||
|
||
install( | ||
IMPORTED_RUNTIME_ARTIFACTS viam_rust_utils | ||
LIBRARY COMPONENT viam-cpp-sdk_runtime | ||
FILES ${viam_rust_utils_file} DESTINATION ${CMAKE_INSTALL_LIBDIR} COMPONENT viam-cpp-sdk_runtime | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @acmorrow fwiw it appears that this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. I may try it out myself after this merges to understand better, because it doesn't align with my understanding of how this ought to be working. |
||
) | ||
endif() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,10 +91,7 @@ def package(self): | |
|
||
def package_info(self): | ||
|
||
# TODO(RSDK-10366): Currently, rust_utils is not published for windows | ||
# and the C++ SDK just doesn't include it as a dependency on that platform | ||
if not self.settings.os == "Windows": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lia-viam can you help me understand what this is doing? Based on testing on the windows laptop, it doesn't seem to matter whether we run this always or not. Same below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://docs.conan.io/2/examples/conanfile/package_info/components.html for more info, but basically we are telling conan that the SDK depends on a library called |
||
self.cpp_info.components["viam_rust_utils"].libs = ["viam_rust_utils"] | ||
self.cpp_info.components["viam_rust_utils"].libs = ["viam_rust_utils"] | ||
|
||
self.cpp_info.components["viamsdk"].libs = ["viamsdk"] | ||
|
||
|
@@ -137,11 +134,8 @@ def package_info(self): | |
"viamapi", | ||
]) | ||
|
||
# TODO(RSDK-10366): Currently, rust_utils is not published for windows | ||
# and the C++ SDK just doesn't include it as a dependency on that platform | ||
if self.settings.os != "Windows": | ||
self.cpp_info.components["viamsdk"].requires.extend([ | ||
"viam_rust_utils" | ||
]) | ||
self.cpp_info.components["viamsdk"].requires.extend([ | ||
"viam_rust_utils" | ||
]) | ||
|
||
self.cpp_info.components["viamsdk"].frameworks = ["Security"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -377,7 +377,7 @@ int main(int argc, char* argv[]) try { | |
}); | ||
|
||
// Print out the top 5 (or fewer) label/score pairs. | ||
for (size_t i = 0; i != std::min(5UL, scored_labels.size()); ++i) { | ||
for (size_t i = 0; i != std::min<std::size_t>(5UL, scored_labels.size()); ++i) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the explicit std::size_t? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On windows specifically, it was necessary to specify the type. Otherwise, there was a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @acmorrow since the template wants both its arguments to be the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or just make the 5 a size_t? I'd expect |
||
// TODO: Avoid hardcoding the width here. | ||
VIAM_SDK_LOG(info) << boost::format("%1%: %2% %|40t|%3%\n") % i % | ||
*scored_labels[i].label % scored_labels[i].score; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,12 +30,14 @@ class Gizmo : public Component { | |
explicit Gizmo(std::string name); | ||
}; | ||
|
||
namespace viam::sdk { | ||
namespace viam { | ||
namespace sdk { | ||
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make compiles on C++14. |
||
template <> | ||
struct API::traits<Gizmo> { | ||
static ::viam::sdk::API api(); | ||
}; | ||
} // namespace viam::sdk | ||
} // namespace sdk | ||
} // namespace viam | ||
|
||
// `GizmoClient` is the gRPC client implementation of a `Gizmo` component. | ||
class GizmoClient : public Gizmo { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,12 +29,14 @@ class Summation : public Service { | |
explicit Summation(std::string name); | ||
}; | ||
|
||
namespace viam::sdk { | ||
namespace viam { | ||
namespace sdk { | ||
Comment on lines
+32
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make compile on C++14 |
||
template <> | ||
struct API::traits<Summation> { | ||
static API api(); | ||
}; | ||
} // namespace viam::sdk | ||
} // namespace sdk | ||
} // namespace viam | ||
|
||
// `SummationClient` is the gRPC client implementation of a `Summation` | ||
// service. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -276,10 +276,10 @@ target_link_libraries(viamsdk | |
PRIVATE Threads::Threads | ||
) | ||
|
||
# TODO(RSDK-10366): Currently, rust_utils is not published for | ||
# windows, so don't link to it. Instead, link a stub implementation | ||
# that just calls `abort`. | ||
if (NOT WIN32) | ||
# rust-utils doesn't build in CI on windows other than x86_64, so confirm that we're either not | ||
# windows, or if we are windows that we're AMD64, or else that the user has provided their own | ||
# build of rust_utils. | ||
if (NOT WIN32 OR num_viam_rust_utils_files GREATER 0 OR ${CMAKE_SYSTEM_PROCESSOR} STREQUAL "AMD64") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this test just look only at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be wrong, but is this a correct assumption? I thought There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, you are right, I got myself tangled up. I was reacting to having complex logic to decide whether to use rust utils or build the stub here. It just feels like it would be better if all of that tortured logic was isolated up in the top level CMakeLists.txt and this file had a way of making a simple yes/no decision about whether to use the library or build the stubs. Actually, I wonder if Again, I know you are eager to land this, but could you please add a TODO (doesn't need a ticket, leave it for someone to clean up later). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. went ahead and switched to |
||
target_link_libraries(viamsdk | ||
PRIVATE viam_rust_utils | ||
) | ||
|
@@ -289,9 +289,14 @@ endif() | |
|
||
if (APPLE) | ||
target_link_libraries(viamsdk PUBLIC "-framework Security") | ||
else() | ||
elseif (NOT WIN32) | ||
acmorrow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
target_link_libraries(viamsdk PRIVATE dl) | ||
target_link_libraries(viamsdk PRIVATE rt) | ||
else() | ||
target_link_libraries(viamsdk INTERFACE Ncrypt.lib) | ||
target_link_libraries(viamsdk INTERFACE Secur32.lib) | ||
target_link_libraries(viamsdk INTERFACE Ntdll.lib) | ||
target_link_libraries(viamsdk INTERFACE Userenv.lib) | ||
endif() | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -299,7 +299,8 @@ ::grpc::Status MockRobotService::FrameSystemConfig( | |
const ::viam::robot::v1::FrameSystemConfigRequest*, | ||
::viam::robot::v1::FrameSystemConfigResponse* response) { | ||
auto client_md = context->client_metadata(); | ||
if (auto client_info = client_md.find("viam_client"); client_info == client_md.end()) { | ||
auto client_info = client_md.find("viam_client"); | ||
if (client_info == client_md.end()) { | ||
Comment on lines
+302
to
+303
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make compile on C++14 |
||
return ::grpc::Status(::grpc::StatusCode::FAILED_PRECONDITION, | ||
"viam_client info not properly set in metadata"); | ||
} | ||
|
@@ -314,7 +315,8 @@ ::grpc::Status MockRobotService::TransformPose(::grpc::ServerContext* context, | |
const ::viam::robot::v1::TransformPoseRequest*, | ||
::viam::robot::v1::TransformPoseResponse* response) { | ||
auto client_md = context->client_metadata(); | ||
if (auto client_info = client_md.find("viam_client"); client_info == client_md.end()) { | ||
auto client_info = client_md.find("viam_client"); | ||
if (client_info == client_md.end()) { | ||
return ::grpc::Status(::grpc::StatusCode::FAILED_PRECONDITION, | ||
"viam_client info not properly set in metadata"); | ||
} | ||
|
@@ -327,7 +329,8 @@ ::grpc::Status MockRobotService::GetMachineStatus( | |
const ::viam::robot::v1::GetMachineStatusRequest*, | ||
::viam::robot::v1::GetMachineStatusResponse* response) { | ||
auto client_md = context->client_metadata(); | ||
if (auto client_info = client_md.find("viam_client"); client_info == client_md.end()) { | ||
auto client_info = client_md.find("viam_client"); | ||
if (client_info == client_md.end()) { | ||
return ::grpc::Status(::grpc::StatusCode::FAILED_PRECONDITION, | ||
"viam_client info not properly set in metadata"); | ||
} | ||
|
@@ -341,7 +344,8 @@ ::grpc::Status MockRobotService::GetOperations(::grpc::ServerContext* context, | |
const ::viam::robot::v1::GetOperationsRequest*, | ||
::viam::robot::v1::GetOperationsResponse* response) { | ||
auto client_md = context->client_metadata(); | ||
if (auto client_info = client_md.find("viam_client"); client_info == client_md.end()) { | ||
auto client_info = client_md.find("viam_client"); | ||
if (client_info == client_md.end()) { | ||
return ::grpc::Status(::grpc::StatusCode::FAILED_PRECONDITION, | ||
"viam_client info not properly set in metadata"); | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.