Closed
Conversation
- `T(T&) = default;` is not a thing.
- That's neither a copy constructor nor a move constructor.
- In order to fix this, it was best to remove the template
flavors of `Expected` member functions. Instead, I manually
define the relevant overloads.
- `#include <windows.h>` before including other Windows-related headers.
- Use `.bazelrc` for platform-specific compiler options.
- But now how will Envoy know to `-DDD_USE_ABSEIL_FOR_ENVOY`?
- Setting an environment variable to an empty string instead removes the environment variable. This affected some tests. - ::gethostname requires special setup. - timegm(...) has a different name. - The compiler wouldn't automatically capture a local `const std::time_t`. - Since bazel builds don't include libcurl, tests had to be updated to use a mock `HTTPClient` instance, otherwise configuration fails. - bazel builds use `absl::string_view`, so some appearances of `operator=` had to be changed to `assign`. - Deleting a file using the facilities in `std::filesystem` does not delete the file. - `::gethostname` requires winsock linkage. - `Expected` now does not contain any member function templates. I don't want to think about how to explain why.
dmehala
reviewed
Jan 8, 2024
Collaborator
There was a problem hiding this comment.
Using _putenv instead of GetEnvironmentVariable in GuardEnv should be enough.
I was quite surprised that you didn't have to update all occurrences of range. Most errors I had 2 weeks ago were related to range accepting const char* while most usage provides an iterator. So, I booted my Windows machine and tried to compile your branch.
Unfortunately, it is not compiling, I use Visual Studio 17.7 - 193732822. Could you provide your version of Visual Studio and the C++ compiler?
Anyway, this morning I cleaned a bit my branch and opened a draft PR, there is a lot more going on. Have a look, try to compile it and if needed take what you need.
dd-trace-cpp on Windows (taken yesterday):
Things our branches do not handle:
Ninjagenerator complaining on targets name.- Update
READMEbuild section withcmake --build <BUILD_DIR> -jinstead ofmake && make install. - Symbol visibility
// export
#define DD_TRACE_SHARED __declspec(dllexport)
// import
#define DD_TRACE_SHARED __declspec(dllimport)
// for linux
#define DD_TRACE_SHARED __attribute__((visibility("default")))Good job and good luck with CircleCI.
src/datadog/environment.cpp
Outdated
| #ifdef _MSC_VER | ||
| // maximum size of an environment variable value on Windows | ||
| char buffer[32767]; | ||
| const DWORD rc = GetEnvironmentVariable(name, buffer, sizeof buffer); |
Collaborator
There was a problem hiding this comment.
No need to write a windows specific version. There is two way to set/get environment variables on Windows:
_putenv/getenvGetEnvironmentVariable/SetEnvironmentVariable
MSVC implementation of the STL uses getenv, so, to set an environment use _putenv instead.
test/test_tracer_config.cpp
Outdated
| const char* get_value() { | ||
| #ifdef _MSC_VER | ||
| const DWORD rc = | ||
| GetEnvironmentVariable(name_.c_str(), buffer_, sizeof buffer_); |
Contributor
Author
|
I booted up my Windows machine again this morning.
Looking at the last line, that's my local Here's getting the content of the file: PS C:\users\mathm\src\dd-trace-cpp> cat bazel-out/x64_windows-fastbuild/bin/_objs/dd_trace_cpp/tags.obj.params
/nologo
/DCOMPILER_MSVC
/DNOMINMAX
/D_WIN32_WINNT=0x0601
/D_CRT_SECURE_NO_DEPRECATE
/D_CRT_SECURE_NO_WARNINGS
/bigobj
/Zm500
/EHsc
/wd4351
/wd4291
/wd4250
/wd4996
/I.
/Ibazel-out/x64_windows-fastbuild/bin
/Iexternal/com_google_absl
/Ibazel-out/x64_windows-fastbuild/bin/external/com_google_absl
/Ibazel-out/x64_windows-fastbuild/bin/_virtual_includes/dd_trace_cpp
/D__CLANG_SUPPORT_DYN_ANNOTATION__
/showIncludes
/MD
/Od
/Z7
/wd4117
-D__DATE__=\"redacted\"
-D__TIMESTAMP__=\"redacted\"
-D__TIME__=\"redacted\"
/std:c++17
/DDD_USE_ABSEIL_FOR_ENVOY
/Fobazel-out/x64_windows-fastbuild/bin/_objs/dd_trace_cpp/tags.obj
/c
src/datadog/tags.cppMaybe it's those If I run the compiler without arguments, it prints its version: PS C:\users\mathm\src\dd-trace-cpp> C:\'Program Files'\'Microsoft Visual Studio'\2022\Community\VC\Tools\MSVC\14.38.33130\bin\HostX64\x64\cl.exe
Microsoft (R) C/C++ Optimizing Compiler Version 19.38.33133 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
usage: cl [ option... ] filename... [ /link linkoption... ]There are no other versions installed: PS C:\users\mathm\src\dd-trace-cpp> ls C:\'Program Files'\'Microsoft Visual Studio'\2022\Community\VC\Tools\MSVC\
Directory: C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC
Mode LastWriteTime Length Name
---- ------------- ------ ----
d----- 1/6/2024 1:21 PM 14.38.33130
PS C:\users\mathm\src\dd-trace-cpp>Windows info: System info: Bazel info: PS C:\users\mathm\src\dd-trace-cpp> bazelisk version
Bazelisk version: v1.19.0
Build label: 7.0.0
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Mon Dec 11 16:54:12 2023 (1702313652)
Build timestamp: 1702313652
Build timestamp as int: 1702313652
PS C:\users\mathm\src\dd-trace-cpp>Visual Studio info: |
Collaborator
|
Just for the record, we already talk about it on Slack. We have a similar Windows setup (that's new we did not talk about it yet). The difference is that bazel build uses abseil and abseil string_view iterator is a const char* while MSVC STL Iterator is not an alias to |
Contributor
Author
|
Et voila! Still remaining:
|
dgoffredo
force-pushed
the
david.goffredo/windows
branch
from
df69e33 to
fd07f86
Compare
Collaborator
|
Soon™️ |
Contributor
Author
|
Consider integrating the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is my attempt to address #84.
Bazel can build and test this code using Visual Studio 2022 Community Edition on Windows 10.
I have yet to check whether Envoy can build this code on Windows, because I can't get Envoy to build on Windows in any case. Something about a missing header in quiche.
See the commit messages for lists of the changes. We'll want to go through the changes in person to discuss alternatives.
Finally, we'll want to add a Windows bazel build/test to CircleCI. Otherwise it'll break immediately. It looks like it's possible with CircleCI's Windows "orb," but I have yet to try it. Ideally, Visual Studio is already installed; then we can install "chocolatey," thenchoco install bazelisk, and then go to town in powershell. None of that is here.Edit: CI is working :D