Skip to content

basic windows and macos build compatibility improvements #401

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 4, 2025
Merged

basic windows and macos build compatibility improvements #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1920bb8
[RSDK-10385] Basic windows and macos build compatibility improvements
acmorrow Apr 3, 2025
5cb8aad
Merge branch 'main' into windows
acmorrow Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ if (VIAMCPPSDK_ENFORCE_COMPILER_MINIMA)
if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS 10.0)
message(FATAL_ERROR "Insufficient Apple clang version: XCode 10.0+ required")
endif()
elseif (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This establishes a compiler minimum for Windows. Since there is no history here, the newest Visual Studio seems like fair game. We can always try to downgrade it later if we find a need.

if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS 19.34)
message(FATAL_ERROR "Insufficient MSVC version: Visual Studio 2022 17.4 (MSVC 19.34) or later is required")
endif()
else()
message(FATAL_ERROR "Unknown / untested compiler ${CMAKE_CXX_COMPILER_ID}")
endif()
Expand Down
1 change: 0 additions & 1 deletion src/viam/examples/camera/example_camera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#include <fstream>
#include <string>
#include <unistd.h>
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why all these files included <unistd.h> and at least on my mac they appear to compile without it. We will see what CI says.

#include <vector>

#include <viam/sdk/common/instance.hpp>
Expand Down
1 change: 0 additions & 1 deletion src/viam/examples/dial/example_dial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include <memory>
#include <ostream>
#include <string>
#include <unistd.h>
#include <vector>

#include <boost/optional.hpp>
Expand Down
1 change: 0 additions & 1 deletion src/viam/examples/dial_api_key/example_dial_api_key.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include <memory>
#include <ostream>
#include <string>
#include <unistd.h>
#include <vector>

#include <boost/optional.hpp>
Expand Down
1 change: 0 additions & 1 deletion src/viam/examples/modules/complex/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
#include <memory>
#include <ostream>
#include <string>
#include <unistd.h>
#include <vector>

#include <grpcpp/channel.h>
Expand Down
2 changes: 0 additions & 2 deletions src/viam/examples/motor/example_motor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#include <unistd.h>

#include <chrono>
#include <fstream>
#include <iostream>
Expand Down
12 changes: 11 additions & 1 deletion src/viam/sdk/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,18 @@ configure_file(common/grpc_fwd.hpp.in common/grpc_fwd.hpp)

# Set compile and link options based on arguments
if (VIAMCPPSDK_USE_WALL_WERROR)
target_compile_options(viamsdk PRIVATE -Wall -Werror)
if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables warnings as errors flags for MSVC, and makes it an error if you ask for it and we can't do it on the current toolchain (or the toolchain is unknown).

