UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods in StackTrace #2643

snakefoot · 2018-04-04T14:55:29Z

Throws PlatformNotSupportedException when Net Native. Resolves #2632

https://github.com/dotnet/corert/blob/master/src/System.Private.Reflection.Core/src/System/Reflection/Runtime/General/ThunkedApis.cs#L63

And also fixed an issue with GetCurrentClassLogger that returned wrong classname. And also fixed an issue with callsite that threw NullReferenceExceptions. Happy days with Net Native.

snakefoot · 2018-04-04T15:22:08Z

@304NotModified Any reason why NLog uses Assembly.CodeBase instead of Assembly.Location:

https://blogs.msdn.microsoft.com/suzcook/2003/06/26/assembly-codebase-vs-assembly-location/

Looks like that the Assembly.Location can be a "random" place when running unit-tests:

https://stackoverflow.com/a/283917/193178

codecov · 2018-04-04T15:33:42Z

Codecov Report

Merging #2643 into master will decrease coverage by <1%.
The diff coverage is 69%.

@@           Coverage Diff           @@
##           master   #2643    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         326     326            
  Lines       24045   24070    +25     
  Branches     3040    3047     +7     
=======================================
+ Hits        19469   19483    +14     
- Misses       3752    3762    +10     
- Partials      824     825     +1

304NotModified · 2018-04-04T17:06:59Z

I will check, there was a good reason.

304NotModified · 2018-04-04T17:08:58Z

Related: #794

snakefoot · 2018-04-04T17:09:21Z

I will check, there was a good reason.

You don't have to. Think the GAC and unit-test-frameworks are enough reasons not to keep Assembly.CodeBase. Though one could consider having fallback to Assembly.Location (when Assembly.CodeBase doesn't work), but that is a different PR.

304NotModified · 2018-04-04T17:12:12Z

There was a fall back, but removed. See #29

304NotModified · 2018-04-04T17:14:00Z

Any way, we need to check the unc path. Afaik there is no unit test for it.

…e Lookup

snakefoot · 2018-04-04T21:22:02Z

@304NotModified Discovered that StackTraces now are more optimized on UWP NetStandard2 with Net Native. Have added extra protection regarding callsite-lookup and current-class-logger-lookup.

Could be nice if this callsite/class-lookup logic could extracted to a separate NLog-package, and remove the StackTrace-dependency completely (Ex. for NLog ver. 6).

304NotModified · 2018-04-04T22:03:25Z

Could be nice if this callsite/class-lookup logic could extracted to a separate NLog-package, and remove the StackTrace-dependency completely (Ex. for NLog ver. 6).

Don't we need it for every platform?

snakefoot · 2018-04-04T22:06:22Z

Don't we need it for every platform?

Many people can live without having GetCurrentClassLogger() and Callsite (Just like they can live without WCF targets), especially when it means less dependencies and less overhead and less fuzzy logic. Do not recall seeing this enricher logic in the core of Serilog.

For those needing them, then they will just be a nuget-package away.

304NotModified · 2018-04-04T22:20:42Z

Not sure about the currentClassLogger, https://mobile.twitter.com/NLogOfficial/status/758390118310875136

(Small poll, but better than no poll).

Of course having it optional is always nice, but a bit difficult as it's a static method (on logmanager).

snakefoot · 2018-04-04T22:25:49Z

(Small poll, but better than no poll).

Think those who voted, regret it now as it took 2 years to complete, and everyone was dead in the water :). The difference between nice-to-have, should-have and must-have is very vague for those standing on the side-line watching the others doing the digging.

optional is always nice, but a bit difficult as it's a static method (on logmanager).

Was thinking NLog 6.0 meant breaking old chains. A quick search and replace (and a recompile) should be something most programmers are capable of. Could probably have a default implementation of LogManager.GetCurrentClassLogger() that returns the calling assembly-name.

snakefoot · 2018-04-05T06:38:42Z

Another thing that should be moved out of the NLog-core is the automatic assembly loading. It has been a pain to maintain, and promiscuous assembly loading is not recommended by security specialists. Instead it should just be an extension to the internal configuration-loader (separate nuget-package).

