RSDK-10366 - rust utils for windows #416

stuqdog · 2025-04-18T15:03:50Z

Adds support for tcp-based rust-utils, updates rust-utils downloading for x86_64 windows.

Tested tcp connection on mac successfully.

Tested on windows: with a properly built viam_rust_utils.lib at the root of the directory, I am able to connect as a client via webRTC. Without a viam_rust_utils.lib at the root of the directory, I instead link in the stubs that were previously being linked, and abort() out whenever making a rust-utils call (as we were doing previously). edit: with the recent update to rust-utils, we now successfully download rust-utils if it's not in the root, so long as we're on x86_64.

stuqdog · 2025-04-18T15:06:26Z

CMakeLists.txt

+elseif(NOT WIN32 OR (CMAKE_SYSTEM_PROCESSOR STREQUAL "X86") OR (CMAKE_SYSTEM_PROCESSOR STREQUAL "X64")) # TODO(RSDK-10366): Currently, rust_utils is not published for windows aarch, so don't even try downloading
  set(lvru_system_name ${CMAKE_SYSTEM_NAME})
+  if (CMAKE_SYSTEM_NAME STREQUAL "Cygwin")
+    set(lvru_system_name "windows")
+  endif()
  if (CMAKE_SYSTEM_NAME STREQUAL "Darwin")
    set(lvru_system_name "macosx")
  endif()
+  set(lvru_system_processor ${CMAKE_SYSTEM_PROCESSOR})
+  if ((CMAKE_SYSTEM_PROCESSOR STREQUAL "X86") OR (CMAKE_SYSTEM_PROCESSOR STREQUAL "X64"))
+    set(lvru_system_processor "x86_64")
+  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}


This should hopefully download rust-utils on windows systems when building the SDK. However, it is currently untested. When I next have access to a windows laptop I will test and confirm. If it doesn't work and is not trivial to get working, I will remove all of this and update the README with instructions on building from source.

It is reasonably easy to spin up a windows machine in GCP and RDP into it if you just need a quick test.

acmorrow · 2025-04-18T15:36:04Z

src/viam/sdk/rpc/dial.cpp

+    std::string address;
+    if (tcp_check.first != localhost_prefix.end()) {
+        // proxy path is not a localhost address and is therefore a UDS socket
+        address += "unix://";


In 62d40eb#diff-03661151ee3ff17fde17964451d2ea2fbd2322f2bd21f4851eade22a2aaad233R254, I found that using unix: enabled Windows support, while I couldn't get anything with unix:// to work on windows. Per the gRPC docs, the only difference between the two is that the latter form with unix:// is restricted to absolute paths, while unix: will accept either: https://grpc.github.io/grpc/cpp/md_doc_naming.html.

It might be worth considering just making the switch to unix: here now, in case client usage on Windows is ever required, this hard to find fact will not need to be rediscovered.

Yeah good call, I'm happy to change this to unix:.

fwiw this PR is actually enabling windows client usage! The restriction was UDS support in tokio, but now that rust-utils will be using TCP sockets instead on windows, this PR enables us to use rust-utils on windows. but this change definitely still seems good, especially if tokio ever starts supporting UDS on windows.

acmorrow · 2025-04-18T15:37:06Z

src/viam/sdk/rpc/dial.cpp

        uri, entity, type, payload, opts.allows_insecure_downgrade(), float_timeout.count(), ptr);