target_compile_options(viamsdk PRIVATE -Wall -Werror)
elseif (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
target_compile_options(viamsdk PRIVATE -Wall -Werror)
elseif (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
# Note: Currently, the generated protos throw W4244 and W4267, so turn that back to a warning.
target_compile_options(viamsdk PRIVATE /W4 /WX /wd4244 /wd4267)
else()
message(ERROR "VIAMCPPSDK_USE_WALL_ERROR is set, but not known how to enable for compiler ID ${CMAKE_CXX_COMPILER_ID}")
endif()
endif()

if (VIAMCPPSDK_SANITIZED_BUILD)
target_link_options(viamsdk PRIVATE -fsanitize=undefined -fno-omit-frame-pointer -fno-sanitize-recover)
target_compile_options(viamsdk PRIVATE -fsanitize=undefined -fno-omit-frame-pointer -fno-sanitize-recover)
Expand Down
5 changes: 5 additions & 0 deletions src/viam/sdk/components/camera.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@
#include <vector>

#include <boost/endian/conversion.hpp>

#if defined(__has_include) && (__has_include(<xtensor/containers/xarray.hpp>))
#include <xtensor/containers/xarray.hpp>
#else
#include <xtensor/xarray.hpp>
#endif

#include <viam/sdk/common/proto_value.hpp>
#include <viam/sdk/common/utils.hpp>
Expand Down
13 changes: 3 additions & 10 deletions src/viam/sdk/module/service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
#include <memory>
#include <sstream>
#include <string>
#include <sys/socket.h>
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these were only needed for the creation of the socket below, which we are no longer doing.

#include <sys/stat.h>

#include <boost/log/trivial.hpp>
#include <boost/none.hpp>
Expand Down Expand Up @@ -115,8 +113,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service {
Registry::get().lookup_model(cfg.name());
if (reg) {
try {
const std::shared_ptr<Resource> res = reg->construct_resource(deps, cfg);
manager->replace_one(cfg.resource_name(), res);
const std::shared_ptr<Resource> resource = reg->construct_resource(deps, cfg);
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows throws a shadowing warning here which gets promoted to an error because we have res above.

manager->replace_one(cfg.resource_name(), resource);
} catch (const std::exception& exc) {
return grpc::Status(::grpc::INTERNAL, exc.what());
}
Expand Down Expand Up @@ -252,13 +250,8 @@ ModuleService::~ModuleService() {
}

void ModuleService::serve() {
const mode_t old_mask = umask(0077);
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention this in my batch of comments. This code, which has been here forever, has always done nothing. It sure looked like it was making the socket that we were going to use for communication with RDK, but look carefully and you can see that we never actually bind the socket anywhere, or invoke it in any way. The actual socket gets made by gRPC.

const int sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
listen(sockfd, 10);
umask(old_mask);

server_->register_service(impl_.get());
const std::string address = "unix://" + module_->addr();
const std::string address = "unix:" + module_->addr();
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually the most important change. Please see https://grpc.github.io/grpc/cpp/md_doc_naming.html regarding unix:// vs unix:. The former, which is what we were using before, requires an absolute path, and I was entirely unable to get it to work with Windows paths. However, the latter can handle both absolute and relative, and it seems to work just fine on windows.

server_->add_listening_port(address);

module_->set_ready();
Expand Down
3 changes: 1 addition & 2 deletions src/viam/sdk/robot/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include <mutex>
#include <string>
#include <thread>
#include <unistd.h>
#include <vector>

#include <boost/log/trivial.hpp>
Expand Down Expand Up @@ -216,7 +215,7 @@ void RobotClient::refresh_every() {
std::this_thread::sleep_for(std::chrono::seconds(refresh_interval_));
refresh();

} catch (std::exception& exc) {
} catch (std::exception&) {
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows warning that this was unused.

break;
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/viam/sdk/services/mlmodel.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@
#include <boost/mpl/list.hpp>
#include <boost/mpl/transform_view.hpp>
#include <boost/variant/variant.hpp>

#if defined(__has_include) && (__has_include(<xtensor/containers/xadapt.hpp>))
#include <xtensor/containers/xadapt.hpp>
#else
#include <xtensor/xadapt.hpp>
#endif

#include <viam/sdk/common/utils.hpp>
#include <viam/sdk/services/service.hpp>
Expand Down
9 changes: 9 additions & 0 deletions src/viam/sdk/services/private/mlmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,11 @@ MLModelService::tensor_views make_sdk_tensor_from_api_tensor_t(const T* data,
shape_accum = next_shape_accum;
}

#ifdef _MSC_VER
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nuisance. Windows concludes that for one instantiation of the template that the conditional on 164 is tautologically true, which it is. But the point is to handle it for the instantiations where it is not a tautology. I opted to just to suppress the warning at the site.

#pragma warning(push)
#pragma warning(disable : 4127)
#endif

// We need to handle the special case of an odd number of 16-bit
// elements because that will arrive appearing to have one more
// element than the shape would indicate, and we don't want to
Expand All @@ -160,6 +165,10 @@ MLModelService::tensor_views make_sdk_tensor_from_api_tensor_t(const T* data,
size -= 1;
}

#ifdef _MSC_VER
#pragma warning(pop)
#endif

if (size != shape_accum) {
std::ostringstream message;
// TODO: Provide the shape and details
Expand Down
8 changes: 4 additions & 4 deletions src/viam/sdk/services/private/navigation_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void NavigationClient::set_mode(const Navigation::Mode mode, const ProtoStruct&
request.set_mode(viam::service::navigation::v1::Mode(mode));
*request.mutable_extra() = to_proto(extra);
})
.invoke([](auto& response) {});
.invoke([](auto&) {});
Copy link
Member Author

@acmorrow acmorrow Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused var name warnings on windows, here and below.

}

Navigation::LocationResponse NavigationClient::get_location(const ProtoStruct& extra) {
Expand All @@ -81,7 +81,7 @@ void NavigationClient::add_waypoint(const geo_point& location, const ProtoStruct
*request.mutable_location() = to_proto(location);
*request.mutable_extra() = to_proto(extra);
})
.invoke([](auto& response) {});
.invoke([](auto&) {});
}

void NavigationClient::remove_waypoint(const std::string id, const ProtoStruct& extra) {
Expand All @@ -90,7 +90,7 @@ void NavigationClient::remove_waypoint(const std::string id, const ProtoStruct&
*request.mutable_id() = id;
*request.mutable_extra() = to_proto(extra);
})
.invoke([](auto& response) {});
.invoke([](auto&) {});
}

std::vector<geo_geometry> NavigationClient::get_obstacles(const ProtoStruct& extra) {
Expand All @@ -107,7 +107,7 @@ std::vector<NavigationClient::Path> NavigationClient::get_paths(const ProtoStruc

NavigationClient::Properties NavigationClient::get_properties() {
return make_client_helper(this, *stub_, &StubType::GetProperties)
.with([&](auto& request) {})
.with([&](auto&) {})
.invoke([](auto& response) { return Properties{MapType(response.map_type())}; });
}

Expand Down