-
Notifications
You must be signed in to change notification settings - Fork 302
Feature: improve logging related to finding files #285
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
base: master
Are you sure you want to change the base?
Feature: improve logging related to finding files #285
Conversation
Speierers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alhirzel, thanks for this PR.
Reading the PR description it seems that the output is missing.
|
|
||
| fs::path FileResolver::resolve(const fs::path &path) const { | ||
| if (!path.is_absolute()) { | ||
| Log(Debug, "Looking for file \"%s\" on the resource search path(s)", path.native()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be logged into Trace as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently Debug is written once per file lookup, and Trace is written once per path check. You're right that I forgot to paste the output--maybe it makes the behavior more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is a little too much for debug mode. I would like to be able to compile in debug mode while not always having those logs. Let's use Trace here to have control. In any case, the log messages you are adding in this PR are useful to debug a situation where a file couldn't be found, in which case you will need to switch to Trace to see all the messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, agree, I will make all of the outputs part of Trace.
f3400a8 to
75c0056
Compare
|
I updated this to include a few more improvements that are in the following spirit (which I also added to the dev docs):
|
|
|
||
| if (fs::exists(resolved)) { | ||
| Log(Debug, "Loading plugin \"%s\" ..", filename.string()); | ||
| Log(Debug, "Loading plugin \"%s\" from \"%s\" ..", name, resolved.string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - I plan to change it to use the full resolved path then. See what you think.
| Log(Error, "\"%s\": file does not exist!", file_path.string()); | ||
|
|
||
| Log(Info, "Loading spectral data file \"%s\" ..", file_path); | ||
| Log(Info, "Loading spectral data file \"%s\" ..", file_path.string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this fix a specific bug? What is the log message before and after this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually made these types of changes during a very pedantic file path review for consistency in explicitly calling .string(); I don't know that it fixes a bug. I will back out this for less noise in the PR!
| if (bitmap->width() < 2 || bitmap->height() < 3) | ||
| Throw("\"%s\": the environment map resolution must be at " | ||
| "least 2x3 pixels", m_filename); | ||
| "least 2x3 pixels", file_path.string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here.
4af72bd to
199b607
Compare
df4cbe0 to
64fedcd
Compare
3f3b8d0 to
1bdea6e
Compare
ff60350 to
4504654
Compare
Description
Improves debug and trace logging when trying to locate files using the file resolver.
Showing the result of this change, note the basic output is unchanged:
With
Debugeach lookup is annotated:And finally for
Tracedetailed info about where is checked:Testing
I did not run the test suite because I am naively assuming this is a non-breaking change.
Checklist
cuda_*andllvm_*variants. If you can't test this, please leave below