304NotModified · 2018-04-05T14:09:51Z

Any way, we need to check the unc path. Afaik there is no unit test for it.

Could you check if a config from UNC still works, or should I do that?

snakefoot · 2018-04-05T14:11:05Z

Could you check if a config from UNC still works, or should I do that?

I have not changed anything in the assembly loading, besides handling the PlatformNotSupportedException. But please test away.

304NotModified · 2018-04-05T14:11:41Z

src/NLog/Config/ConfigurationItemFactory.cs

-                    var entryAssembly = Assembly.GetEntryAssembly();
-                    if (!string.IsNullOrEmpty(entryAssembly?.CodeBase))
+                    var entryLocation = GetAssemblyFileLocation(Assembly.GetEntryAssembly());
+                    if (!string.IsNullOrEmpty(entryLocation) && !string.Equals(entryLocation, assemblyLocation, StringComparison.OrdinalIgnoreCase))


good check for the string.Equals (saves us a GetNLogExtensionFiles) 👍

304NotModified · 2018-04-05T14:12:22Z

src/NLog/Config/ConfigurationItemFactory.cs

                Uri assemblyCodeBase;
                if (!Uri.TryCreate(assembly.CodeBase, UriKind.RelativeOrAbsolute, out assemblyCodeBase))
                {
-                    InternalLogger.Warn("No auto loading because assembly code base is unknown");
+                    InternalLogger.Warn("Skipping auto loading location because code base is unknown: `{0}` ({1})", assembly.CodeBase, fullName);


not very important, but why the backticks and not quotes?

304NotModified · 2018-04-05T14:15:41Z

src/NLog/LoggerImpl.cs

                {
                    if (stackFrames.Length > i)
                    {
                        var nextStackFrame = stackFrames[i + 1];
-                        var declaringType = nextStackFrame.GetMethod().DeclaringType;
+                        var declaringType = nextStackFrame.GetMethod()?.DeclaringType;


.GetMethod() cannot return null now, correct?

Not sure I understand. GetMethod() can return null, so free navigation can be dangerous. See the stackFrames[i + 1].

yes, missed that, thanks!

304NotModified · 2018-04-05T14:17:15Z

Also small performance improvement, isn't? Added it to the title :)

snakefoot · 2018-04-05T14:30:39Z

Also small performance improvement, isn't?

Don't think I have made any performance improvements on purpose. Mostly refactoring to improve the code-reuse for callsite-logic and GetCurrentClassLogger .

…e Lookup

304NotModified · 2018-04-06T21:14:32Z

I have not changed anything in the assembly loading, besides handling the PlatformNotSupportedException. But please test away.

tested it anyway, :) Thanks!

snakefoot · 2018-08-01T19:58:20Z

@snakefoot I have not changed anything in the assembly loading,

@304NotModified Guess I can withdraw that statement and wait for #2830 to be merged :(

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch 2 times, most recently from c15e476 to 2087eb9 Compare April 4, 2018 15:14

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch from 2087eb9 to 006ee51 Compare April 4, 2018 15:33

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch from 006ee51 to eb243af Compare April 4, 2018 15:40

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch from eb243af to 0362f6f Compare April 4, 2018 18:16

UWP with NetStandard2 on Net Native does not support Assembly.CodeBas…

73ac870

…e Lookup

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch from 0362f6f to 73ac870 Compare April 4, 2018 18:32

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch 3 times, most recently from 3b40380 to 6fa69dc Compare April 4, 2018 21:49

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch 7 times, most recently from 4a729c9 to 809d9b8 Compare April 5, 2018 05:13

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch 2 times, most recently from 2dd2976 to ba7749e Compare April 5, 2018 07:06

snakefoot mentioned this pull request Apr 5, 2018

Xamarin UWP doesn't create a logfile #2632

Closed

304NotModified added this to the 4.5.2 milestone Apr 5, 2018

304NotModified assigned 304NotModified and unassigned 304NotModified Apr 5, 2018

304NotModified added Xamarin and removed Xamarin labels Apr 5, 2018

304NotModified reviewed Apr 5, 2018

View reviewed changes

304NotModified changed the title ~~UWP with NetStandard2 on Net Native does not support Assembly.CodeBase~~ UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + performance improvement for callstack-based operations Apr 5, 2018

304NotModified added the performance label Apr 5, 2018

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch from ba7749e to 4e2fa55 Compare April 5, 2018 14:27

snakefoot changed the title ~~UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + performance improvement for callstack-based operations~~ UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods on callstack Apr 5, 2018

snakefoot changed the title ~~UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods on callstack~~ UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods in StackTrace Apr 5, 2018

304NotModified added refactoring and removed performance labels Apr 5, 2018

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch from 4e2fa55 to f8609aa Compare April 5, 2018 15:08

UWP with NetStandard2 on Net Native does not support Assembly.CodeBas…

58d93b2

…e Lookup

snakefoot force-pushed the UwpNetStandardAssemblyLookup branch from f8609aa to 58d93b2 Compare April 5, 2018 15:38

304NotModified merged commit ab141c4 into NLog:master Apr 6, 2018

snakefoot mentioned this pull request Apr 29, 2018

Removed obsolete static Assembly lookup caching #2682

Merged

This was referenced Aug 1, 2018

Errors thrown without NLog.Web in extensions #2768

Closed

Auto loading of assemblies was broken in some cases #2830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods in StackTrace #2643

UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods in StackTrace #2643

snakefoot commented Apr 4, 2018 •

edited

Loading

snakefoot commented Apr 4, 2018 •

edited

Loading

codecov bot commented Apr 4, 2018 •

edited

Loading

304NotModified commented Apr 4, 2018

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018

304NotModified commented Apr 4, 2018

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018 •

edited

Loading

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018 •

edited

Loading

snakefoot commented Apr 5, 2018 •

edited

Loading

304NotModified commented Apr 5, 2018

snakefoot commented Apr 5, 2018

304NotModified Apr 5, 2018

304NotModified Apr 5, 2018

snakefoot Apr 5, 2018

304NotModified Apr 5, 2018

snakefoot Apr 5, 2018

304NotModified Apr 5, 2018

304NotModified commented Apr 5, 2018

snakefoot commented Apr 5, 2018

304NotModified commented Apr 6, 2018

snakefoot commented Aug 1, 2018

UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods in StackTrace #2643

UWP with NetStandard2 on Net Native does not support Assembly.CodeBase + Handle native methods in StackTrace #2643

Conversation

snakefoot commented Apr 4, 2018 • edited Loading

snakefoot commented Apr 4, 2018 • edited Loading

codecov bot commented Apr 4, 2018 • edited Loading

Codecov Report

304NotModified commented Apr 4, 2018

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018

304NotModified commented Apr 4, 2018

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018 • edited Loading

304NotModified commented Apr 4, 2018

snakefoot commented Apr 4, 2018 • edited Loading

snakefoot commented Apr 5, 2018 • edited Loading

304NotModified commented Apr 5, 2018

snakefoot commented Apr 5, 2018

304NotModified Apr 5, 2018

Choose a reason for hiding this comment

304NotModified Apr 5, 2018

Choose a reason for hiding this comment

snakefoot Apr 5, 2018

Choose a reason for hiding this comment

304NotModified Apr 5, 2018

Choose a reason for hiding this comment

snakefoot Apr 5, 2018

Choose a reason for hiding this comment

304NotModified Apr 5, 2018

Choose a reason for hiding this comment

304NotModified commented Apr 5, 2018

snakefoot commented Apr 5, 2018

304NotModified commented Apr 6, 2018

snakefoot commented Aug 1, 2018

snakefoot commented Apr 4, 2018 •

edited

Loading

snakefoot commented Apr 4, 2018 •

edited

Loading

codecov bot commented Apr 4, 2018 •

edited

Loading

snakefoot commented Apr 4, 2018 •

edited

Loading

snakefoot commented Apr 4, 2018 •

edited

Loading

snakefoot commented Apr 5, 2018 •

edited

Loading