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

🐛 Remove the OPENEXR_INSTALL_EXAMPLES CMake option #1666

Merged

Conversation

Jamim
Copy link
Contributor

@Jamim Jamim commented Mar 10, 2024

Hello @cary-ilm,

The OPENEXR_INSTALL_EXAMPLES option is misleading and doesn't have any effect.
It might be safely removed in favor of the OPENEXR_BUILD_EXAMPLES option.

Best regards!

Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

One small suggestion below, but otherwise perfect, thanks! Last time I looked I suspected that was superfluous but didn't do anything about it.

@@ -460,7 +460,7 @@ Component Options
Install the binary programs (exrheader, exrinfo,
exrmakepreview, etc). Default is ``ON``.

* ``OPENEXR_INSTALL_EXAMPLES``
* ``OPENEXR_BUILD_EXAMPLES``

Build and install the example code. Default is ``ON``.
Copy link
Member

Choose a reason for hiding this comment

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

Should remove any reference to install here:

Suggested change
Build and install the example code. Default is ``ON``.
Build the example code. Default is ``ON``.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I've submitted an updated commit.

The OPENEXR_INSTALL_EXAMPLES option is misleading and doesn't have any effect.
It might be safely removed in favor of the OPENEXR_BUILD_EXAMPLES option.

Co-authored-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Aliaksei Urbanski <aliaksei.urbanski@gmail.com>
@Jamim Jamim force-pushed the fix/build-examples-option branch from 4811de4 to bd47a0a Compare March 10, 2024 18:31
@Jamim Jamim requested a review from cary-ilm March 10, 2024 18:34
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

Thanks!

@cary-ilm cary-ilm merged commit 82d0275 into AcademySoftwareFoundation:main Mar 10, 2024
31 checks passed
@Jamim Jamim deleted the fix/build-examples-option branch March 11, 2024 09:20
pleprince pushed a commit to pleprince/openexr_clusty that referenced this pull request Mar 12, 2024
…undation#1666)

The OPENEXR_INSTALL_EXAMPLES option is misleading and doesn't have any effect.
It might be safely removed in favor of the OPENEXR_BUILD_EXAMPLES option.

Signed-off-by: Aliaksei Urbanski <aliaksei.urbanski@gmail.com>
Co-authored-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Philippe Leprince <github.pleprince@gmail.com>
cary-ilm added a commit that referenced this pull request Mar 23, 2024
The OPENEXR_INSTALL_EXAMPLES option is misleading and doesn't have any effect.
It might be safely removed in favor of the OPENEXR_BUILD_EXAMPLES option.

Signed-off-by: Aliaksei Urbanski <aliaksei.urbanski@gmail.com>
Co-authored-by: Cary Phillips <cary@ilm.com>
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