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

Include exported OpenEXR headers with "" instead of <> #1097

Merged

Conversation

cary-ilm
Copy link
Member

Signed-off-by: Cary Phillips cary@ilm.com

Signed-off-by: Cary Phillips <cary@ilm.com>
Comment on lines 25 to 26
#include <ImathBox.h>
#include <half.h>
Copy link
Contributor

@lgritz lgritz Jul 18, 2021

Choose a reason for hiding this comment

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

shouldn't these be

<Imath/ImathBox.h>

etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would seem reasonable, but our CMake files must not be set up that way. The compile flags have -I_build/_deps/imath-src/src/Imath. The Imath headers only end up in the Imath subfolder after the install step. During the build, they're referenced out of the cloned source directory.

If you configure cmake with -DCMAKE_INSTALL_PATH=, then it works for the OpenEXR headers to include <Imath/ImathBox.h>, because the cflags have -I/include.

@@ -18,9 +18,9 @@
#include "ImfUtilExport.h"

#include <IexBaseExc.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

what about this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, missed that one, and another reference to in ImfAttribute.h. I pushed another commit to fix these.

Signed-off-by: Cary Phillips <cary@ilm.com>
@cary-ilm
Copy link
Member Author

I'd like to get this in for 3.1, does anyone see any issues? It doesn't resolve everything but at least it makes #includes consistent for OpenEXR library headers.

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

irrespective of the angle question, consistency is good. Since you didn't change the angle references to Imath, I'm assuming that's also in the realm of consistency.

@cary-ilm cary-ilm merged commit c42bb20 into AcademySoftwareFoundation:master Jul 19, 2021
@cary-ilm cary-ilm added the v3.1.0 label Sep 2, 2021
@cary-ilm cary-ilm deleted the core-angle-brackets branch November 5, 2022 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants