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

Support for OpenEXR-3 / Imath? #100

Open
waebbl opened this issue Oct 3, 2022 · 18 comments
Open

Support for OpenEXR-3 / Imath? #100

waebbl opened this issue Oct 3, 2022 · 18 comments

Comments

@waebbl
Copy link
Contributor

waebbl commented Oct 3, 2022

Hi,

is there a chance, the library will support OpenEXR-3 / Imath in the foreseeable future?

The package is one of very few in Gentoo Linux, which still needs the old OpenEXR-2.5 / IlmBase packages. As a maintainer of these packages, I'm currently checking removal of these versions from the repository.

TIA

@lgritz
Copy link

lgritz commented Oct 3, 2022

Does anything else in Gentoo use CTL as a dependency? It's a bit of a specialized thing, I'm a little curious how it got swept into a major Linux distro in the first place.

Note: there doesn't appear to have been a tagged release of CTL since 2014?

@waebbl
Copy link
Contributor Author

waebbl commented Oct 4, 2022

No package in Gentoo depends on CTL. It has been in Gentoo for many years and got moved from CVS when we migrated to Git back in 2015. Eventually someone adapted it from BDS ports, on which the Gentoo package manager is based off, but it's origin is currently unknown to me. It's on a few other major distros as well[1][2].

As long as it builds, no major bugs are reported and the package is still maintained, it's age isn't very important.

[1] https://repology.org/project/ctl/versions
[2] https://repology.org/project/ampasctl/versions

@michaeldsmith
Copy link
Collaborator

michaeldsmith commented Oct 4, 2022

Hi - I volunteered to be the maintainer of CTL earlier this year. So far I've focused on fixing the bugs that impacted my own usage of CTL and ctlrender in my work. I also added CI workflows including valgrind and addressanitizer, and I fixed the bugs that were found by those tools.

@waebbl we could work on supporting OpenEXR-3/Imath if you think its important, is your goal to completely remove OpenEXR 2.5.x ? It appears that the OpenEXR 2.5.x branch is still under active maintenance (last release was March 2022).

@lgritz would it help if we made a new 1.5.3 tag release of CTL? Do you think it would need to include anything more than the bug fixes I've already committed to master branch?

@lgritz
Copy link

lgritz commented Oct 4, 2022

I think we should take a hard look at whether anybody actually needs the Gentoo pre-built CTL package. If not, they should drop it from their distro in an effort to clear the way for them to upgrade to OpenEXR/Imath 3.1 for the rest of their packages that need OpenEXR/Imath.

OpenEXR 2.5 is definitely past its support window. In March 2022 we tagged a release that contained two very minor build-related fixes, probably as a favor for somebody who specifically requested it. The previous 2.5 patch was almost a year earlier and only contained fixes to critical security bugs. Currently, we're working toward 3.2 and I think I can speak for the OpenEXR dev team and say that 2.x should not expect any further updates and it's our position that everybody should be moving to 3.x ASAP.

@waebbl
Copy link
Contributor Author

waebbl commented Oct 5, 2022

@michaeldsmith Sadly I can not say how important the package is. We don't have reliable numbers on how often a package is installed by users. It doesn't hurt to keep IlmBase / OpenEXR-2.5 around for a while longer, as long as the packages build and security issues are getting fixed. If there's a decision to not upgrade CTL to use Imath / OpenEXR-3, we might eventually have to drop CTL from the repo at some point, if IlmBase / OpenEXR-2 don't get any more security updates.

@lgritz We don't have pre-built CTL packages. Gentoo is a source-base rolling release distro. The package also doesn't block upgrades for packages using OpenEXR-3 / Imath. We currently have packages for v3.1 and v2.5, but they can't be installed at the same time. So if someone wants to install CTL it will in fact block installing the many packages which already use OpenEXR-3 / Imath, including packages like blender, krita, osl or oiio. At the beginning of v3 we had a way to install both, v3 and v2.x in parallel, but this was very fragile and we needed to manually change a few things in the build system for either v3 or v2.x, so we dropped this.

