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

Ptex prefer static #121

Closed

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Dec 21, 2016

Description of Change(s)

Adds a cmake option, PTEX_PREFER_STATIC, which for linux and osx will make FindPTex.cmake use static versions of the ptex libraries preferentially to dynamic ones

Included Commit(s)

Fixes Issue(s)

  • none

@@ -27,6 +27,8 @@ if (WIN32)
"$ENV{PROGRAMFILES}/Ptex/include"
/usr/include
DOC "The directory where Ptexture.h resides")
# TODO: figure out a way to add windows support for PTEX_PREFER_STATIC
# (since both static and dynamic libs can have a ".lib" suffix)
Copy link
Member

@meshula meshula Dec 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ask the Ptex devs to name the libs according to some convention, like boost's? Doesn't solve the problem here of course, but one has to start somewhere :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I suspect something upstream (on the Ptex side) may be the only way to handle this - since I'm not even sure the libraries can / do exist side-by-side, like they do for OSX/Linux.

Or maybe it's just less of an issue on Windows - perhaps there the convention is that you simply have two completely separate builds for static + dynamic, and you would simply set PTEX_LOCATION accordingly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do it two ways, quarantine by directory, or by filename. filename has the advantage that the versions can co-exist in one directory and there will never ever be ambiguity.

It's a serious issue because in general with gcc/clang, if you mix and match release and debug dylibs, in general nothing terrible will happen, modulo missing inlined symbols. On Windows, debug libs and release libs have separate heaps so you can end up in the amazing world of allocating in one heap and releasing to another, yielding absolute havoc.

@sunyab
Copy link
Contributor

sunyab commented Jan 13, 2017

Hi @elrond79, would you mind changing the base branch for this pull request to the dev branch?
See https://github.com/blog/2224-change-the-base-branch-of-a-pull-request.

Thanks!

@pmolodo pmolodo changed the base branch from master to dev January 18, 2017 18:51
@pmolodo
Copy link
Contributor Author

pmolodo commented Jan 18, 2017

Done - didn't know github had that feature, thanks for pointing it out!

@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 3, 2017

Removed the TODO about windows support (since it is outside the scope of this project to allow this with a simple CMake option), and changed the description of the option to clarify that it only works on linux and osx.

@chadrik
Copy link
Contributor

chadrik commented Feb 23, 2017

Below are some snippets from the official FindBoost.cmake for setting a static library preference. I used a similar approach when adding this feature to OIIO. Perhaps we should adopt it here as well?

# Support preference of static libs by adjusting CMAKE_FIND_LIBRARY_SUFFIXES
if( Boost_USE_STATIC_LIBS )
  set( _boost_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
  if(WIN32)
    set(CMAKE_FIND_LIBRARY_SUFFIXES .lib .a ${CMAKE_FIND_LIBRARY_SUFFIXES})
  else()
    set(CMAKE_FIND_LIBRARY_SUFFIXES .a )
  endif()
endif()

...

# Restore the original find library ordering
if( Boost_USE_STATIC_LIBS )
  set(CMAKE_FIND_LIBRARY_SUFFIXES ${_boost_ORIG_CMAKE_FIND_LIBRARY_SUFFIXES})
endif()

Also, there's this bit which is used in calls to find_library to narrow the search for static libraries on windows:

# Setting some more suffixes for the library
set(Boost_LIB_PREFIX "")
if ( WIN32 AND Boost_USE_STATIC_LIBS AND NOT CYGWIN)
  set(Boost_LIB_PREFIX "lib")
endif()

Presumably building static boost libs on windows adds the 'lib' prefix. Can we rely on, or enforce, that behavior for Ptex?

There's also this note about the previous behavior:

# The previous behavior of FindBoost when Boost_USE_STATIC_LIBS was enabled
# on WIN32 was to:
#  1. Search for static libs compiled against a SHARED C++ standard runtime library (use if found)
#  2. Search for static libs compiled against a STATIC C++ standard runtime library (use if found)
# We maintain this behavior since changing it could break people's builds.
# To disable the ambiguous behavior, the user need only
# set Boost_USE_STATIC_RUNTIME either ON or OFF.

Good for learning from their previous mistakes.

@jtran56
Copy link

jtran56 commented Jun 2, 2017

Filed as internal issue #147122.

@pmolodo
Copy link
Contributor Author

pmolodo commented Apr 4, 2018

Going to close the PR, since there hasn't been much action, and we don't use it ourselves anymore either (since we just set PXR_ENABLE_PTEX_SUPPORT=OFF nowadays).

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.

6 participants