PowerShell / PowerShellStandard

MIT License
158 stars 23 forks source link

Missing ReferenceAssemblyAttribute decoration #70

Closed SeeminglyScience closed 4 years ago

SeeminglyScience commented 4 years ago

The attribute ReferenceAssemblyAttribute informs analysis tools that the assembly is not a runtime assembly.

Also consider decorating with AssemblyFlags((AssemblyNameFlags)0x70) to prevent the runtime from actually loading the assembly. Example in AspNetCore.

JamesWTruher commented 4 years ago

My tests actually load the reference assembly and use reflection to inspect it, so I need that part of it. And at least MacOS doesn't support ReferenceOnly loading. I'm afraid I have to leave this as it is. The nuget package should be keeping the SMA.dll out of publish locations.

iSazonov commented 4 years ago

My tests actually load the reference assembly and use reflection to inspect it, so I need that part of it.

The suggested code could be under #if-#else-#end condition.

SeeminglyScience commented 4 years ago

Yeah @JamesWTruher you should be able to add the ReferenceAssemblyAttribute decoration without changing anything, afaik it's just the ProcessorArchitecture=None that disables loading the assembly.

I do think it's important to include the assembly flag though. One of the most common problems I see folks run into is trying to or accidently using this assembly in xunit tests. I've seen folks troubleshoot mysterious NRE's for actual days assuming it's a problem with their tests. Unless you're already familiar with how reference assemblies are built, it's incredibly difficult to determine what's going on. An assembly load time exception would make tracking down the issue way easier.

I think @iSazonov's idea is best value so to speak, in that it's super easy to get up and running. You can just set up a build time constant that you pass to dotnet when building for tests. If you wanted to go the extra mile you could set up the tests to use MetadataReader. That API is a lot more complicated and scarcely documented, but if you go that route you can use this as reference. It implements all the System.Reflection API's using SRM so it can be used as a reference on how to translate existing reflection code.

JamesWTruher commented 4 years ago

i experimented with this, when the attribution was present, the module could not be loaded and my tests cannot run. On Mac, a message in produced which says that reflection only loading is not supported on the platform. If you have a different experience on non-Windows, I would love to know how. @iSazonov, @SeeminglyScience

SeeminglyScience commented 4 years ago

Oh hey look at that it does, I wonder what the point of the ProcessorArchitecture=None decoration is then...

Anyway as noted above, you should be able to get your tests working pretty easily by defining a build time constant when building for tests and using preprocessor directives to exclude the attribute(s).

JamesWTruher commented 4 years ago

I guess I'm not clear. I'm using the shipping assembly and testing it in my Pester tests. The pester tests load the assembly and then runs the tests against it. I really don't want to build one assembly for test and then another for ship.

SeeminglyScience commented 4 years ago

I hear ya, but I've helped quite a few folks troubleshoot things like "why is Parser.ParseInput returning null" just from occasionally looking at the PS slack. Not one of them considered that it could be the library they were loading, they were all trying to troubleshoot their tests. Even @TylerLeonhardt ran into that iirc.

This would at least give those folks something concrete to google.

iSazonov commented 4 years ago

I found .Net Runtime uses the attribute too https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Extensions/ReferenceAssemblyInfo.cs

Perhaps there are tests for reference assemblies too as example.

TylerLeonhardt commented 4 years ago

@SeeminglyScience yeah we hit that trying to use PSStandard inside of xUnit tests... Obviously that wouldn't work until we loaded in the Microsoft.PowerShell.SDK instead of PSStandard.

iSazonov commented 4 years ago

On Mac, a message in produced which says that reflection only loading is not supported on the platform.

I found only https://github.com/dotnet/runtime/issues/31200. It is for ReflectionOnlyLoadFrom(). I wonder if MetadataLoadContext is not supported on Mac.

SeeminglyScience commented 4 years ago

@SeeminglyScience yeah we hit that trying to use PSStandard inside of xUnit tests... Obviously that wouldn't work until we loaded in the Microsoft.PowerShell.SDK instead of PSStandard.

Yeah that's it. And just to be clear, that's expected and won't change with this (I know you know this @TylerLeonhardt, just clarifying for everyone else). The big difference is that with this change, if that same mistake is made you'll get an error message about loading a reference library. The only problem here is the cryptic null reference exceptions that folks get that leads to days of troubleshooting tests because no one understands how reference assemblies work (nor should they need to).