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

Simplify OSL test setup #1868

Conversation

ld-kerley
Copy link
Contributor

Use the OSL cmake exports to configure the variables needed to enable OSL testing.

Also remove the need for specifying the standard OSL include path, an oslc install will already know where these are.

@ld-kerley
Copy link
Contributor Author

This is ready to go - pending someone who works on Windows validating that this still allows OSL to be used with windows.

@jstone-lucasfilm
Copy link
Member

This proposal looks very promising to me, and I should have a chance to test it soon with Windows OSL (which uses a less standardized build approach and folder structure).

@jstone-lucasfilm
Copy link
Member

@ld-kerley In my local test, I get an error message from CMake before I have a chance to select any build options, so this seems to occur even if the user doesn't choose to build with OSL support:

CMake Warning at CMakeLists.txt:185 (find_package):
  By not providing "FindOSL.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "OSL", but
  CMake did not find one.

  Could not find a package configuration file provided by "OSL" with any of
  the following names:

    OSLConfig.cmake
    osl-config.cmake

  Add the installation prefix of "OSL" to CMAKE_PREFIX_PATH or set "OSL_DIR"
  to a directory containing one of the above files.  If "OSL" provides a
  separate development package or SDK, be sure it has been installed.

@ld-kerley
Copy link
Contributor Author

@jstone-lucasfilm - was this an error message in CMake, or just a warning and things continued? I would expect things to have just continued, albeit with an annoyingly loud warning.

I just pushed a PR where I'm quietening the warning, and also guarding the check to only when we need it (testing OSL).

…the OSL search path, oslc already knows where the standard headers are located.
… found we'll just fallback to the user provided paths anyway.
@jstone-lucasfilm
Copy link
Member

@ld-kerley I believe you're correct that this was just a warning, though CMake makes it look as if the warning is fatal through the use of red text. Let me try the latest build and see how things are looking now.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @ld-kerley!

@jstone-lucasfilm jstone-lucasfilm merged commit dd075c0 into AcademySoftwareFoundation:main Aug 3, 2024
34 checks passed
jstone-lucasfilm pushed a commit that referenced this pull request Aug 23, 2024
A previous PR #1868 removed the ability to define the location for the OSL standard includes.  This is still required for Windows builds.
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