basic windows and macos build compatibility improvements #401
Conversation
| 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") |
There was a problem hiding this comment.
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.
| @@ -1,6 +1,5 @@ | |||
| #include <fstream> | |||
| #include <string> | |||
| #include <unistd.h> | |||
There was a problem hiding this comment.
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.
| # 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") |
There was a problem hiding this comment.
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).
| #include <memory> | ||
| #include <sstream> | ||
| #include <string> | ||
| #include <sys/socket.h> |
There was a problem hiding this comment.
I think these were only needed for the creation of the socket below, which we are no longer doing.
| 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); |
There was a problem hiding this comment.
Windows throws a shadowing warning here which gets promoted to an error because we have res above.
|
|
||
| server_->register_service(impl_.get()); | ||
| const std::string address = "unix://" + module_->addr(); | ||
| const std::string address = "unix:" + module_->addr(); |
There was a problem hiding this comment.
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.
| refresh(); | ||
|
|
||
| } catch (std::exception& exc) { | ||
| } catch (std::exception&) { |
There was a problem hiding this comment.
Windows warning that this was unused.
| shape_accum = next_shape_accum; | ||
| } | ||
|
|
||
| #ifdef _MSC_VER |
There was a problem hiding this comment.
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.
| *request.mutable_extra() = to_proto(extra); | ||
| }) | ||
| .invoke([](auto& response) {}); | ||
| .invoke([](auto&) {}); |
There was a problem hiding this comment.
Unused var name warnings on windows, here and below.
| } | ||
|
|
||
| void ModuleService::serve() { | ||
| const mode_t old_mask = umask(0077); |
There was a problem hiding this comment.
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.
lia-viam
left a comment
lia-viam
left a comment
There was a problem hiding this comment.
this looks good overall, __has_include comment would be nice to address but non-blocking
2 participants
Small fixes needed for building the C++ SDK on Windows. This is far from complete - this is just the stuff that seems reasonable to land as is. I'll leave some comments to explain a few things.