search

PixarAnimationStudios / OpenSubdiv

An Open-Source subdivision surface library.
graphics.pixar.com/opensubdiv
Other
2.88k stars 558 forks source link

Renaming limit.h in opensubdiv/bfr (3.5.0) #1300

Closed dleex closed 1 year ago

dleex commented 1 year ago

Hi. On macOS we're running into problems compiling the library due to naming conflict of 'limit.h' with standard headers in 'opensubdiv/bfr' dir. I believe you can reproduce this issue by running cmake from project root (instead of a separate 'build' directory - please don't ask). We're currently working around this problem by renaming this file, but we'd appreciate an official solution. Thank you.

davidgyu commented 1 year ago

Filed as internal issue #OSD-412

davidgyu commented 1 year ago

We're going to have to ask for more details...

We don't expect this to be a problem since the expected way of including that header is using a file name qualified path, e.g. #include <opensubdiv/bfr/limits.h> If you take a look at tutorials/bfr/tutorial_3_1/customSurfaceFactory.cpp you will find an example including that header as well as the standard header #include <limits> without any problems that we know of on macOS or any other platform.

dleex commented 1 year ago

We noticed it in various configurations (MacOS 11, 12, 13 with clang 12, 13, and 14): You should be able to reproduce it by following the instruction on readme (https://github.com/PixarAnimationStudios/OpenSubdiv#macos) but instead using subdirectory (eg 'build') from project root, run within it (and of course, you'll have to replace '..' on cmake command line argument with '.')

davidgyu commented 1 year ago

We still would like to know why you want to build this way.

Any client of OpenSubdiv should need only files installed within the CMAKE_INSTALL_PREFIX and should never need to access source files directly. If there's something additional that you would like to be installed, we'd be curious to know and could discuss that.

Otherwise, without knowing more we'd suggest just building within a separate build directory as that's the configuration we test and has been sufficient for all other client use cases that we know about.

davidgyu commented 1 year ago

Marking this closed based on the discussion above. The name spaced include path is sufficient to distinguish this from other headers, and without knowing more we aren't able to suggest other alternatives.