Uploaded image for project: 'Apache Arrow'
  1. Apache Arrow
  2. ARROW-1134

[C++] Allow C++/CLI projects to build with Arrow​

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: C++

      Description

      Currently, the inclusion of <mutex> in some of Arrow's C++ headers prevents C++/CLI code from building against it.

      From a C++/CLI project:

      #include <arrow/io/file.h>
      ...

      "#error directive: <mutex> is not supported when compiling with /clr or /clr:pure."

      This could be patched by optionally relying on Boost's mutex/lock_guard instead of std, or not exposing the #include <mutex> publically.

        Issue Links

          Activity

          Hide
          xhochy Uwe L. Korn added a comment -

          Toby Shaw I would be in favour here to not expose the #include <mutex> publicly. This won't be straightforward as you would need to write private implementations then for some of the classes. Alternatively in some cases, we probably could implement functions without a mutex, e.g. https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/memory.cc#L152 could be refactored to be lock-free. Can you make a patch for that?

          Is there a list showing what is disallowed in C++/CLI?

          Show
          xhochy Uwe L. Korn added a comment - Toby Shaw I would be in favour here to not expose the #include <mutex> publicly. This won't be straightforward as you would need to write private implementations then for some of the classes. Alternatively in some cases, we probably could implement functions without a mutex, e.g. https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/memory.cc#L152 could be refactored to be lock-free. Can you make a patch for that? Is there a list showing what is disallowed in C++/CLI?
          Hide
          tobyshaw Toby Shaw added a comment - - edited

          I agree that is probably the most sensible way to fix this.

          As for the list, sadly I don't believe there exists one (if there is I would love to know about it). It's a fair point that simply hiding the mutex includes might not necessarily resolve all of the restrictions on C++/CLI. Having said that, I locally resolved the mutex issue by using Boost's headers instead, and there was one complaint about a nullptr (fixed by replacing it with NULL), and after that it worked fine. Of course I haven't touched the entire Arrow API, so certainly more digging might be needed.

          Show
          tobyshaw Toby Shaw added a comment - - edited I agree that is probably the most sensible way to fix this. As for the list, sadly I don't believe there exists one (if there is I would love to know about it). It's a fair point that simply hiding the mutex includes might not necessarily resolve all of the restrictions on C++/CLI. Having said that, I locally resolved the mutex issue by using Boost's headers instead, and there was one complaint about a nullptr (fixed by replacing it with NULL), and after that it worked fine. Of course I haven't touched the entire Arrow API, so certainly more digging might be needed.
          Hide
          xhochy Uwe L. Korn added a comment -

          It would be nice if we would have a compile check in our AppVeyor CI build that tests if you can use the Arrow API in C++/CLI.
          Note that you should be able to include all public Arrow headers in your code by #include <arrow/api.h>

          Show
          xhochy Uwe L. Korn added a comment - It would be nice if we would have a compile check in our AppVeyor CI build that tests if you can use the Arrow API in C++/CLI. Note that you should be able to include all public Arrow headers in your code by #include <arrow/api.h>
          Hide
          wesmckinn Wes McKinney added a comment -

          I'm OK with hiding mutexes in private implementations where needed

          Show
          wesmckinn Wes McKinney added a comment - I'm OK with hiding mutexes in private implementations where needed
          Hide
          tobyshaw Toby Shaw added a comment -

          Uwe L. Korn Somewhat tangential, but I'm not sure where else to post this.

          You mention all public Arrow headers can be included via #include <arrow/api.h>. Unless I'm mistaken, this doesn't seem to include arrow/io/file.h, which is included in parquet-cpp, see: https://github.com/apache/parquet-cpp/blob/master/src/parquet/file/reader.cc#L26. Should parquet depend on non-public Arrow headers?

          Show
          tobyshaw Toby Shaw added a comment - Uwe L. Korn Somewhat tangential, but I'm not sure where else to post this. You mention all public Arrow headers can be included via #include <arrow/api.h>. Unless I'm mistaken, this doesn't seem to include arrow/io/file.h, which is included in parquet-cpp, see: https://github.com/apache/parquet-cpp/blob/master/src/parquet/file/reader.cc#L26 . Should parquet depend on non-public Arrow headers?
          Hide
          wesmckinn Wes McKinney added a comment -

          That's an artifact of that fact that libarrow used to be 3 libraries (libarrow/libarrow_io/libarrow_ipc). We should clean up the "api.h" files

          Show
          wesmckinn Wes McKinney added a comment - That's an artifact of that fact that libarrow used to be 3 libraries (libarrow/libarrow_io/libarrow_ipc). We should clean up the "api.h" files
          Hide
          tobyshaw Toby Shaw added a comment - - edited

          I mentioned earlier about C++ CLI complaining about nullptr usage. C++ CLI already had a nullptr keyword when C++11 introduced its own. This leads to incompatibilities when building with native projects which use nullptrs in header files.

          Annoyingly, the nullptrs which exist in header files are as default arguments, which need to live in headers. The recommended fix is to use __nullptr instead of nullptr, but I'm not sure if that is valid C++ outside of Visual C++

          (All these changes also apply to parquet-cpp)

          Show
          tobyshaw Toby Shaw added a comment - - edited I mentioned earlier about C++ CLI complaining about nullptr usage. C++ CLI already had a nullptr keyword when C++11 introduced its own. This leads to incompatibilities when building with native projects which use nullptrs in header files. Annoyingly, the nullptrs which exist in header files are as default arguments, which need to live in headers. The recommended fix is to use __nullptr instead of nullptr, but I'm not sure if that is valid C++ outside of Visual C++ (All these changes also apply to parquet-cpp)
          Hide
          wesmckinn Wes McKinney added a comment -

          Is there some way to detect C++CLI with an ifdef?

          Show
          wesmckinn Wes McKinney added a comment - Is there some way to detect C++CLI with an ifdef?
          Hide
          tobyshaw Toby Shaw added a comment -

          __cplusplus_cli seems to work, I'll do that.

          Show
          tobyshaw Toby Shaw added a comment - __cplusplus_cli seems to work, I'll do that.
          Hide
          wesmckinn Wes McKinney added a comment -

          Toby Shaw marked for 0.6.0. Would you be able to submit a patch?

          Show
          wesmckinn Wes McKinney added a comment - Toby Shaw marked for 0.6.0. Would you be able to submit a patch?
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user TobyShaw opened a pull request:

          https://github.com/apache/arrow/pull/1098

          ARROW-1134 - Allow C++/CLI projects to build with Arrow

          I'm going to attempt a few things:

          In public headers:
          Hide mutexes through pimpl pattern.
          Replace nullptr with __nullptr when building in C++/CLI mode.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/TobyShaw/arrow master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/arrow/pull/1098.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #1098


          commit d8cb1f676452af940bbc963c43ab3c5809d85459
          Author: labuser <labuser@labwks10>
          Date: 2017-09-13T10:34:21Z

          Attempt PImpl on FixedSizeBufferWriter

          commit d1903d7aa768a27e304c3f00d59cb725dfddf0f2
          Author: labuser <labuser@labwks10>
          Date: 2017-09-13T10:35:29Z

          remove mutex include


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user TobyShaw opened a pull request: https://github.com/apache/arrow/pull/1098 ARROW-1134 - Allow C++/CLI projects to build with Arrow I'm going to attempt a few things: In public headers: Hide mutexes through pimpl pattern. Replace nullptr with __nullptr when building in C++/CLI mode. You can merge this pull request into a Git repository by running: $ git pull https://github.com/TobyShaw/arrow master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/arrow/pull/1098.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1098 commit d8cb1f676452af940bbc963c43ab3c5809d85459 Author: labuser <labuser@labwks10> Date: 2017-09-13T10:34:21Z Attempt PImpl on FixedSizeBufferWriter commit d1903d7aa768a27e304c3f00d59cb725dfddf0f2 Author: labuser <labuser@labwks10> Date: 2017-09-13T10:35:29Z remove mutex include
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user TobyShaw commented on the issue:

          https://github.com/apache/arrow/pull/1098

          Work-in-progress.

          Show
          githubbot ASF GitHub Bot added a comment - Github user TobyShaw commented on the issue: https://github.com/apache/arrow/pull/1098 Work-in-progress.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user wesm commented on the issue:

          https://github.com/apache/arrow/pull/1098

          I'm happy to help you with this, I will let you work for a bit, but if you add me as a collaborator on your fork I can do some work on e.g. hiding mutex

          Show
          githubbot ASF GitHub Bot added a comment - Github user wesm commented on the issue: https://github.com/apache/arrow/pull/1098 I'm happy to help you with this, I will let you work for a bit, but if you add me as a collaborator on your fork I can do some work on e.g. hiding mutex
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user TobyShaw commented on the issue:

          https://github.com/apache/arrow/pull/1098

          Sure thing, added.

          Show
          githubbot ASF GitHub Bot added a comment - Github user TobyShaw commented on the issue: https://github.com/apache/arrow/pull/1098 Sure thing, added.

            People

            • Assignee:
              Unassigned
              Reporter:
              tobyshaw Toby Shaw
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development