openexr icon indicating copy to clipboard operation
openexr copied to clipboard

update namespacing options to reflect simpler structure

Open meshula opened this issue 4 years ago • 3 comments
trafficstars

OpenEXR 3 simplifies it's library structure, but didn't simplify namespacing.

set(OPENEXR_NAMESPACE_CUSTOM "0" CACHE STRING "Whether the namespace has been customized (so external users know)")
set(OPENEXR_INTERNAL_IMF_NAMESPACE "Imf_${OPENEXR_VERSION_API}" CACHE STRING "Real namespace for Imath that will end up in compiled symbols")
set(OPENEXR_IMF_NAMESPACE "Imf" CACHE STRING "Public namespace alias for OpenEXR")
set(OPENEXR_PACKAGE_NAME "OpenEXR ${OPENEXR_VERSION}" CACHE STRING "Public string / label for displaying package")

# Namespace-related settings, allows one to customize the
# namespace generated, and to version the namespaces
set(ILMBASE_NAMESPACE_CUSTOM "0" CACHE STRING "Whether the namespace has been customized (so external users know)")
set(ILMBASE_INTERNAL_IEX_NAMESPACE "Iex_${ILMBASE_VERSION_API}" CACHE STRING "Real namespace for Iex that will end up in compiled symbols")
set(ILMBASE_INTERNAL_ILMTHREAD_NAMESPACE "IlmThread_${ILMBASE_VERSION_API}" CACHE STRING "Real namespace for IlmThread that will end up in compiled symbols")
set(ILMBASE_IEX_NAMESPACE "Iex" CACHE STRING "Public namespace alias for Iex")
set(ILMBASE_ILMTHREAD_NAMESPACE "IlmThread" CACHE STRING "Public namespace alias for IlmThread")
set(ILMBASE_PACKAGE_NAME "IlmBase ${ILMBASE_VERSION}" CACHE STRING "Public string / label for displaying package")

Is there any reason not to strip it down to something like this, or is there an even simpler option? I seem to recall some recent discussion about simplifying the namespacing overall, in the c++ code.

set(OPENEXR_NAMESPACE_CUSTOM "0" CACHE STRING "Whether the namespace has been customized (so external users know)")
set(OPENEXR_INTERNAL_IMF_NAMESPACE "Imf_${OPENEXR_VERSION_API}" CACHE STRING "Real namespace for Imath that will end up in compiled symbols")
set(OPENEXR_IMF_NAMESPACE "Imf" CACHE STRING "Public namespace alias for OpenEXR")
set(OPENEXR_PACKAGE_NAME "OpenEXR ${OPENEXR_VERSION}" CACHE STRING "Public string / label for displaying package")

meshula avatar Mar 23 '21 20:03 meshula

I support this.

Additionally, since even in this case, OPENEXR_INTERNAL_IMF_NAMESPACE is a dependent symbol, on OPENEXR_VERSION_API which can change based on updates, it can cause some problems having it be a being a cache variable. Specifically it may not be reliably updated when the version updates.

I was hoping CMakeDependentOption might be a solution, but it looks like that's only for ON/OFF options, perhaps somethin

Perhaps a solution here would be to use, maybe something like:

set(OPENEXR_NAMESPACE_CUSTOM "0" CACHE STRING "Whether the namespace has been customized (so external users know)")
set(OPENEXR_IMF_NAMESPACE "Imf" CACHE STRING "Public namespace alias for OpenEXR")
if(OPENEXR_NAMESPACE_CUSTOM)
  set(OPENEXR_INTERNAL_IMF_NAMESPACE "Imf_${OPENEXR_VERSION_API}" CACHE STRING "Real namespace for Imath that will end up in compiled symbols")
  set(OPENEXR_PACKAGE_NAME "OpenEXR ${OPENEXR_VERSION}" CACHE STRING "Public string / label for displaying package")
else()
  set(OPENEXR_INTERNAL_IMF_NAMESPACE "Imf_${OPENEXR_VERSION_API}")
  set(OPENEXR_PACKAGE_NAME "OpenEXR ${OPENEXR_VERSION}")
endif()

ScatteredRay avatar Mar 23 '21 21:03 ScatteredRay

That looks like some old code, there shouldn't be any remaining references to ILMBASE in OpenEXR 3. If you're suggesting eliminating the Iex and IlmThread namespaces and putting those symbols into the Imf namespace, that will cause compatibility problems with application code, especially where the variables are set via command-line options via a legacy build system outside of CMake. @lgritz suggested collapsing the double "INTERNAL_IMATH" and "IMATH" namespaces into one, and I agree that would be simpler, although again it will cause compatibility issues with application code I'd rather avoid.

cary-ilm avatar Mar 23 '21 21:03 cary-ilm

My bad, the repo I copy/pasted from wasn't fully synched. Same number of options though on top of tree. I think I was hoping that Iex/IlmThread/OpenEXR could be further combined into a single unit, but that would be a different issue ;)

set(OPENEXR_NAMESPACE_CUSTOM "0" CACHE STRING "Whether the namespace has been customized (so external users know)")
set(OPENEXR_INTERNAL_IMF_NAMESPACE "Imf_${OPENEXR_VERSION_API}" CACHE STRING "Real namespace for Imath that will end up in compiled symbols")
set(OPENEXR_IMF_NAMESPACE "Imf" CACHE STRING "Public namespace alias for OpenEXR")
set(OPENEXR_PACKAGE_NAME "OpenEXR ${OPENEXR_VERSION}" CACHE STRING "Public string / label for displaying package")

# Namespace-related settings, allows one to customize the
# namespace generated, and to version the namespaces
set(ILMTHREAD_NAMESPACE_CUSTOM "0" CACHE STRING "Whether the namespace has been customized (so external users know)")
set(ILMTHREAD_INTERNAL_NAMESPACE "IlmThread_${OPENEXR_VERSION_API}" CACHE STRING "Real namespace for IlmThread that will end up in compiled symbols")
set(ILMTHREAD_NAMESPACE "IlmThread" CACHE STRING "Public namespace alias for IlmThread")

set(IEX_NAMESPACE_CUSTOM "0" CACHE STRING "Whether the namespace has been customized (so external users know)")
set(IEX_INTERNAL_NAMESPACE "Iex_${OPENEXR_VERSION_API}" CACHE STRING "Real namespace for Iex that will end up in compiled symbols")
set(IEX_NAMESPACE "Iex" CACHE STRING "Public namespace alias for Iex")

meshula avatar Mar 24 '21 01:03 meshula