-
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 2 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 |
---|---|---|
|
@@ -266,14 +266,21 @@ 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 "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() | ||
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 elseif here? |
||
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") | ||
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} | ||
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. 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. 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. It is reasonably easy to spin up a windows machine in GCP and RDP into it if you just need a quick test. |
||
${viam_rust_utils_file} | ||
STATUS lvru_status | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
#include <algorithm> | ||
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. This should be down with the other C++ standard library includes around line 4-5 |
||
#include <viam/sdk/rpc/dial.hpp> | ||
|
||
#include <istream> | ||
|
@@ -148,20 +149,27 @@ std::shared_ptr<ViamChannel> ViamChannel::dial(const char* uri, | |
if (opts.entity()) { | ||
entity = opts.entity()->c_str(); | ||
} | ||
char* socket_path = ::dial( | ||
char* proxy_path = ::dial( | ||
uri, entity, type, payload, opts.allows_insecure_downgrade(), float_timeout.count(), ptr); | ||
if (socket_path == NULL) { | ||
if (proxy_path == NULL) { | ||
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.
|
||
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 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. There are other forms of localhost, like 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, the address we get back from |
||
auto tcp_check = std::mismatch(localhost_prefix.begin(), localhost_prefix.end(), proxy_path); | ||
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 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. 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::string address; | ||
if (tcp_check.first != localhost_prefix.end()) { | ||
// proxy path is not a localhost address and is therefore a UDS socket | ||
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.
|
||
address += "unix://"; | ||
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. In 62d40eb#diff-03661151ee3ff17fde17964451d2ea2fbd2322f2bd21f4851eade22a2aaad233R254, I found that using It might be worth considering just making the switch to 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 good call, I'm happy to change this to fwiw this PR is actually enabling windows client usage! The restriction was UDS support in tokio, but now that |
||
} | ||
address += proxy_path; | ||
|
||
const std::shared_ptr<grpc::Channel> channel = | ||
impl::create_viam_channel(address, grpc::InsecureChannelCredentials()); | ||
const std::unique_ptr<viam::robot::v1::RobotService::Stub> st = | ||
viam::robot::v1::RobotService::NewStub(channel); | ||
return std::make_shared<ViamChannel>(channel, socket_path, ptr); | ||
return std::make_shared<ViamChannel>(channel, proxy_path, ptr); | ||
}; | ||
|
||
unsigned int Options::refresh_interval() const { | ||
|
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.
Is supporting cygwin something important?
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.
Not that I'm aware of, I wasn't sure so I was trying to be conservative.