AcademySoftwareFoundation / openexr

The OpenEXR project provides the specification and reference implementation of the EXR file format, the professional-grade image storage format of the motion picture industry.
http://www.openexr.com/
BSD 3-Clause "New" or "Revised" License
1.62k stars 608 forks source link

Broken version header files? #1397

Open lgritz opened 1 year ago

lgritz commented 1 year ago

Possibly caused by #1386, OSL's "bleeding edge" CI test that uses top-of-main OpenEXR is getting this suspicious build break:

In file included from /home/runner/ext/dist/include/OpenEXR/ImfExport.h:9,
                 from /home/runner/ext/dist/include/OpenEXR/ImfForward.h:15,
                 from /home/runner/ext/dist/include/OpenEXR/ImfChannelList.h:16,
                 from /home/runner/work/OpenShadingLanguage/OpenShadingLanguage/src/liboslexec/shadingsys.cpp:34:
/home/runner/ext/dist/include/OpenEXR/OpenEXRConfig.h:11:10: fatal error: openexr_version.h: No such file or directory
   11 | #include <openexr_version.h>
      |          ^~~~~~~~~~~~~~~~~~~
compilation terminated.
meshula commented 1 year ago

Ah, I did need that installation line that we removed in code review. When I push a fix, will the "bleeding edge" CI test run, so I can monitor an outcome there?

lgritz commented 1 year ago

I can force it to run by poking the button to rerun one of the failed tests.

meshula commented 1 year ago

I think this fixes it https://github.com/AcademySoftwareFoundation/openexr/pull/1398

please LMK how that goes

meshula commented 1 year ago

@Vertexwahn This change impacts bazel/example. Sadly the break doesn't follow any of the ones you've helped me fix previously. I'm not sure why the example can't see the openexr dependencies. I'm guessing maybe

deps = ["//:OpenEXR", "//:OpenEXRCore"],

but I don't really understand why that's necessary, since it's not also specifying Imath and other dependencies.

lgritz commented 1 year ago

Fixed OSL's CI

meshula commented 1 year ago

Great, thanks for the confirmation. Enabling OpenEXRConfig.h to be able to include a file from OpenEXR needs another look; maybe the version should be parsed from the config.in instead to avoid all the CI, build system, and packaging jujitsu that goes along with a parameterized configuration file.

Vertexwahn commented 1 year ago

@meshula

Bazel assumes includes are relative to the WORKSPACE.bazel file. When trying to build Iex (bazel build //:Iex) you are including "OpenEXRCore/openexr_version.h" - but the expected path would be "src/lib/OpenEXRCore/openexr_version.h" (relative to the WORKSPACE file). Via the includes attribute of cc_library additional include dirs can be set. You need to add here "src/lib".

Here is the diff to fix the issues:

diff --git a/BUILD.bazel b/BUILD.bazel
index 0e932677..17140b27 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -105,6 +105,7 @@ cc_library(
         "//conditions:default": [],
     }),
     includes = [
+        "src/lib",
         "src/lib/Iex",
         "src/lib/OpenEXR",
         "src/lib/OpenEXRCore",
@@ -231,6 +232,7 @@ cc_library(
         "//conditions:default": [],
     }),
     includes = [
+        "src/lib",
         "src/lib/IlmThread",
         "src/lib/OpenEXR",
         "src/lib/OpenEXRCore",

Branch with this changes included is here: https://github.com/Vertexwahn/openexr/tree/fix-it

Imath is a transitive dependency of OpenEXR. To see the full dependency graph you can do:

bazel query --noimplicit_deps 'deps(//bazel/example:Demo)' --output graph > graph.in
dot -Tpng < graph.in > dependency_graph.png

local_execution_deps