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

deps: Add build recipe for PNG #4423

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

zachlewis
Copy link
Contributor

As requested in #4387.

zachlewis and others added 5 commits September 13, 2024 13:11
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Required for building redistributable python wheels on Windows Wheels
runners.

Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
…dation#4429)

This test was running very slowly, taking too long in all circumstances,
but shockly causing GHA CI Mac runners to hit the test timeout
sometimes.

It turns out we were doing a lot of redundant work. There was no need to
make separate pattern and bayer images for every data type. Instead,
hoist the test image generation out of the loop and just make one
(float), then use `-i:type=<type>` to read it into an ImageBuf of the
appropriate data type.

This cuts the time for this test to run by about 3x.

Signed-off-by: Larry Gritz <lg@larrygritz.com>
MacOS builds require that PNG_FRAMEWORK=OFF in order to find and install dynamic libraries...
...but Windows still has trouble finding and installing dynamic libraries.

Only building static libraries seems to work, as far as building wheels is concerned; but I fear older dynamic libpngs found on the system will be preferred over newer static libpngs freshly-built by OIIO when linking.

Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
@lgritz
Copy link
Collaborator

lgritz commented Sep 18, 2024

Looks like we're still failing CI here

@zachlewis
Copy link
Contributor Author

indeed... it's the strangest thing. Still trying to get to the bottom of this.

