-
Notifications
You must be signed in to change notification settings - Fork 242
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
Feature: improve logging related to finding files #285
base: master
Are you sure you want to change the base?
Feature: improve logging related to finding files #285
Conversation
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.
@@ -21,10 +22,15 @@ bool FileResolver::contains(const fs::path &p) const { | |||
|
|||
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):
|
@@ -113,7 +113,7 @@ struct PluginManager::PluginManagerPrivate { | |||
fs::path resolved = resolver->resolve(filename); | |||
|
|||
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!
@@ -108,7 +108,7 @@ class EnvironmentMapEmitter final : public Emitter<Float, Spectrum> { | |||
ref<Bitmap> bitmap = new Bitmap(file_path); | |||
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
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
Debug
each lookup is annotated:And finally for
Trace
detailed 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