-
Notifications
You must be signed in to change notification settings - Fork 201
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
Use proper boost target name #637
Conversation
@@ -201,7 +201,7 @@ target_link_libraries(${LIBRARY_NAME} | |||
usdImagingGL | |||
vt | |||
Boost::python | |||
$<IF:$<BOOL:${IS_WINDOWS}>,Boost::thread,${Boost_THREAD_LIBRARY}> | |||
Boost::thread |
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.
Because of my change in PR #602 , we should now use the proper boost target name here. This is needed for python3 builds where Boost_THREAD_LIBRARY is not defined.
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.
Seems reasonable to me!
target and version.
5de3c45
to
86943fd
Compare
@mattyjams Sorry I had to rework it again to handle both our python 2 and 3 builds. |
# There were bugs in both cmake and findboost with the boost version string. So in | ||
# order to support all boost and cmake versions, we check how the boost version is | ||
# defined and create our version comparison string. | ||
if(${Boost_VERSION} MATCHES "^([0-9]+)\\.([0-9]+)\\.([0-9]+)$") | ||
set(boost_1_70_0_ver_string "1.70.0") | ||
else() | ||
set(boost_1_70_0_ver_string "107000") | ||
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.
USD uses a different version of boost on Windows (1.70.0) and Linux/Mac, python 2 (1.66.0) and python 3 (1.70.0). Because of this the Boost_VERSION string has a different format between the two versions. And Boost_VERSION_STRING is not always set. There is also a fix in cmake 3.15, but we support 3.13-3.17.
-- Boost_VERSION = 106100
-- Boost_VERSION_STRING = 1.61.0
-- Boost_VERSION = 1.70.0
-- Boost_VERSION_STRING =
So in order to work around all of this I detect the format of the boost version and set a string for 1.70.0 to compare against.
@@ -201,7 +210,7 @@ target_link_libraries(${LIBRARY_NAME} | |||
usdImagingGL | |||
vt | |||
Boost::python | |||
$<IF:$<BOOL:${IS_WINDOWS}>,Boost::thread,${Boost_THREAD_LIBRARY}> | |||
$<IF:$<VERSION_GREATER_EQUAL:${Boost_VERSION},${boost_1_70_0_ver_string}>,Boost::thread,${Boost_THREAD_LIBRARY}> |
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.
If we are 1.70.0 or greater use the Boost::thread, otherwise use Boost_THREAD_LIBRARY. Note: in 1.70.0, Boost_THREAD_LIBRARY is not set because we are setting boost_root to the USD location so it uses the boost config cmake files there.
I would like to use Boost::thread all the time, but that causes a problem with v1.66.0 on the Mac in that it brings in Boost::chrono, but we don't have that.
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.
Yikes. Definitely less pretty, but it seems well contained and is probably ok for now.
Maybe it's worth taking a pass and seeing whether any of this code actually depends on any Boost::thread
-specific features or behaviors? We might be able to get away with swapping existing usages over to their std
equivalents and just dropping the Boost::thread
dependency all together.
The more boost we can remove the better! Thanks. |
No description provided.