A list of packages depending on IlmBase can be seen at [1]. [B] means blocking, i.e. the package can't be installed at the same time with IlmBase. I checked this list and all packages, but CTL also have newer version in the repo which use OpenEXR-3 / Imath, so all of these could be dropped. One other package YafaRay is using OpenEXR-2, but we can disable it by dropping optional support for EXR format until they come up with a newer release.

[1] https://qa-reports.gentoo.org/output/genrdeps/rindex/media-libs/ilmbase

waebbl added a commit to waebbl/gentoo that referenced this issue Nov 15, 2022
Package currently doesn't build with OpenEXR-3/Imath. The patch
restricts to <media-libs/openxr-3

Bug: ampas/CTL#100
Closes: https://bugs.gentoo.org/878247
Signed-off-by: Bernd Waibel <[email protected]>
@michaeldsmith
Copy link
Collaborator

@waebbl I have merged pull request #103 into CTL master branch which enables building CTL and ctlrender without the OpenEXR v2 library. The IlmBase library is still required. Does that help address your original issue and question? It seems IlmBase is still available from modern ubuntu package mangers. I'm not sure if Gentoo will continue to support IlmBase. Also, I don't know if its possible to keep a legacy IlmBase v2 and a newer OpenEXR v3 on the same system.

@lgritz do you have any interest in submitting a pull request to CTL that would update it to use the latest OpenEXR v3 libraries instead of IlmBase ?

@waebbl
Copy link
Contributor Author

waebbl commented Nov 15, 2022

@michaeldsmith Thanks for your effort in updating the library. It will not help with my original issue though. We still have OpenEXR-2.5 and IlmBase packages in Gentoo, but they are both definitely bound for EOL. There's no set date for this however, and if there's effort to update to OpenEXR-3 I think we can keep them some more time. On Gentoo it's currently not possible to have OpenEXR-3 and IlmBase-2 packages installed on the same system at the same time. AFAIR this will lead to file collisions for some header files which are in both, OpenEXR-3 and IlmBase, like for example Iex.h.
If I read your patch in #103 correctly, OpenEXR is only needed for one test suite. This gives me the opportunity to restrict the specific test for the test suite, and build with only IlmBase support. I'm trying to use your patch in my PR gentoo/gentoo#28278 and remove the dependency on OpenEXR.

waebbl added a commit to waebbl/gentoo that referenced this issue Nov 15, 2022
Package currently doesn't build with OpenEXR-3/Imath. This revision adds
an upstream patch to disable certain tests if no OpenEXR-2 is available
and thus needs to depend on IlmBase only.

Bug: ampas/CTL#100
Closes: https://bugs.gentoo.org/878247
Signed-off-by: Bernd Waibel <[email protected]>
@lgritz
Copy link

lgritz commented Nov 15, 2022

Keeping an old version of the "IlmBase" portions of OpenEXR 2.5 in an otherwise Imath-3.x world will cause all sorts of subtle problems with conflicting header files and symbols. Plus, those 2.x versions no longer get updates, even for critical security bugs, which puts any project that uses them in danger of having vulnerabilities discovered that could prevent them from being used in commercial products or being included in OS distributions. CTL really needs to be converted to a modern, maintained version of Imath.

I'm not a good candidate for writing a PR for CTL, both because my time is already wildly oversubscribed, and also I have never built or used CTL and am not really familiar with its codebase -- I'd be starting from scratch. Surely there is somebody better suited than me to do this work.

I did write a porting guide with the specific goal of making it so I wouldn't have to do the conversion myself in other code bases. That was actually done for the 3.0 release when we thought people would need to simultaneously support OpenEXR/Imath 2.x and 3.x for some time. Now that it's a few years later, if you can take the leap and require 3.0 as a minimum, it's even simpler because you won't need all the #if nonsense that handles either version.

waebbl added a commit to waebbl/gentoo that referenced this issue Nov 16, 2022
Package currently doesn't build with OpenEXR-3/Imath. This revision adds
an upstream patch to disable certain tests if no OpenEXR-2 is available
and thus needs to depend on IlmBase only.

