PowerShell/PowerShellStandard

Missing ReferenceAssemblyAttribute decoration

SeeminglyScience opened this issue · 11 comments

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.

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.

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.

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.

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

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).

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.

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.

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.

@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.

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

I found only dotnet/runtime#31200. It is for ReflectionOnlyLoadFrom(). I wonder if MetadataLoadContext is not supported on Mac.

@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).