Skip to content
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.

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

Fix OpenVDB Python module with NumPy compilation on Windows MSVC #1755

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MichaelMoroz
Copy link

I couldn't compile the OpenVDB python module with NumPy due to ssize_t missing on MSVC. This hopefully fixes it.

@MichaelMoroz MichaelMoroz requested a review from kmuseth as a code owner January 23, 2024 17:29
Copy link

linux-foundation-easycla bot commented Jan 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: MichaelMoroz / name: Mykhailo Moroz (2f2d86b)

@jmlait
Copy link
Contributor

jmlait commented Feb 13, 2024

Thank you for this. Unfortunately we need two TLAs: CLA and DCO. Can you please re-do the commit with --signoff so it passes the DCO test? Thank you.

@NeedsMoar
Copy link

Just a comment but ssize_t is absent on Windows because it's a POSIX specific data type which is poorly defined in the POSIX standard (-1 is the only guaranteed negative value, there isn't a guarantee about the size of the data type). The Windows header you used defines it as LONG_PTR which is int64_t on 64 bit windows and int32_t otherwise, but the leftover POSIX functions on Windows return int where ssize_t would have been used.

AFAIK it's not defined or used in the newest or any previous version of the C standard so the proper fix is to call the Windows version of whatever POSIX function is involved in returning it. Python does a fairly good job of that internally even where it uses a posix name in code... If numpy is returning ssize_t somewhere on Windows, it's an error, and it would be preferable to file a bug with them to conditionalize that or just use standard C types (even on Linux really, it's not like they're paying to be POSIX certified on a yearly basis or much else is going on in the UNIX world but BSDs) rather than propagating what amounts to an error somewhere, IMO.

@francoiscoulon
Copy link

francoiscoulon commented Mar 5, 2024

Hi,

I had the same issue and I have food for thoughts here with my humble suggestion down at the end that I used to fix this on my end.

  1. Here the preprocessor macro is using _MSC_VER limiting to visual studio compiler. To work with _WIN32, we'd have to look for another solution, leading to:
  2. Understand what ssize_t really means in both posix and windows and if there is a standard solution.
    • ssize_t by posix:
      The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}] and shall be signed integer type.
    • SSIZE_T in msvc BaseTsd.h:
      A signed version of SIZE_T. This type is declared in BaseTsd.h as follows:
      typedef LONG_PTR SSIZE_T;
    • Standard C++ library has std::ptrdiff_t:
      [It] acts as the signed counterpart of size_t.

With all these information(links included) we see we should be able to use std::ptrdiff_t.

Quick workaround more "generic" could be:

#if defined (_WIN32)
#include <cstddef> // for std::ptrdiff_t
#define ssize_t std::ptrdiff_t
#endif

A fix could be to use std::ptrdiff_t instead of ssize_t all over.

I'm happy to create a MR with either these suggestions if you think they're improving Michael's suggestion...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants