ARROW-15415: [C++] Fixes for MSVC + vcpkg Debug build by wjones127 · Pull Request #13108 · apache/arrow

wjones127 · 2022-05-09T21:05:53Z

These are the changes needed for me to be able to compile Arrow in MSCV (from Visual Studio 2022) with the following CMakeUserPresets entry:

{
  "name": "user-cpp-debug-mscv",
  "inherits": [ "ninja-debug"],
  "cacheVariables": {
    "ARROW_DEPENDENCY_SOURCE": "VCPKG",
    "CMAKE_BUILD_TYPE": "Debug",
    "VCPKG_TARGET_TRIPLET": "x64-windows",
    "VCPKG_LIBRARY_LINKAGE": "dynamic",
    "ARROW_DEPENDENCY_USE_SHARED": "ON",
    "ARROW_BUILD_EXAMPLES": "ON"
  }
}

github-actions · 2022-05-09T21:06:14Z

https://issues.apache.org/jira/browse/ARROW-15415

wjones127 · 2022-05-09T21:14:06Z

I have been working with these changes in my local, but there is one final error I haven't been able to figure out how to fix.

arrow/cpp/src/arrow/util/tracing.h

Line 64 in d00caa9

std::unique_ptr<Impl> inner_impl;

[build] FAILED: src/arrow/CMakeFiles/arrow_shared.dir/compute/exec/tpch_node.cc.obj 
[build] C:\PROGRA~1\MICROS~4\2022\COMMUN~1\VC\Tools\MSVC\1430~1.307\bin\Hostx64\x64\cl.exe  /nologo /TP -DARROW_EXPORTING -DARROW_EXTRA_ERROR_CONTEXT -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_MIMALLOC -DARROW_WITH_BROTLI -DARROW_WITH_BZ2 -DARROW_WITH_LZ4 -DARROW_WITH_RE2 -DARROW_WITH_SNAPPY -DARROW_WITH_TIMING_TESTS -DARROW_WITH_UTF8PROC -DARROW_WITH_ZLIB -DARROW_WITH_ZSTD -DBOOST_ALL_DYN_LINK -DBOOST_ALL_NO_LIB -DPROTOBUF_USE_DLLS -DURI_STATIC_BUILD -D_CRT_SECURE_NO_WARNINGS -D_ENABLE_EXTENDED_ALIGNED_STORAGE -Darrow_shared_EXPORTS -IC:\Users\voltron\arrow\cpp\build\user-cpp-debug-mscv\src -IC:\Users\voltron\arrow\cpp\src -IC:\Users\voltron\arrow\cpp\src\generated -IC:\Users\voltron\arrow\cpp\thirdparty\flatbuffers\include -IC:\Users\voltron\arrow\cpp\vcpkg_installed\x64-windows\include -IC:\Users\voltron\arrow\cpp\build\user-cpp-debug-mscv\mimalloc_ep\src\mimalloc_ep\include\mimalloc-1.7 -IC:\Users\voltron\arrow\cpp\vcpkg_installed\x64-windows\share\rapidjson\..\..\include -IC:\Users\voltron\arrow\cpp\build\user-cpp-debug-mscv\xsimd_ep\src\xsimd_ep-install\include -IC:\Users\voltron\arrow\cpp\thirdparty\hadoop\include /DWIN32 /D_WINDOWS  /GR /EHsc /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING  /W3 /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4365 /wd4267 /wd4838 /WX /wd4800 /wd4996 /wd4065   /MDd /Zi /Ob0 /Od /RTC1 /showIncludes /Fosrc\arrow\CMakeFiles\arrow_shared.dir\compute\exec\tpch_node.cc.obj /Fdsrc\arrow\CMakeFiles\arrow_shared.dir\ /FS -c C:\Users\voltron\arrow\cpp\src\arrow\compute\exec\tpch_node.cc
[build] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.30.30705\include\memory(3086): error C2027: use of undefined type 'arrow::internal::tracing::SpanImpl'
[build] C:\Users\voltron\arrow\cpp\src\arrow/util/tracing.h(30): note: see declaration of 'arrow::internal::tracing::SpanImpl'
[build] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.30.30705\include\memory(3085): note: while compiling class template member function 'void std::default_delete<arrow::util::tracing::Span::Impl>::operator ()(_Ty *) noexcept const'
[build]         with
[build]         [
[build]             _Ty=arrow::util::tracing::Span::Impl
[build]         ]
[build] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.30.30705\include\memory(3195): note: see reference to function template instantiation 'void std::default_delete<arrow::util::tracing::Span::Impl>::operator ()(_Ty *) noexcept const' being compiled
[build]         with
[build]         [
[build]             _Ty=arrow::util::tracing::Span::Impl
[build]         ]
[build] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.30.30705\include\memory(3122): note: see reference to class template instantiation 'std::default_delete<arrow::util::tracing::Span::Impl>' being compiled
[build] C:\Users\voltron\arrow\cpp\src\arrow/util/tracing.h(64): note: see reference to class template instantiation 'std::unique_ptr<arrow::util::tracing::Span::Impl,std::default_delete<arrow::util::tracing::Span::Impl>>' being compiled
[build] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.30.30705\include\memory(3086): error C2338: can't delete an incomplete type
[build] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.30.30705\include\memory(3087): warning C4150: deletion of pointer to incomplete type 'arrow::internal::tracing::SpanImpl'; no destructor called
[build] C:\Users\voltron\arrow\cpp\src\arrow/util/tracing.h(30): note: see declaration of 'arrow::internal::tracing::SpanImpl'

