Skip to content
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

Check if directory NLog.dll is detected in actually exists #1432

Merged
merged 2 commits into from
May 4, 2016

Conversation

gregmac
Copy link
Contributor

@gregmac gregmac commented May 3, 2016

Fixes crash when an assembly location is found that doesn't actually exist.

I believe this happens when using in-memory or bundling loading of the library, as described at #736 (comment).

Fixes crash when an assembly location is found that doesn't actually exist. 

I believe this happens when using in-memory or bundling loading of the library, as described at NLog#736 (comment).
@304NotModified
Copy link
Member

Thanks for this PR. It's a bit strange that this can happen, but no problem to check it first.

The change is in a good direction, bit I think it wise to:

  • write another warning to the InternalLogger in this case
  • check related code, like auto loading the xmlconfig (inside xmlloggingconfiguration)

@gregmac
Copy link
Contributor Author

gregmac commented May 4, 2016

@304NotModified I updated with a separate log message. I was originally going to do that but didn't really have a good idea of what log message to use, hopefully 'not found' is appropriate?

I'm not sure what should be updated in XmlLoggingConfiguration (though I'm also not familiar with this codebase, other than as a user). All the file operations I see are a result of explicit configuration inside a config file -- so if a user specifies an include path that doesn't exist in a config file, I don't think this change should modify the current behaviour.

@codecov-io
Copy link

Current coverage is 75.43%

Merging #1432 into master will decrease coverage by -<.01%

@@             master      #1432   diff @@
==========================================
  Files           267        267          
  Lines         15993      15997     +4   
  Methods           0          0          
  Messages          0          0          
  Branches       1724       1725     +1   
==========================================
  Hits          12067      12067          
- Misses         3517       3520     +3   
- Partials        409        410     +1   

Powered by Codecov. Last updated by 04944f6...4faa313

@304NotModified 304NotModified added the enhancement Improvement on existing feature label May 4, 2016
@304NotModified 304NotModified added this to the 4.3.4 milestone May 4, 2016
@304NotModified
Copy link
Member

Thanks!

@304NotModified 304NotModified merged commit 87952f6 into NLog:master May 4, 2016
@304NotModified
Copy link
Member

I checked other places, and those are also OK.

This will be released next minor version. I have changed the log message so also the assemblyLocation will be printed.

@gregmac gregmac deleted the patch-1 branch May 4, 2016 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants