Skip to content

Conversation

Treata11
Copy link

No description provided.

@Treata11
Copy link
Author

Addressed #2025 in d6e4e9c

Signed-off-by: Treata11 <[email protected]>
@Treata11
Copy link
Author

A question about OpenEXRLibraryDefine.cmake.

Why does it exist in the first place? It's not utilized anywhere & the LibraryDefine.cmake is used instead...

Why two similar files in the first place?


if(BUILD_TESTING AND OPENEXR_BUILD_LIBS AND NOT OPENEXR_IS_SUBPROJECT)
add_subdirectory(src/test)
add_subplot(src/test)
Copy link
Member

Choose a reason for hiding this comment

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

What is add_subplot()?

option(OPENEXR_FRAMEWORK "Build as Apple Frameworks" OFF)
endif ()

if(OPENEXR_FRAMEWORK)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to name this OPENEXR_BUILD_APPLE_FRAMEWORKS

add_subdirectory( src/examples )
endif()

if (OPENEXR_BUILD_LIBS AND NOT OPENEXR_IS_SUBPROJECT)
Copy link
Member

@cary-ilm cary-ilm Apr 29, 2025

Choose a reason for hiding this comment

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

Is there a reason this moved? Better to minimize changes unless there's a specific reason.

# Set OUTPUT_NAME to avoid suffix for frameworks
set_target_properties(${libname} PROPERTIES
OUTPUT_NAME "${libname}${OPENEXR_LIB_SUFFIX}"
OUTPUT_NAME "${libname}"
Copy link
Member

Choose a reason for hiding this comment

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

The output name needs the OPENEXR_LIB_SUFFIX.

set(OPENEXR_MISSING_ARM_VLD1 TRUE)
endif()
endif()
endif()
Copy link
Member

@cary-ilm cary-ilm Apr 29, 2025

Choose a reason for hiding this comment

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

Why remove the newline?

In fact, it looks like all your edited files now lack a newline as the final character, which is odd, an artifact of your text editor, perhaps? Please add them back in.

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.

2 participants