-
Notifications
You must be signed in to change notification settings - Fork 333
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 compiling with OSL 1.11.8 #2900
Conversation
@@ -403,6 +403,7 @@ foreach (osl_source ${osl_sources}) | |||
OUTPUT ${oso_dir}/${oso_filename} | |||
COMMAND ${CMAKE_COMMAND} -E make_directory ${oso_dir} | |||
COMMAND ${PROJECT_SOURCE_DIR}/sandbox/bin/oslc ${OSLC_OPTIONS} | |||
-I"${PROJECT_SOURCE_DIR}/sandbox/shaders/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSL 1.11.7 and 1.11.8 changed how stdosl.h
is looked up: INSTALL_PREFIX/share/OSL/shaders takes precedence over ../shaders (where INSTALL_PREFIX is the compile time path of the installation).
However, if stdosl.h
found in the includes it takes the highest precedence.
These changes were introduced in AcademySoftwareFoundation/OpenShadingLanguage#956 and the relevant change can be seen here:
https://github.com/imageworks/OpenShadingLanguage/blob/8ff28a0e062c714b74244cc89fd55461421c5a97/src/liboslcomp/oslcomp.cpp#L334-L341
// wrap_python.hpp undefines _DEBUG and then redefines it, losing its value of 1 | ||
// In OSL 1.11.8 osl/platform.h header assumes that _DEBUG=1, otherwise the | ||
// header can't be compiled | ||
#ifdef _DEBUG | ||
#define _OLD_DEBUG _DEBUG | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap_python undefines _DEBUG and then redefines it, which makes it lose its value of 1. This value is used in osl/platform.h here
https://github.com/imageworks/OpenShadingLanguage/blob/8ff28a0e062c714b74244cc89fd55461421c5a97/src/include/OSL/platform.h#L229-L232
where OSL_DEBUG = _DEBUG as of here
https://github.com/imageworks/OpenShadingLanguage/blob/8ff28a0e062c714b74244cc89fd55461421c5a97/src/include/OSL/platform.h#L220-L222
// wrap_python.hpp undefines _DEBUG and then redefines it without assigning it value of 1 | ||
// In OSL 1.11.8 osl/platform.h header uses the syntax "#if _DEBUG" which assumes | ||
// that _DEBUG is not an empty macro, otherwise the header can't be compiled | ||
#ifdef _DEBUG | ||
#undef _DEBUG | ||
#define _DEBUG 1 | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is a conflit between boost and OSL ?
Why putting this here exactly ? There isn't any OSL include bellow those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. wrap_python.hpp
undefines "_DEBUG" then redefines it as an empty macro.
https://github.com/boostorg/python/blob/f1320bf30b4887aff28b9fc67b98f6f8be8c3684/include/boost/python/detail/wrap_python.hpp#L45
https://github.com/boostorg/python/blob/f1320bf30b4887aff28b9fc67b98f6f8be8c3684/include/boost/python/detail/wrap_python.hpp#L209
I added this here directly after "wrap_python.hpp" to fix the macro definition.
Personally I think that the syntax #if OSL_DEBUG
instead of #ifdef OSL_DEBUG
used in OSL is suboptimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. We have some headers in appleseed wich goal is to include another header and then fix some things, like you are doing here. This ensure that the fix will be avilable everywhere we need wrap_python
without having to write the fix multiple times.
@dictoon Can you confirm this ? Do you have an example in mind ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @theartful !
Confirmed 👍 Work on ArchLinux against |
Fixes #2892