Skip to content

Conversation

@lgritz
Copy link
Collaborator

@lgritz lgritz commented Oct 1, 2025

A test needed updating because the set of color spaces in the built-in config are different than for 2.4.

Fixes #4915

@lgritz lgritz added the build / testing / port / CI Affecting the build system, tests, platform support, porting, or continuous integration. label Oct 1, 2025
@lgritz
Copy link
Collaborator Author

lgritz commented Oct 1, 2025

Because CI and tests are broken without this and downstream users have already noticed it, AND today is release day for us, it's emergency circumstances, so I'm going to preemptively merge this as soon as it passes CI.

@darix
Copy link

darix commented Oct 1, 2025

I dont see any code to not use the 2.4 file. I can see in a local testbuild it works for the 2.5 file but still tries to do something with the 2.4 file

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 1, 2025

When multiple reference outputs are checked into the ref directory, any one of them being a match will allow the test to pass. That's how we deal with slightly different results for different versions of a dependency.

@lgritz lgritz marked this pull request as ready for review October 1, 2025 18:38
@lgritz
Copy link
Collaborator Author

lgritz commented Oct 1, 2025

Golly I'm not sure how that got marked as a draft PR. It was the real ting.

Copy link
Contributor

@doug-walker doug-walker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to get the tests working again.

However, it will probably break again every time the built-in configs are updated (which tends to be at least annually). Perhaps in the future it would be better to have the test only validate what OIIO needs rather than test the number of color spaces in the config?

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 1, 2025

I think the test is fine. I'm not sure how to fully test without actually extracting some of the cs information from the built-in config and verifying that it's correct. I expected all along that it would need updating possibly every year. Our test suite is set up to easily accommodate that.

@darix
Copy link

darix commented Oct 1, 2025

@zachlewis
Copy link
Collaborator

I dont see any code to not use the 2.4 file. I can see in a local testbuild it works for the 2.5 file but still tries to do something with the 2.4 file

If you mean the bit at the end referencing "oiio_test_v0.9.2.ocio", that's fine -- we're using a test config here specifically to insulate certain tests from exactly these kinds of failures :)

@darix
Copy link

darix commented Oct 1, 2025

I dont see any code to not use the 2.4 file. I can see in a local testbuild it works for the 2.5 file but still tries to do something with the 2.4 file

If you mean the bit at the end referencing "oiio_test_v0.9.2.ocio", that's fine -- we're using a test config here specifically to insulate certain tests from exactly these kinds of failures :)

well I see some successful looking output for the 2.5 file but some error looking output from the 2.4 file.

you will see it on the url up there soon.

@zachlewis
Copy link
Collaborator

I think the test is fine. I'm not sure how to fully test without actually extracting some of the cs information from the built-in config and verifying that it's correct. I expected all along that it would need updating possibly every year. Our test suite is set up to easily accommodate that.

We've actually discussed this before (in Slack):

Zach Lewis
3:29 PM
Since the default config that ships with OCIO (ocio://default) is updated on minor version releases, i feel like we should be using a static OCIO config to test some parts of the ColorConfig API -- i.e., so the number and names of color spaces and displays and whathaveyou remains constant across versions

Larry Gritz
3:47 PM
Yes, I'm working on fixing this one myself. Sorry about that. In general, the "bleeding edge" test (which is a test build against the latest TOT master/main of several key dependencies) breaks frequently due to changes in those packages. This is all expected, and we never hold up a merge for a broken bleeding edge test, unless inspecting the log really does implicate the changes in our PR as being the culprit. So we had a bit of bad luck in that OCIO pushed this change as Dev Days was unfolding, so everybody had at least this test fail. Some things you just can't control for!

Zach Lewis
4:06 PM
Roger that -- I was actually just about to submit a PR to fix this.
(you can call oiio.ColorConfig('ocio://cg-config-v1.0.0_aces-v1.3_ocio-v2.1') which will retrieve the same OCIO config, regardless of OCIO version).
Would you still like me to submit?

Larry Gritz
4:27 PM
OH
4:31
holy cow is that a mouthful
4:33
I'm torn about the right strategy. That's a good trick to know, and it would probably be fine, but I think I do still want to instead add a new ref output for the new config. Something feels off about locking down to an old config and then possibly forgetting about it, we wouldn't know if and when behavior on the current config was changing or if we break anything that we'd only discover when using the latest config.

Zach Lewis
5:23 PM
Fair enough!

@darix
Copy link

darix commented Oct 1, 2025

my buildfailure. https://build.opensuse.org/package/live_build_log/home:darix:branches:graphics/OpenImageIO/openSUSE_Tumbleweed/x86_64 the patch has removed the INSTALL.md and github ci chunks.

@zachlewis
Copy link
Collaborator

I'm not quite sure what to tell you... our tests seem to be behaving exactly as intended...

@lgritz
Copy link
Collaborator Author

lgritz commented Oct 1, 2025

Seems to work on our end, so I'll merge and get releases out today. If there are still downstream failures after using a proper release, we'll investigate.

@lgritz lgritz merged commit 9529077 into AcademySoftwareFoundation:main Oct 1, 2025
33 checks passed
@lgritz lgritz deleted the lg-ocio25 branch October 1, 2025 19:52
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 1, 2025
A test needed updating because the set of color spaces in the built-in
config are different than for 2.4.

Fixes AcademySoftwareFoundation#4915

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 1, 2025
A test needed updating because the set of color spaces in the built-in
config are different than for 2.4.

Fixes AcademySoftwareFoundation#4915

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Oct 1, 2025

Aha, in retrospect, it is obvious: This patch against main will fail if directly backported to the 3.0 branch, since there are other color management changes in 3.1+ that result in slightly different output. Stay tuned for a real 3.0 release that will have the correct fixes.

lgritz added a commit that referenced this pull request Oct 1, 2025
A test needed updating because the set of color spaces in the built-in
config are different than for 2.4.

Fixes #4915

Signed-off-by: Larry Gritz <[email protected]>
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Oct 2, 2025
A test needed updating because the set of color spaces in the built-in
config are different than for 2.4.

Fixes AcademySoftwareFoundation#4915

Signed-off-by: Larry Gritz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build / testing / port / CI Affecting the build system, tests, platform support, porting, or continuous integration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] OCIO 2.5.0 breaks 104 - python-colorconfig (Failed)

4 participants