ci: require all dependencies, with explicit exceptions #4898
+118
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Anybody have comments about this? |
EmilDohne
approved these changes
Sep 25, 2025
We recently had a problem where our use of a dependency was broken, and it had gone unnoticed because none of our CI jobs actually had the dependency present and tested it. This can easily happen because so many of our dependencies are "optional" -- our build will just give a warning and silently disable the functionality that would have been supported by the missing dependency. We have long had build-time options `OpenImageIO_REQUIRED_DEPS`, to consider specific dependencies (or "all") required even if they would ordinarily be optional, and `OpenImageIO_OPTIONAL_DEPS`, to make exceptions. But we didn't use these in CI. So this PR makes at least some job sets, or individual jobs within those sets, assume all dependencies are required, and list any exceptions explicitly. This should make it much more difficult in the future to make a mistake where use of a dependency is completely untested in our CI. Some changes that came long for the ride: * checked_find_package: explicitly disabled packages are treated as optional. * cuda_macros.cmake: Only look for CUDA on platforms that might conceivably have it (i.e., don't even look on Mac). * Separate linux-aswf from linux-ubuntu into separate job groups, because that makes it easier to have shared commonalities of which dependencies they test. Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
force-pushed
the
lg-ensuredeps
branch
from
67499f9 to
20a8dce
Compare
lgritz
added a commit
to lgritz/OpenImageIO
that referenced
this pull request
Sep 27, 2025
…reFoundation#4898) We recently had a problem where our use of a dependency was broken, and it had gone unnoticed because none of our CI jobs actually had the dependency present and tested it. This can easily happen because so many of our dependencies are "optional" -- our build will just give a warning and silently disable the functionality that would have been supported by the missing dependency. We have long had build-time options `OpenImageIO_REQUIRED_DEPS`, to consider specific dependencies (or "all") required even if they would ordinarily be optional, and `OpenImageIO_OPTIONAL_DEPS`, to make exceptions. But we didn't use these in CI. So this PR makes sets `OpenImageIO_REQUIRED_DEPS=all` to make all dependencies ostensibly required, and then list all exceptions explicitly. This should make it much more difficult in the future to make a mistake where use of a dependency is completely untested in our CI without our being aware of it. And it gives us a visible "hit list" of untested or under-tested dependencies to slowly whittle down. Some changes that came long for the ride: * checked_find_package: explicitly disabled packages are treated as optional. * cuda_macros.cmake: Only look for CUDA on platforms that might conceivably have it (i.e., don't even look on Mac). * Separate linux-aswf from linux-ubuntu into separate job groups, because that makes it easier to have shared commonalities of which dependencies they test. Signed-off-by: Larry Gritz <lg@larrygritz.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We recently had a problem where our use of a dependency was broken, and it had gone unnoticed because none of our CI jobs actually had the dependency present and tested it. This can easily happen because so many of our dependencies are "optional" -- our build will just give a warning and silently disable the functionality that would have been supported by the missing dependency.
We have long had build-time options
OpenImageIO_REQUIRED_DEPS, to consider specific dependencies (or "all") required even if they would ordinarily be optional, andOpenImageIO_OPTIONAL_DEPS, to make exceptions. But we didn't use these in CI.So this PR makes sets
OpenImageIO_REQUIRED_DEPS=allto make all dependencies ostensibly required, and then list all exceptions explicitly. This should make it much more difficult in the future to make a mistake where use of a dependency is completely untested in our CI without our being aware of it. And it gives us a visible "hit list" of untested or under-tested dependencies to slowly whittle down.Some changes that came long for the ride:
checked_find_package: explicitly disabled packages are treated as optional.
cuda_macros.cmake: Only look for CUDA on platforms that might conceivably have it (i.e., don't even look on Mac).
Separate linux-aswf from linux-ubuntu into separate job groups, because that makes it easier to have shared commonalities of which dependencies they test.