Bug: ampas/CTL#100
Closes: https://bugs.gentoo.org/878247
Signed-off-by: Bernd Waibel <[email protected]>
waebbl added a commit to waebbl/gentoo that referenced this issue Nov 17, 2022
Package currently doesn't build with OpenEXR-3/Imath. This revision adds
an upstream patch to disable certain tests if no OpenEXR-2 is available
and thus needs to depend on IlmBase only.

Bug: ampas/CTL#100
Closes: https://bugs.gentoo.org/878247
Signed-off-by: Bernd Waibel <[email protected]>
waebbl added a commit to waebbl/gentoo that referenced this issue Nov 18, 2022
Package currently doesn't build with OpenEXR-3/Imath.
Because the package uses automagic dependencies if OpenEXR-2 is available,
we still need to depend on <media-libs/openexr-3 for now.
Upstream is in the process of migrating the package to Imath/OpenEXR-3.

Bug: ampas/CTL#100
Closes: https://bugs.gentoo.org/878247
Signed-off-by: Bernd Waibel <[email protected]>
@michaeldsmith
Copy link
Collaborator

michaeldsmith commented Dec 24, 2022

@waebbl I just merged #105 that adds support for OpenEXR-3 and Imath. Can you let me know if this resolves your issue? Do you have any other feedback?

@lgritz thanks for your suggestions and mentioning your porting guide, it was helpful. I would like to maintain support for both OpenEXR2 and OpenEXR3 until all the package managers pick-up OpenEXR3. Please let me know if you have any feedback.

@waebbl
Copy link
Contributor Author

waebbl commented Dec 24, 2022

@michaeldsmith I can test next week. Left a comment on a missing use of the GNUInstallDirs variable.

@waebbl
Copy link
Contributor Author

waebbl commented Dec 31, 2022

@michaeldsmith I can successfully build the package at commit 3fc4ae7 and the tests run successfully using Imath-3.1.6 and OpenEXR-3.1.5.

Some feedback for further improvements on the current build files:

  • Neither the pkg-config nor the cmake config files are currently installed. There's a code snippet in OpenEXR_CTL/CMakeLists.txt which installs this pkg-config file, but there's no find_package call for pkg-config, so PKG_CONFIG_FOUND is always false and the file doesn't get installed.
  • The *.ctl files are installed into /CTL. Shouldn't they get installed into either ${CMAKE_INSTALL_DATADIR}/CTL or ${CMAKE_INSTALL_LIBDIR}/CTL?
  • The header file lib/IlmImfCtl/ImfCtlApplyTransform.h is installed into ${CMAKE_INSTALL_LIBDIR}/OpenEXR instead of ${CMAKE_INSTALL_INCLUDEDIR}/OpenEXR.

waebbl added a commit to waebbl/gentoo that referenced this issue Dec 31, 2022
Upstream has updated to be compatible to Imath / OpenEXR-3, but no new
version released yet, so we use the commit id to fetch the correct version.

Bug: ampas/CTL#100
Closes: https://bugs.gentoo.org/878247
Signed-off-by: Bernd Waibel <[email protected]>
@waebbl
Copy link
Contributor Author

waebbl commented Dec 31, 2022

You can pick a patch for the above mentioned changes from https://github.com/gentoo/gentoo/pull/28906/files#diff-d01076278b117f6aad20b60e7cce7f7712310822e8d401e376b1539051fd2d46 if you like. I you prefer, I can also open a pull request with the changes.

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Jan 1, 2023
Upstream has updated to be compatible to Imath / OpenEXR-3, but no new
version released yet, so we use the commit id to fetch the correct version.

Bug: ampas/CTL#100
Closes: https://bugs.gentoo.org/878247
Signed-off-by: Bernd Waibel <[email protected]>
Closes: #28906
Signed-off-by: John Helmert III <[email protected]>
@michaeldsmith
Copy link
Collaborator

michaeldsmith commented Jan 2, 2023

@waebbl thanks for the additional review. Can you please submit a pull request that fixes the issues you have found? I would also be interested to hear if you have any additional ideas about cleaning up the build and configuration files.

