-
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 58 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,55 @@ jobs: | |
github.event_name == 'schedule' && | ||
steps.git_info.outputs.current_commit == steps.last_successful_commit.outputs.commit-hash | ||
|
||
# CR erodkin: we have (expected) working rust-utils builds now, but need to get that merged. | ||
# once it is and we have a new release, try actually testing and delete the TODO. | ||
# TODO(RSDK-10638) - uncomment and make all this work once we have good rust-utils windows builds | ||
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 cmake | ||
uses: ssrobins/install-cmake@v1 | ||
with: | ||
version: 3.27.2 | ||
|
||
- name: Install python | ||
uses: actions/setup-python@v5 | ||
with: | ||
python-version: '3.12' | ||
|
||
- name: Install Scoop | ||
uses: MinoruSekine/[email protected] | ||
|
||
- name: install buf 'n stuff | ||
run: scoop install buf ninja | ||
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. Do you still need any of this if you have choco? 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. unfortunately I think so. possibly 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 a little puzzled about this, because as far as I know I was able to build the C++ SDK as part of the windows tflite build without scoop, or many of these other steps. 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. Also, this page, describing https://github.com/actions/runner-images/blob/main/images/windows/Windows2022-Readme.md 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. Oh: I made it so the C++ SDK downloads buf. So you shouldn't need to install it at all!: https://github.com/viamrobotics/viam-cpp-sdk/blob/main/src/viam/api/CMakeLists.txt#L64-L94 |
||
|
||
- 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 . | ||
env: | ||
CONAN_USER_HOME: c:/cache | ||
CONAN_USER_HOME_SHORT: c:/cache/conan_shortpaths | ||
|
||
build_macos: | ||
if: github.repository_owner == 'viamrobotics' | ||
needs: [prepare] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,77 @@ jobs: | |
uses: andymckay/[email protected] | ||
if: steps.release_exists.outputs.id != '' | ||
|
||
|
||
# CR erodkin: we have (expected) working rust-utils builds now, but need to get that merged. | ||
# once it is and we have a new release, try actually testing and delete the TODO. | ||
# TODO (RSDK-10638) - uncomment and make all this work once we have good rust-utils windows builds | ||
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 cmake | ||
uses: ssrobins/install-cmake@v1 | ||
with: | ||
version: 3.27.2 | ||
|
||
- name: Install python | ||
uses: actions/setup-python@v5 | ||
with: | ||
python-version: '3.12' | ||
|
||
- name: Install Scoop | ||
uses: MinoruSekine/[email protected] | ||
|
||
- name: install buf 'n stuff | ||
run: scoop install buf ninja | ||
|
||
- 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 create . | ||
env: | ||
CONAN_USER_HOME: c:/cache | ||
CONAN_USER_HOME_SHORT: c:/cache/conan_shortpaths | ||
|
||
- name: list stuff | ||
run: | | ||
ls | ||
ls build/ | ||
|
||
- name: Copy | ||
run: | | ||
cmake --install 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,21 +289,30 @@ 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) | ||
if (NOT WIN32) # build `SHARED` on unix-based systems | ||
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. Building 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's not shared though, so I don't really understand how this is working. I think this said 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 don't immediately recall, but I'll switch back 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.
|
||
add_library(viam_rust_utils SHARED IMPORTED) | ||
|
||
target_link_directories(viam_rust_utils | ||
INTERFACE | ||
"$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>" | ||
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_LIBDIR}>" | ||
) | ||
set_property(TARGET viam_rust_utils PROPERTY IMPORTED_LOCATION ${viam_rust_utils_file}) | ||
|
||
elseif (${CMAKE_SYSTEM_PROCESSOR} STREQUAL "AMD64") # build `STATIC` for windows | ||
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 |
||
) | ||
set_property(TARGET viam_rust_utils PROPERTY IMPORTED_LOCATION ${viam_rust_utils_file}) | ||
endif() | ||
|
||
|
||
if (NOT WIN32) # installing seems to be necessary for tests to pass in CI | ||
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'd imagine that it should only be necessary to install |
||
install( | ||
IMPORTED_RUNTIME_ARTIFACTS viam_rust_utils | ||
LIBRARY COMPONENT viam-cpp-sdk_runtime | ||
|
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 |
---|---|---|
|
@@ -12,6 +12,10 @@ | |
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
#ifdef _WIN32 | ||
#define NOMINMAX | ||
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 this may no longer be necessary because of the |
||
#endif | ||
|
||
#include <chrono> | ||
#include <cstdlib> | ||
#include <fstream> | ||
|
@@ -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) { | ||
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. | ||
|
Uh oh!
There was an error while loading. Please reload this page.