-
Notifications
You must be signed in to change notification settings - Fork 652
ci: better spread of libpng versions we test against #4883
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
Conversation
lgritz
commented
Sep 13, 2025
- The "oldest" didn't really test against the oldest we claim to support.
- The "latest versions" didn't really test against the latest versions.
- The "bleeding edge" didn't really test against libpng master.
- The auto-build was unnecessarily a few releases behind.
- Documentation touch-ups
jessey-git
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here all look fine to me. Completely confused at to why theres that one test failure with VFX 2023 in an unrelated test.
|
Oh, I think I've done something bad here. Looks like I'm getting confusion between the libpng I'm building and the one already installed. Let me iterate on this a bit. |
|
vfx2023 was some random glitch, worked fine when I just told it to rerun. |
* The "oldest" didn't really test against the oldest we claim to support. * The "latest versions" didn't really test against the latest versions. * The "bleeding edge" didn't really test against libpng master. * The auto-build was unnecessarily a few releases behind. * Documentatin touch-ups Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
|
Updated and fixed |
jessey-git
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved per CI passing. Though I don't quite understand the failure from before and how using that particular OpenImageIO_BUILD_LOCAL_DEPS=PNG directive for the build fixes it?
|
There's an older version of libpng already on the system image, but as you pointed out, that was too old to support CICP, and so was not exercising this new code. In my first stab at this PR, I just ran the build_libpng.bash script that we use for CI to build and install, thus ending up with a second version of the library. Because of inclulde paths and LD_LIBRARY_PATH ordering (which can be really hard to get right), it ended up using the headers of one but the library of the other and finding missing symbols. That's what the failures were. But setting OpenImageIO_BUILD_LOCAL_DEPS=PNG gets us a new version in a totally different way: it runs our build system's "autobuild" approach for that package, which builds a static version and doesn't get confused about the system version. |
|
Gotcha, it was the detail about how this build option provides a static build that I was missing. |
98f43a6
into
AcademySoftwareFoundation:main
…Foundation#4883) * The "oldest" didn't really test against the oldest we claim to support. * The "latest versions" didn't really test against the latest versions. * The "bleeding edge" didn't really test against libpng master. * The auto-build was unnecessarily a few releases behind. * Documentation touch-ups --------- Signed-off-by: Larry Gritz <[email protected]> Signed-off-by: Zach Lewis <[email protected]>
…Foundation#4883) * The "oldest" didn't really test against the oldest we claim to support. * The "latest versions" didn't really test against the latest versions. * The "bleeding edge" didn't really test against libpng master. * The auto-build was unnecessarily a few releases behind. * Documentation touch-ups --------- Signed-off-by: Larry Gritz <[email protected]>