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.
There was a problem hiding this comment.
Agreed! Though should I switch this to just OCIO::Exception for now then? Ultimately it's a runtime error so it kind of fits!
| // 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
There was a problem hiding this comment.
No issue with that. I can reduce the scope of this specific change to just simply a better message if config file does not exists.
There was a problem hiding this comment.
Reverted changes done to PathUtil which can be put in a separate PR.
| 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.
There was a problem hiding this comment.
That is all good points. I cannot say for sure myself about that other particular FileExists overload. I see it being used for trying to find lut files with search_path context. I have no issue with it being a different name when we want to replace pystring::path usages!
Perhaps we can create our own internal pystring-like module that basically does some of those pystring tasks that we use then? That was sort of where I was heading by adding these path-related stuff to PathUtil.
There was a problem hiding this comment.
Reverted changes done to PathUtil which can be put in a separate PR.
Signed-off-by: Mei Chu <meimchu@gmail.com>
Signed-off-by: Mei Chu <meimchu@gmail.com>
|
For now, I've reverted the more ambitious changes and opted for a targeted solution. I looked a little deeper into the existing PathUtils::FileExists function which seems caching file hash within a context. My shallow understanding is that it helps speed up the various lut files found via search_path. I also tried creating an empty context to try to utilize that same FileExists() with the config file path but it doesn't feel right to me. The primary impact of this though is that ExceptionMissingFile() is raised if input file path does not exist. Relying on ifstream.fail() for other reasons why reading that file could fail. |
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!