-    if (socket_path == NULL) {
+    if (proxy_path == NULL) {


nullptr is preferred, or just invert the check: if (!proxy_path) {

acmorrow · 2025-04-18T15:44:14Z

src/viam/sdk/rpc/dial.cpp

        free_rust_runtime(ptr);
        throw Exception(ErrorCondition::k_connection, "Unable to establish connecting path");
    }

-    std::string address("unix://");
-    address += socket_path;
+    std::string localhost_prefix("127.0.0.1");


There are other forms of localhost, like ::1. Probably not important?

Yeah, the address we get back from rust-utils is consistently of the form 127.0.0.1:{port} so I opted for that.

acmorrow · 2025-04-18T15:45:00Z

src/viam/sdk/rpc/dial.cpp

+    auto tcp_check = std::mismatch(localhost_prefix.begin(), localhost_prefix.end(), proxy_path);
+    std::string address;
+    if (tcp_check.first != localhost_prefix.end()) {
+        // proxy path is not a localhost address and is therefore a UDS socket


S socket is redundant. Maybe better to say it fully as unix domain socket (UDS).

acmorrow · 2025-04-18T15:48:53Z

src/viam/sdk/rpc/dial.cpp

-    std::string address("unix://");
-    address += socket_path;
+    std::string localhost_prefix("127.0.0.1");
+    auto tcp_check = std::mismatch(localhost_prefix.begin(), localhost_prefix.end(), proxy_path);


Why std::mismatch over just std::string::find? and comparing to npos? Is this doing something more than just checking for "127.0.0.1" as a substring?

no good reason why! I asked the internet how to find a substring in C++ and somehow the first thing I landed on was an SO post recommending std::mismatch 😅 I'll switch to std::string::find.

acmorrow · 2025-04-18T15:50:48Z

CMakeLists.txt

  set(lvru_system_name ${CMAKE_SYSTEM_NAME})
+  if (CMAKE_SYSTEM_NAME STREQUAL "Cygwin")
+    set(lvru_system_name "windows")
+  endif()


Maybe elseif here?

acmorrow · 2025-04-18T15:51:39Z

CMakeLists.txt

+elseif(NOT WIN32 OR (CMAKE_SYSTEM_PROCESSOR STREQUAL "X86") OR (CMAKE_SYSTEM_PROCESSOR STREQUAL "X64")) # TODO(RSDK-10366): Currently, rust_utils is not published for windows aarch, so don't even try downloading
  set(lvru_system_name ${CMAKE_SYSTEM_NAME})
+  if (CMAKE_SYSTEM_NAME STREQUAL "Cygwin")
+    set(lvru_system_name "windows")
+  endif()
  if (CMAKE_SYSTEM_NAME STREQUAL "Darwin")
    set(lvru_system_name "macosx")
  endif()
+  set(lvru_system_processor ${CMAKE_SYSTEM_PROCESSOR})
+  if ((CMAKE_SYSTEM_PROCESSOR STREQUAL "X86") OR (CMAKE_SYSTEM_PROCESSOR STREQUAL "X64"))
+    set(lvru_system_processor "x86_64")
+  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}


It is reasonably easy to spin up a windows machine in GCP and RDP into it if you just need a quick test.

acmorrow · 2025-04-18T15:52:09Z

src/viam/sdk/rpc/dial.cpp

@@ -1,3 +1,4 @@
+#include <algorithm>


This should be down with the other C++ standard library includes around line 4-5

acmorrow

LGTM!

lia-viam · 2025-05-08T18:36:44Z

.github/workflows/conan.yml

-          conan create . `
-            --build=missing `
-            -o "&:shared=False"
+          cmake . --preset conan-default


rehashing an offline discussion, this type of invocation is suitable for release.yml but with conan.yml we do want to test that we can get a successful conan create, because that packages and tests the SDK conan package

stuqdog · 2025-05-09T12:35:28Z

.github/workflows/conan.yml

+  # 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


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.

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

lia-viam · 2025-05-09T14:35:51Z

.github/workflows/conan.yml

+  # 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


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

lia-viam · 2025-05-09T14:36:29Z

oh also were you able to run the release pipeline using this updated yaml?

stuqdog · 2025-05-09T14:46:52Z

oh also were you able to run the release pipeline using this updated yaml?

More or less! I didn't try running the release pipeline because it involves updating version number and I didn't want to do that and change the rules for when/how the release workflow runs and so on, but the release pipeline as it's written now was run under the Conan script to make sure it ran successfully, and I confirmed that cmake_install.cmake does exist at the build-conan/build path that we try to install from.

acmorrow

If this is working I think you should merge it more or less as is, though I have a few comments and questions here and there that I'd appreciate your taking a look at. I don't think any of them amount to something requiring another round of review.

acmorrow · 2025-05-07T18:28:12Z

.github/workflows/conan.yml

+          conan create . `
+            --build=missing `
+            -o "&:shared=False" `
+            -s:h compiler.cppstd=17 `
+            -s:h compiler.runtime=static


I expect you need to keep:

--build=missing ` -o "&:shared=False" `

acmorrow · 2025-05-14T18:13:31Z

.github/workflows/release.yml

+      - name: Install dependencies
+        uses: crazy-max/ghaction-chocolatey@v3
+        with:
+          args: install -y conan git


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

Yeah you're probably right that it's not necessary! I kept it in just because we needed to do a choco install some way or another, it seemed totally innocuous to include, and I didn't want to do another round of testing to confirm. If you think there's a good reason to switch to a choco install invocation instead, let me know!

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 choco with args:

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?

Ahh that's a good point! this is reason enough to just make the change now imo, tested and it works.

acmorrow · 2025-05-14T18:15:14Z

.github/workflows/release.yml

+          cmake . --preset conan-default
+          cmake --build --preset=conan-release --target ALL_BUILD install -j 8


Just curious: but why direct invocation of cmake here rather than conan build?

For reasons that weren't entirely clear, the conan build invocation resulted in a number of undefined symbols coming from the libraries that we were importing in the CMakeLists.txt. @lia-viam and I spent a short time trying to debug and then just determined that the cmake invocation was good enough to get releases built. Will fix in RSDK-10666.

I'm trying to remember if we were in fact using conan build or that was with conan create ?

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.

Maybe throw a # TODO(RSDK-10666): ... on there.

.github/workflows/release.yml

acmorrow · 2025-05-14T18:18:23Z

CMakeLists.txt

  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


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.

acmorrow · 2025-05-14T18:19:09Z

src/viam/examples/mlmodel/example_audio_classification_client.cpp

@@ -377,7 +381,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) {


Or just make the 5 a size_t? I'd expect std::min(size_t{5}, scored_labels.size()) to do the right thing.

src/viam/sdk/CMakeLists.txt

acmorrow · 2025-05-14T18:21:53Z

src/viam/sdk/CMakeLists.txt

+# 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")


Could this test just look only at num_viam_rust_utils_files now? I'd assume that if it is greater than zero then one was found and it should be used.

I may be wrong, but is this a correct assumption? I thought num_viam_rust_utils_files was evaluated before we downloaded, in order to determine if we should download a rust_utils file. So the not WIN32 or AMD64 cases are important for the cases where the user hasn't provided a rust-utils file but we can download one for them.

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 if (TARGET... is what I'm after: https://cmake.org/cmake/help/latest/command/if.html#target

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).

went ahead and switched to if (TARGET viam_rust_utils), confirmed that it builds correctly both with a user-provided rust-utils file and without one.

…stuqdog/cpp-sdk into RSDK-10366-rust-utils-for-windows

acmorrow

LGTM with a few requests for additional comments / TODO's tickets before merge, but no need to post an update with those.

acmorrow · 2025-05-16T15:20:22Z

.github/workflows/release.yml

+      - name: Install dependencies
+        uses: crazy-max/ghaction-chocolatey@v3
+        with:
+          args: install -y conan git


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 choco with args:

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?

acmorrow · 2025-05-16T15:28:46Z

.github/workflows/release.yml

+          cmake . --preset conan-default
+          cmake --build --preset=conan-release --target ALL_BUILD install -j 8


Maybe throw a # TODO(RSDK-10666): ... on there.

acmorrow · 2025-05-16T15:33:13Z

src/viam/sdk/CMakeLists.txt

+# 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")


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 if (TARGET... is what I'm after: https://cmake.org/cmake/help/latest/command/if.html#target

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).

stuqdog added 2 commits April 18, 2025 10:40

only prepend unix when using uds

e9d3ea1

untested download and install rust-utils for windows

0e3b8af

stuqdog requested a review from a team as a code owner April 18, 2025 15:03

stuqdog requested review from njooma and lia-viam and removed request for a team April 18, 2025 15:03

stuqdog commented Apr 18, 2025

View reviewed changes

acmorrow requested changes Apr 18, 2025

View reviewed changes

cr comments

b968803

stuqdog requested a review from acmorrow April 18, 2025 16:55

const correctness

5d5bb2d

acmorrow approved these changes Apr 18, 2025

View reviewed changes

stuqdog added 18 commits April 21, 2025 11:49

try windows build

d9827e5

test it

adff932

try getting all dependencies installed

59fd83c

no setting buf_config_dir

a8e1751

try earlier cmake

ff01f42

goldilocks cmake

96652c8

set boost root

46dad50

set boost includedir, try to find librarydir

1d1ad1e

try list better

3268789

set librarydir

57e6d5d

set boost compiler

cb01ea7

12 not 13

5cf9590

had boost compiler backwards

ff5a2bf

github action for grpc

bc83e5b

specify grpc version

674e729

install grpc with vcpkg

952c305

powershell

7e38e1b

typo

9eab8c5

stuqdog added 3 commits May 8, 2025 11:44

conanfile always include rust-utils info

c6382e1

include .lib

c4143af

interface not private

165ca0c

lia-viam reviewed May 8, 2025

View reviewed changes

stuqdog added 8 commits May 8, 2025 15:26

try cmake invocation again

d292417

typos

9095b79

actually install

84b1874

no install

bfc6faf

no all

4ba53d5

all-build

7cb2dd7

more comprehensive listing

3186390

fix release, comment out windows conan build for now

3d56ccc

stuqdog requested review from lia-viam and acmorrow May 9, 2025 12:32

stuqdog commented May 9, 2025

View reviewed changes

lia-viam approved these changes May 9, 2025

View reviewed changes

stuqdog added 2 commits May 9, 2025 10:47

remove unused code

b710c7a

Merge branch 'main' into RSDK-10366-rust-utils-for-windows

47aca71

acmorrow approved these changes May 14, 2025

View reviewed changes

stuqdog added 3 commits May 16, 2025 09:21

cr comments

b56ab5c

Merge branch 'RSDK-10366-rust-utils-for-windows' of ssh://github.com/…

3693c60

…stuqdog/cpp-sdk into RSDK-10366-rust-utils-for-windows

Merge branch 'main' into RSDK-10366-rust-utils-for-windows

9459e17

acmorrow approved these changes May 16, 2025

View reviewed changes

abe-winter and others added 3 commits May 16, 2025 11:59

cr comments, test direct choco invocation

230b29d

get primetime ready

cd7690b

improve comment

fa063e4

stuqdog merged commit 8d813ed into viamrobotics:main May 16, 2025
13 checks passed

stuqdog deleted the RSDK-10366-rust-utils-for-windows branch May 16, 2025 18:59

		cmake . --preset conan-default
		cmake --build --preset=conan-release --target ALL_BUILD install -j 8

RSDK-10366 - rust utils for windows #416

RSDK-10366 - rust utils for windows #416

Conversation

stuqdog commented Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lia-viam commented May 9, 2025

stuqdog commented May 9, 2025

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuqdog commented Apr 18, 2025 •

edited

Loading