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

[test] Add switch to imageinout_test for enabling floating point exceptions. #4463

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

stolk
Copy link
Contributor

@stolk stolk commented Sep 30, 2024

Adds new feature: floating point exceptions, to imageinout_test binary.

Description

Running imageinout_test --enable-fpe will run the test with FPEs enabled.

Currently it is catching a FP overflow in libjxl.
Once that is fixed, it may find other FPEs.

For now, this is a Linux-only feature.

Tests

$ ./imageinout_test --enable-fpe

Checklist:

  • [ x] I have read the contribution guidelines.
  • [ x] I have updated the documentation, if applicable. (Check if there is no
    need to update the documentation, for example if this is a bug fix that
    doesn't change the API.)
  • [ x] I have ensured that the change is tested somewhere in the testsuite
    (adding new test cases if necessary).
  • [ x] If I added or modified a C++ API call, I have also amended the
    corresponding Python bindings (and if altering ImageBufAlgo functions, also
    exposed the new functionality as oiiotool options).
  • [ x] My code follows the prevailing code style of this project. If I haven't
    already run clang-format before submitting, I definitely will look at the CI
    test that runs clang-format and fix anything that it highlights as being
    nonconforming.

…ptions.

Running imageinout_test --enable-fpe will run the test with FPEs
enabled.

Currently it is catching a FP overflow in libjxl.
Once that is fixed, it may find other FPEs.

For now, this is a Linux-only feature.

Signed-off-by: Bram Stolk <[email protected]>
@stolk
Copy link
Contributor Author

stolk commented Sep 30, 2024

Updated for clang formatting.

NOTE: if I run $ make clang-format I get a lot of modified source files. More than the files I have changed. Is the formatting in the repo out of date? Or is my clang-formatter too old or too new?

@stolk
Copy link
Contributor Author

stolk commented Sep 30, 2024

This found an FP overflow in libjxl and a FP domain error in OpenColorIO.

PR for OpenColorIO is here: AcademySoftwareFoundation/OpenColorIO#2067

But the CLA check failed on OpenColorIO. Not sure why, because it succeeded for PRs on OpenImageIO.

I have no CLA for libjxl, but the patch is here: libjxl/libjxl@main...stolk:libjxl:fix_fpe

With jxl and ocio fixed, $ imageinout_test --enable-fpe passes.

@stolk
Copy link
Contributor Author

stolk commented Oct 2, 2024

I think the CI fail is unrelated to my change?

CMake Error at src/doc/cmake_install.cmake:46 (file):
  file INSTALL cannot find
  "/__w/OpenImageIO/OpenImageIO/src/doc/CHANGES-0.x.md": No such file or
  directory.
Call Stack (most recent call first):
  cmake_install.cmake:283 (include)

@lgritz
Copy link
Collaborator

lgritz commented Oct 2, 2024

Yes, I already fixed. Just hit the "rerun failed tests" button on the GHA page for that run.

@lgritz
Copy link
Collaborator

lgritz commented Oct 2, 2024

Sorry, I take it back. You need to rebase. I'll do that for you.

@lgritz
Copy link
Collaborator

lgritz commented Oct 3, 2024

NOTE: if I run $ make clang-format I get a lot of modified source files. More than the files I have changed. Is the formatting in the repo out of date? Or is my clang-formatter too old or too new?

Unfortunately, clang-format seems to have little changes in every major release, which is like every 6-9 months it seems. So we have to pick one, and then we lock down for a while so that we don't end up with a big pointless reformat more frequently than every couple years. Currently, our CI is using clang-format from llvm 17.

I tend to deal with any mismatch by (a) running clang-format from my editor, reformatting only the file, and sometimes only the functions, that I know I'm touching; (b) if I must clang-format the whole codebase, I'm careful not to git add anything that had spurious reformatting but that isn't intended to be part of the PR; (c) if CI still gets slightly different formatting than on my machine (usually it matches, sometimes there are small differences), I will do minor touch-ups by hand to make it pass CI.

@lgritz lgritz merged commit 3f4285b into AcademySoftwareFoundation:main Oct 3, 2024
26 checks passed
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