openexr icon indicating copy to clipboard operation
openexr copied to clipboard

[OpenEXRCore] angle bracket includes within the OpenEXR library should have an OpenEXR/ prefix

Open anderslanglands opened this issue 3 years ago • 6 comments

https://github.com/kdt3rd/openexr/blob/017f056239e817cbe2ab601efe99dc9f4c9e7f52/src/lib/OpenEXRCore/openexr_conf.h#L10

Is there a reason to use angled brackets for including headers rather than quotes? This appears to break clang. I can work around it by passing both -I${OPENEXR_ROOT}/include and -I${OPENEXR_ROOT}/include/OpenEXR but this is a bit hacky.

anderslanglands avatar Jul 16 '21 23:07 anderslanglands

That's typical for ASWF libraries and ASWF adjacent libraries. Like OIIO for example, uses angle brackets. If you're noticing an issue with OpenEXR in particular, is it because other libraries like OIIO are installed site wide, and you've got a -I for your site headers, but OpenEXR is not in the site wide location?

meshula avatar Jul 17 '21 03:07 meshula

But it's not supposed to be "bare".

#include <OpenEXR/OpenEXRConfig.h>

is fine. But

#include <OpenEXRConfig.h>

is not going to find it in the include directory.

lgritz avatar Jul 17 '21 05:07 lgritz

Yep that is indeed the issue.

On Sat, 17 Jul 2021 at 17:45, Larry Gritz @.***> wrote:

But it's not supposed to be "bare".

#include <OpenEXR/OpenEXRConfig.h>

is fine. But

#include <OpenEXRConfig.h>

is not going to find it in the include directory.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/openexr/issues/1095#issuecomment-881835117, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOYQXIOVH6ILREYHJ67CVLTYEKHBANCNFSM5AQMYUOQ .

anderslanglands avatar Jul 17 '21 06:07 anderslanglands

Ah, I'll update the issue title.

meshula avatar Jul 17 '21 22:07 meshula

I'm not entirely sure the correct solution here. #1097 changes the <> to "" inside of all exported headers, for the few files that were using <>. #include <OpenEXR/OpenEXRConfig.h> does not work; it might work in application code, but it doesn't work in building OpenEXR itself.

OpenEXR.pc includes both -I -I<include/OpenEXR>, which I think we did for backwards compatibility.

What about includes of Imath from inside OpenEXR headers? We're inconsistent, with some included via "" and others with <>. Should they all be ""?

cary-ilm avatar Jul 18 '21 19:07 cary-ilm

I think long-term, Imath headers should be <Imath/ImathBox.h>, but you’re still supporting everything being installed under include/OpenEXR for now right? In which case using “ImathBox.h” and adding both -Iimath/include and -Iimath/include/Imath works.

OIIO is able to use the <OpenImageIO/imageio.h> form because it splits source and public headers in the build, whereas everything’s together in OpenEXR, which means “OpenEXRConfig.h” rather than <OpenEXR/OpenEXRConfig.h> is the only way to make both build and dependencies work with a single include path without restructuring the source tree.

Probably the cleanest solution would be to wait until you're ready to break compatibility with the old "everything under include/OpenEXR" method of installation and then revisit, choosing either modifying all public includes to quotes, or restructuring the soruce tree such that <OpenEXR/OpenEXRConfig.h> would work.

In the meantime, this is only really an issue for projects like ours like are not using CMake for getting include paths and have to remember to specify both include paths (and I'm probably going to modify our stuff to use CMake for grabbing includes anyway at some point before too long, we already do it for link args).

On Mon, 19 Jul 2021 at 07:03, Cary Phillips @.***> wrote:

I'm not entirely sure the correct solution here. #1097 https://github.com/AcademySoftwareFoundation/openexr/pull/1097 changes the <> to "" inside of all exported headers, for the few files that were using <>. #include <OpenEXR/OpenEXRConfig.h> does not work; it might work in application code, but it doesn't work in building OpenEXR itself.

OpenEXR.pc includes both -I -I<include/OpenEXR>, which I think we did for backwards compatibility.

What about includes of Imath from inside OpenEXR headers? We're inconsistent, with some included via "" and others with <>. Should they all be ""?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/openexr/issues/1095#issuecomment-882102999, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOYQXIBYIUOB25S7R5FORLTYMQR5ANCNFSM5AQMYUOQ .

anderslanglands avatar Jul 18 '21 19:07 anderslanglands