I am able to get this to work if I change it to a shared_ptr instead of unique_ptr, but that's not actually a good idea.

This build is configured with ARROW_WITH_OPENTELEMETRY=OFF (there's a completely different set of errors if I turn it on).

westonpace · 2022-05-09T21:17:35Z

+1 to this change. I had noticed it myself but hadn't gotten around to making a PR.

wjones127 · 2022-05-10T19:47:12Z

For the tracing impl incomplete type issue, it seemed like the fix was to make sure that any file with the line:

#include "arrow/compute/exec/exec_plan.h" // contains #include "arrow/util/tracing.h"

Also has the line

#include "arrow/util/tracing_internal.h"

lidavidm · 2022-05-12T12:26:16Z

For the tracing impl incomplete type issue, it seemed like the fix was to make sure that any file with the line:
#include "arrow/compute/exec/exec_plan.h" // contains #include "arrow/util/tracing.h"
Also has the line
#include "arrow/util/tracing_internal.h"

Hmm, isn't that a problem? exec_plan.h is a public API, so this means the header is not usable by applications?

lidavidm · 2022-05-12T12:27:57Z

It seems something else is implicitly using SpanImpl::~SpanImpl

westonpace · 2022-05-13T01:19:36Z

Shouldn't we be able to remove ARROW_EXPORT from Span in tracing.h? I think that is the problem in this case.

wjones127 · 2022-05-17T16:20:59Z

@github-actions crossbow submit test-build-vcpkg-win

wjones127 · 2022-05-17T17:51:46Z

Shouldn't we be able to remove ARROW_EXPORT from Span in tracing.h? I think that is the problem in this case.

@westonpace Worked before I rebase, but looks like #13040 introduced a reference to arrow::util::tracing::Span here:

arrow/cpp/src/arrow/dataset/file_base.cc

Line 486 in 5f7a5a4

util::tracing::Span span;

Should I replace that with something like below?

arrow/cpp/src/arrow/dataset/file_csv.cc

Lines 182 to 185 in 5f7a5a4

    
           #ifdef ARROW_WITH_OPENTELEMETRY 
        
             auto tracer = arrow::internal::tracing::GetTracer(); 
        
             auto span = tracer->StartSpan("arrow::dataset::CsvFileFormat::OpenReaderAsync"); 
        
           #endif

github-actions · 2022-05-17T18:26:02Z

Revision: ff8cb17419d49121507a640c43e4f9f894fb2efd

Submitted crossbow builds: ursacomputing/crossbow @ actions-2102

Task	Status
test-build-vcpkg-win

westonpace · 2022-05-17T19:56:31Z

Ah, that will be a problem. Well, Impl, std::unique_ptr and ARROW_EXPORT are not going to work together well. If a type is exported and used in std::unique_ptr then the type has to be fully defined becuase "windows". Is there any chance we could get rid of the pimpl pattern and just use a virtual class?

wjones127 · 2022-05-18T19:09:32Z

Is there any chance we could get rid of the pimpl pattern and just use a virtual class?

I'm not sure what you mean by this. It seems like we want to have Get() return opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span> if #ifdef ARROW_WITH_OPENTELEMETRY, but not expose that condition in the header file. Not sure how a virtual class helps hide that.

wjones127 · 2022-05-19T18:29:03Z

@github-actions crossbow submit test-build-vcpkg-win

github-actions · 2022-05-19T18:30:41Z

Revision: 95b93a29f997ff47da8c644848f604c2ca6fb6d1

Submitted crossbow builds: ursacomputing/crossbow @ actions-c95608c96f

Task	Status
test-build-vcpkg-win

westonpace · 2022-06-07T19:27:28Z

I'm not sure what you mean by this. It seems like we want to have Get() return opentelemetry::nostd::shared_ptropentelemetry::trace::Span if #ifdef ARROW_WITH_OPENTELEMETRY, but not expose that condition in the header file. Not sure how a virtual class helps hide that.

Then you can make the Get methods a part of the subclass and force callers to do a checked / dynamic cast. Since these methods are called primarily from helper-macros this shouldn't make any real difference to normal usage of the type.

Or we just create our own Span that proxies the surface of opentelemetry::trace::Span that we care about and then not use opentelemetry::trace::Span anywhere.

wjones127 · 2022-06-08T22:37:33Z

                        std::shared_ptr<const KeyValueMetadata> metadata = NULLPTR)
-      : ExecPlan(exec_context), metadata_(std::move(metadata)) {}
+      : ExecPlan(exec_context), metadata_(std::move(metadata)) {
+        span_ = arrow::internal::tracing::OTSpan();


I'm not sure why, but we segfault in the first line of StartProducing(). I believe this is because I am not initialized span_ correctly. In the header file, it's type is is the public Span class arrow::util::tracing::Span, but for the macros to work it needs to be initialized as an arrow::internal::tracing::OTSpan so that is has the shared_ptr field. (When the spans are used just internally, I was able to solve this same segfault by switching the declaration to OTSpan, but that won't work for the member variables.)

Any idea what might be going wrong?

How about something like this: westonpace@85090cc

Then we can still use span but the details are hidden.

Thanks, Weston. That works much better.

Co-authored-by: Weston Pace <weston.pace@gmail.com>

wjones127 · 2022-06-11T01:30:05Z

This builds and tests successfully on my local Windows setup now.

westonpace

Big thanks for doing this. These issues have been a pain in my side every time I need to build on Windows. I'm glad you figured out the Thrift one because I was never able to get that working.

westonpace · 2022-06-11T01:52:07Z

@lidavidm can you take a look at the OT changes?

westonpace · 2022-06-11T01:55:15Z

Also, CC @mbrobbel

github-actions Bot added the Component: C++ label May 9, 2022

wjones127 mentioned this pull request May 9, 2022

ARROW-15415: [C++] Fixes for Windows Debug build #12230

Closed

westonpace reviewed May 9, 2022

View reviewed changes

wjones127 marked this pull request as ready for review May 10, 2022 20:48

ianmcook mentioned this pull request May 17, 2022

[arrow] Update to 8.0.0 microsoft/vcpkg#24714

Merged

wjones127 force-pushed the mscv-fixes branch from de167aa to ff8cb17 Compare May 17, 2022 16:17

github-actions Bot added Component: FlightRPC Component: Gandiva Component: Go Component: Java Component: Python Component: R Component: Ruby Component: Parquet labels Jun 7, 2022

wjones127 added 2 commits June 7, 2022 15:39

fix: Don't use pimpl for Span

f109430

fix: use correct name for Windows thrift debug libs

b89f9f9

wjones127 force-pushed the mscv-fixes branch from 07c0703 to b89f9f9 Compare June 7, 2022 22:40

wjones127 added 3 commits June 8, 2022 12:40

fix: Add missing import in test

403954a

Attempt to get casting working, but failing

b92e2f4

fix: Initialize OTSpan correctly

399e0d7

wjones127 commented Jun 8, 2022

View reviewed changes

save-buffer mentioned this pull request Jun 10, 2022

ARROW-16713: [C++] Pull join accumulation outside of HashJoinImpl #13332

Merged

wjones127 and others added 2 commits June 10, 2022 17:37

feat: Implement with SpanDetail

c2f8faf

Co-authored-by: Weston Pace <weston.pace@gmail.com>

fix: formatting

539529e

westonpace approved these changes Jun 11, 2022

View reviewed changes

westonpace requested a review from lidavidm June 11, 2022 01:52

lidavidm approved these changes Jun 11, 2022

View reviewed changes

mbrobbel approved these changes Jun 13, 2022

View reviewed changes

lidavidm merged commit a53f2bd into apache:master Jun 13, 2022

wjones127 deleted the mscv-fixes branch June 13, 2022 14:48

asfimport mentioned this pull request Nov 23, 2022

[C++] Cannot build debug with MSVC and vcpkg #30897

Closed

Uh oh!

Conversation

wjones127 commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 9, 2022

Uh oh!

wjones127 commented May 9, 2022

Uh oh!

westonpace May 9, 2022

Choose a reason for hiding this comment

Uh oh!

wjones127 commented May 10, 2022

Uh oh!

lidavidm commented May 12, 2022

Uh oh!

lidavidm commented May 12, 2022

Uh oh!

westonpace commented May 13, 2022

Uh oh!

wjones127 commented May 17, 2022

Uh oh!

wjones127 commented May 17, 2022

Uh oh!

github-actions Bot commented May 17, 2022

Uh oh!

westonpace commented May 17, 2022

Uh oh!

wjones127 commented May 18, 2022

Uh oh!

wjones127 commented May 19, 2022

Uh oh!

github-actions Bot commented May 19, 2022

Uh oh!

westonpace commented Jun 7, 2022

Uh oh!

wjones127 Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

westonpace Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

wjones127 Jun 11, 2022

Choose a reason for hiding this comment

Uh oh!

wjones127 commented Jun 11, 2022

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

westonpace commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonpace commented Jun 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wjones127 commented May 9, 2022 •

edited

Loading

westonpace commented Jun 11, 2022 •

edited

Loading