Skip to content

Add support for Apple framework builds #2026

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

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?

@@ -127,7 +151,7 @@ endif()
include(CTest)

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

@@ -102,6 +121,11 @@ if(OPENEXR_BUILD_EXAMPLES AND OPENEXR_BUILD_LIBS)
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_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.

@@ -384,4 +384,4 @@ int main() {
if(NOT HAS_VLD1)
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