ociocheck to check OpenColorIO library version first#2310
Conversation
Signed-off-by: Mei Chu <meimchu@gmail.com>
Signed-off-by: Mei Chu <meimchu@gmail.com>
Signed-off-by: Mei Chu <meimchu@gmail.com>
| if (!filename || !*filename) | ||
| { | ||
| throw ExceptionMissingFile ("The config filepath is missing."); | ||
| throw ExceptionMissingFile("The config filepath is missing."); |
There was a problem hiding this comment.
Probably OK for now, but it is worth noting that as you highlighted in the issue this condition isn't quite the same as a missing file. ideally we'd have a different type for this.
| // working directory must be an absolute path. | ||
| const char * workingDirectory = getWorkingDir(); | ||
| if ((workingDirectory && !workingDirectory[0]) || !pystring::os::path::isabs(workingDirectory)) | ||
| if ((workingDirectory && !workingDirectory[0]) || !IsPathAbs(workingDirectory)) |
There was a problem hiding this comment.
My gut suggests we should deal with the switch away from the pystring implementation as a separate issue to cleaning up the error messages. Changing the implementation to use std::filesystem could have more subtle issues
| return (!hash.empty()); | ||
| } | ||
|
|
||
| bool FileExists(const std::string & filename) |
There was a problem hiding this comment.
As an example of the subtlety When comparing this function with the other overload
bool FileExists(const std::string & filename, const Context & context)
Shows a much more complicated implementation which immediately makes me wonder why. Should we be dealing with the IOProxy here or not, if not then should the function have a different name? or at least a clear documentation stating why etc.
At this point I have not reasoned this through as I don't have the understanding of the IOProxy workings without studying it.
2 participants
The idea behind this PR originated with me using
ociocheckand getting stuck with config read error (#2303). So I just wanted clearer error message when the error was due to the file path provided being non-existent. Upon looking at the existing issues, I noticed there is an issue about removing pystring dependency (#2256). So I took this opportunity to get started by addingPathUtils::IsPathAbs(),PathUtils::NormalizePath()... etc with standard libraryfilesystemand using them inConfig. Also a change toExceptionMissingFileraised if file does not exist. (Can revert back to just raise Exception if it's causing too much API change)I would appreciate if @KevinJW has any thought about this as a starting point and the right track. Thank you!