Also, it could help me test with Gentoo if you could post a Gentoo Dockerfile to this issue that can be used for testing CTL with Gentoo using Docker. I could also try adding a Gentoo Dockerfile to the Github workflow CI.

@waebbl
Copy link
Contributor Author

waebbl commented Jan 3, 2023

@michaeldsmith I submit a PR in the next few days and notice you if I have any ideas on cleaning up the build.

It's been an extended time, since I last used docker, but I see what I can do to provide a Dockerfile for you to test the build. I'm not sure though if it's meaningful to add it to the GitHub CI workflow, but it's up to you. Gentoo isn't a mainstream distribution, and not widely used by the average user. Common Use Cases for Gentoo include people who like to build the system on their machine, in contrast to installing pre-built packages, and people who like to have an individual setup in terms of compiler configuration and needed / installed dependencies for packages.

waebbl added a commit to waebbl/CTL that referenced this issue Jan 7, 2023
Use GNUInstallDirs variable throughout the build system.

Bug: ampas#100
Signed-off-by: Bernd Waibel <[email protected]>
@waebbl
Copy link
Contributor Author

waebbl commented Jan 7, 2023

@michaeldsmith The search for IlmBase / Imath and OpenEXR seems to be overcomplex. Currently there are several checks if Imath or IlmBase are found, as well if we have =OpenEXR-3 available.

From my test, this could be simplified to only check for OpenEXR and provide a base version to the find_package call. OpenEXR itself, depends on either Imath (>=OpenEXR-3) or IlmBase (<OpenEXR-3) and checks for the availability of the respective package. So I think, it's enough to only look for OpenEXR and leave the details regarding Imath / IlmBase to that package. I've done some patching in my PR.
Let me know, what you're thinking about it. I can do some rework of this, if needed and am happy for feedback on this.

waebbl added a commit to waebbl/CTL that referenced this issue Jan 12, 2023
Use GNUInstallDirs variable throughout the build system.

Bug: ampas#100
Signed-off-by: Bernd Waibel <[email protected]>
waebbl added a commit to waebbl/CTL that referenced this issue Jan 12, 2023
Use GNUInstallDirs variable throughout the build system.

Bug: ampas#100
Signed-off-by: Bernd Waibel <[email protected]>
michaeldsmith added a commit that referenced this issue Jan 12, 2023
* fix some installation paths

Use GNUInstallDirs variable throughout the build system.

Bug: #100
Signed-off-by: Bernd Waibel <[email protected]>

* streamline finding OpenEXR and Imath / IlmBase

The search for OpenEXR and IlmBase resp. Imath dependencies can be
simplified, as OpenEXR depends on either Imath or IlmBase, depending
if we use <OpenEXR-3 or >=OpenEXR-3.

Signed-off-by: Bernd Waibel <[email protected]>

* enable testing only when CMAKE_BUILD_TESTS=ON

The function enable_testing was globally enabled, no matter the value
of the CMAKE_BUILD_TESTS option. This patch guards this call, so it
only gets called, when CMAKE_BUILD_TESTS=ON.

Signed-off-by: Bernd Waibel <[email protected]>

* require C++ 11 using set(CMAKE_CXX_STANDARD 11)

* add #include <limits> for std::numeric_limits

* remove pkg-config *.pc.in and cmake *.cmake.in files

Signed-off-by: Bernd Waibel <[email protected]>
Co-authored-by: michaeldsmith <[email protected]>
@michaeldsmith
Copy link
Collaborator

@waebbl I think we have addressed this issue with our recent pull requests #105 and #115, if you agree, please go ahead and close your issue. If your issue is still not addressed, please describe what remains to be fixed.

@waebbl
Copy link
Contributor Author

waebbl commented Jan 19, 2023

The issue is fixed. I left it open for me as a reminder for the yet to do Dockerfile you've asked for.

@michaeldsmith
Copy link
Collaborator

OK sounds good - I added other Dockerfiles in the repo here https://github.com/ampas/CTL/tree/master/docker and the docker github CI workflow file is here https://github.com/ampas/CTL/blob/master/.github/workflows/docker_linuxes.yml

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

No branches or pull requests

3 participants