I think what's happening is, for systems that have a "too old" version of dynamic PNG libraries installed, now that there's a build recipe available, even if we build newer static PNG libs, the build system is still preferring to link the older shared libraries (even though they're too old).

I dunno if this is a clue or a red herring, but it seems the build system might be getting confused about which version of PNG it's found versus the version it's trying to build (at least nominally):

 Could NOT find PNG: Found unsuitable version "1.5.13", but required is at least "1.6.0" (found /usr/lib64/libpng.so)
-- Building package PNG 1.5.13 locally
--         PNG_BUILD_VERSION = 1.6.44
--         PNG_BUILD_SHARED_LIBS = OFF
--         Building local PNG 1.6.44 from https://github.com/glennrp/libpng
--         Downloading local https://github.com/glennrp/libpng
Note: checking out 'f5e92d76973a7a53f517579bc95d61483bf108c0'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at f5e92d7 Release libpng version 1.6.44
-- Configuring done
-- Generating done
CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_COMPILE_WARNING_AS_ERROR
    CMAKE_MESSAGE_INDENT
    CMAKE_RULE_MESSAGES


-- Build files have been written to: /__w/OpenImageIO/OpenImageIO/build/deps/PNG-build
[1/31] Generating pngprefix.h
[2/31] Generating scripts/pnglibconf.c
[3/31] Generating pnglibconf.c
[4/31] Generating scripts/symbols.out
[5/31] Generating scripts/symbols.chk
[6/31] Generating pnglibconf.out
[7/31] Generating pnglibconf.h
[8/31] Generating scripts/vers.out
[9/31] Generating scripts/sym.out
[10/31] Generating scripts/prefix.out
[11/31] Generating libpng.sym
[12/31] Generating libpng.vers
[13/31] Generating scripts/intprefix.out
[14/31] Building C object CMakeFiles/png_static.dir/pngmem.c.o
[15/31] Building C object CMakeFiles/png_static.dir/pngrio.c.o
[16/31] Building C object CMakeFiles/png_static.dir/pngpread.c.o
[17/31] Building C object CMakeFiles/png_static.dir/pngerror.c.o
[18/31] Building C object CMakeFiles/png_static.dir/pngget.c.o
[19/31] Building C object CMakeFiles/png_static.dir/pngtrans.c.o
[20/31] Building C object CMakeFiles/png_static.dir/pngset.c.o
[21/31] Building C object CMakeFiles/png_static.dir/png.c.o
[22/31] Building C object CMakeFiles/png_static.dir/pngwio.c.o
[23/31] Building C object CMakeFiles/png_static.dir/pngread.c.o
[24/31] Building C object CMakeFiles/png_static.dir/intel/intel_init.c.o
[25/31] Building C object CMakeFiles/png_static.dir/pngwtran.c.o
[26/31] Building C object CMakeFiles/png_static.dir/pngwrite.c.o
[27/31] Building C object CMakeFiles/png_static.dir/pngrutil.c.o
[28/31] Building C object CMakeFiles/png_static.dir/pngrtran.c.o
[29/31] Building C object CMakeFiles/png_static.dir/intel/filter_sse2_intrinsics.c.o
[30/31] Building C object CMakeFiles/png_static.dir/pngwutil.c.o
[31/31] Linking C static library libpng16.a
[0/1] Install the project...
-- Install configuration: "Release"
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/libpng16.a
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/libpng.a
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/include/png.h
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/include/pngconf.h
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/include/pnglibconf.h
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/include/libpng16/png.h
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/include/libpng16/pngconf.h
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/include/libpng16/pnglibconf.h
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/bin/libpng-config
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/bin/libpng16-config
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/share/man/man3/libpng.3
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/share/man/man3/libpngpf.3
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/share/man/man5/png.5
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/pkgconfig/libpng.pc
-- Up-to-date: /__w/OpenImageIO/OpenImageIO/build/deps/dist/bin/libpng-config
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/pkgconfig/libpng16.pc
-- Up-to-date: /__w/OpenImageIO/OpenImageIO/build/deps/dist/bin/libpng16-config
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/libpng/libpng16.cmake
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/libpng/libpng16-release.cmake
-- Up-to-date: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/libpng16.a
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/cmake/PNG/PNGTargets.cmake
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/cmake/PNG/PNGTargets-release.cmake
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/cmake/PNG/PNGConfig.cmake
-- Installing: /__w/OpenImageIO/OpenImageIO/build/deps/dist/lib/cmake/PNG/PNGConfigVersion.cmake
-- Refinding PNG with PNG_ROOT=/__w/OpenImageIO/OpenImageIO/build/deps/dist
-- Found PNG: /usr/lib64/libpng.so (found version "1.5.13") 
-- Found PNG 1.5.13 
--     PNG_INCLUDE_DIR = /usr/include;/usr/include
--     PNG_INCLUDE_DIRS = /usr/include;/usr/include
--     PNG_LIBRARIES = /usr/lib64/libpng.so;/usr/lib64/libz.so

so... in the second line in the above log snippet, it claims to be "Building package PNG 1.5.13 locally", even though it's building 1.6.44.

I'm not sure what, if anything, that has to do with the more serious problems:

  1. That it can't "refind" the freshly-built static PNG given PNG_ROOT, PNG_REFIND_VERSION, or even PNG_REFIND_ARGS CONFIG
  2. That it proceeds to link the system-level dynamic PNG 1.5.13 that it previously detected as too old

A lesser problem, but one worth figuring out -- currently, I'm forcing the recipe to only build static libraries, cuz Windows has trouble figuring out what to do with / how to install the dynamic PNG library it just built. (I was having similar trouble on macOS until I set -DPNG_FRAMEWORK=OFF; but I've not yet investigated what specifically is (or isn't) happening on Windows.

Ideally, we'd only link the static libraries, since that seems to behave properly across platforms, given the opportunity...
I believe the OCIO missing-dependency-build-system business allows one to forcibly prefer static libraries instead of dynamic libraries if both are found (on a per-library basis). I don't suppose there's a way to do that here?

@zachlewis
Copy link
Contributor Author

I'm kind of baffled by the CI build failures... I can't seem to pinpoint anything in the logs or the configs that would explain why or how the failures occur on some runs, but not others...

The problematic runs are:

  • CI / VFX2021 oldest gcc9.3/C++17 py3.7 exr-3.1
  • CI / VFX2021 gcc9/C++17 py3.7 exr3.1 ocio2.0
  • CI / VFX2022 gcc9/C++17 py39 exr3.1 ocio2.1
  • CI / VFX2022 clang13/C++17 py39 avx2 exr3.1 ocio2.1

And they all fail the same way:

FAILED: bin/simd_test 
: && ccache /usr/local/bin/_ccache/clang++ -O3 -DNDEBUG  src/libutil/CMakeFiles/simd_test.dir/Unity/unity_0_cxx.cxx.o -o bin/simd_test  -Wl,-rpath,/__w/OpenImageIO/OpenImageIO/build/lib:/usr/local/lib64:/__w/OpenImageIO/OpenImageIO/build/deps/dist/lib  lib/libOpenImageIO.so.2.6.6  lib/libOpenImageIO_Util.so.2.6.6  -lpthread  /usr/local/lib64/libImath-3_1.so.29.4.0  -lm  -Wl,-rpath-link,/usr/local/lib64:/__w/OpenImageIO/OpenImageIO/build/deps/dist/lib && :
/opt/rh/devtoolset-9/root/usr/lib/gcc/x86_64-redhat-linux/9/../../../../bin/ld: lib/libOpenImageIO.so.2.6.6: undefined reference to `png_set_eXIf_1'
/opt/rh/devtoolset-9/root/usr/lib/gcc/x86_64-redhat-linux/9/../../../../bin/ld: lib/libOpenImageIO.so.2.6.6: undefined reference to `png_get_eXIf_1'
/opt/rh/devtoolset-9/root/usr/lib/gcc/x86_64-redhat-linux/9/../../../../bin/ld: lib/libOpenImageIO.so.2.6.6: undefined reference to `png_set_option'

But... considering the rest of the checks pass, it doesn't seem to be an issue with the compiler or compiler version, the VFX202X build image, the version of Python, the version of EXR, the version of OCIO, or whether sse instructions are used. And I can't seem to ascertain a particular constellation of settings that produces a failure.

The only thing I could identify that looked suspicious to me is the ordering of paths in the -Wl,-rpath-link,/usr/local/lib64:/__w/OpenImageIO/OpenImageIO/build/deps/dist/lib part of the build command for bin/simd_test. Earlier in the command, the path order is the reverse, with OIIO's build/deps/dist/lib path preceding /usr/local/lib64 (-Wl,-rpath,/__w/OpenImageIO/OpenImageIO/build/lib:/usr/local/lib64).

The logs aren't verbose enough for successful builds for me to tell if this is the difference that explains the failures of unsuccessful runs.

Attempt to force the build system to always find the locally-built PNG before any system-installed PNGs

Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
*Now* it should use PNG's CMake Config....

Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
To keep the build system from getting confused and trying to link found-but-too-old libraries if it can't find the recipe's libpng. So, if our libpng can't be found for whatever reason, this should prevent the build system from silently failing over to linking the found-but-too-old libraries.

Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
once more, with feeling.

Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
@zachlewis
Copy link
Contributor Author

Okay, this doesn't seem to be working properly -- CMake is having trouble finding the correct PNG library, and my understanding is, the issue stems from PNG's own build system idiosyncracies. The failures above occur on systems with libpng v1.5 installed, which are initially detected as too-old; but are later "refound" after find_package fails to find the libraries under $PNG_ROOT. This seems to be a problem with either kitware's FindPNG.cmake, PNG's CMake config, or both.

This said, it looks like the libpng team has been very actively working on improving and modernizing their CMake-based build system recently, seemingly in anticipation of an imminent libpng 1.8 release.

Importantly, two weeks ago, libpng v1.6.44 made some improvements and changes to the CMake build system (see pnggroup/libpng#545 among others) -- which is great -- but the difference between 1.6.43 and 1.6.44 seems to have serious implications for how find_package behaves (or fails to behave) and where targets can be found -- namely, we need find_package to consider the CONFIG introduced in 1.6.44, and likewise set PNG_DIR.

However, sometimes, the find_package still fails to (re)find the correct libpng library. Locally, I can produce conditions with the VFX2021 image where find_package is able to find the proper self-built libraries... but only with incremental builds that reuse a build directory, and only after a first build attempt that manages to self-build PNG, but still proceeds to link the too-old system dynamic PNG, as described above.

We do have other means for finding PNG -- for one, PkgConfig / png_check_module seems to find our PNG without difficulty -- but I understand this might be problematic on Windows. Alternatively, we can use the output of the libpng-config cli tool for setting PNG_LIBRARIES and PNG_INCLUDE_DIRS (and PNG_FOUND)... but setting those values doesn't seem to sufficiently inform the rest of the build process. And I don't really know enough about CMake or building and linking stuff in general to know what exactly to do next.

I feel the best solution involves coercing CMake's own Find mechanism to do the right thing. I'm not sure if that means creating a custom FindPNG.cmake module or patching PNG's CMake config, or what; but I suspect the path of least resistance is contingent on the build system improvements the libpng team is currently working on.

This PR was initially motivated by Wheels workflow tasks failing on vanilla macos-14 runners; and while this build_PNG.cmake recipe does seem to help with building the macos-arm64 wheels, it obviously introduces problems under other conditions. So, for the time being, we're bypassing the PNG plugin entirely for the Wheels / Build MacOS ARM64 tasks until we can figure out a more robust solution for PNG.

@lgritz
Copy link
Collaborator

lgritz commented Oct 3, 2024

@zachlewis Any update on this one?

@zachlewis
Copy link
Contributor Author

I'm afraid not. I'll have another look, but I think I might be stuck...

The failures occur when the build system detects a too-old version of libpng:

  1. Build PNG locally with build_PNG.cmake
  2. Fail to find "our" locally-built libpng
  3. Build the PNG plugin against the "too old" system libpng

I'm trying to find a way to force the build system to always find and link "our" PNG; but find_package has trouble finding PNG to begin with.

Also, libpng 1.6.43 and 1.6.44 behave very differently.

Signed-off-by: Zach Lewis <zachcanbereached@gmail.com>
@zachlewis zachlewis marked this pull request as draft October 3, 2024 21:40
@zachlewis zachlewis marked this pull request as draft October 3, 2024 21:40
Signed-off-by: Zach Lewis <zachcanbereached@gmail.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