Unable to add native .dll reference to source generator project

[myblindy]

Version Used: VS 17.0.0 Preview 2.0

Steps to Reproduce:

You can find the full source code at https://github.com/myblindy/GrimBuilding/tree/efcore (the efcore branch).

I understand that source generators can't automatically harvest dependencies from nuget packages and you have to use a clunky work-around to get it to work, and I have done so. This is my source generator project:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <LangVersion>preview</LangVersion>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.0.0-1.final" PrivateAssets="all" />
    <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.2" PrivateAssets="all" />
    <PackageReference Include="SQLitePCLRaw.bundle_e_sqlite3" Version="2.*-*" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="Microsoft.Data.Sqlite.Core" Version="6.*-*" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="SQLitePCLRaw.lib.e_sqlite3" Version="2.*-*" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="SQLitePCLRaw.provider.e_sqlite3" Version="2.*-*" GeneratePathProperty="true" PrivateAssets="all" />
  </ItemGroup>

  <PropertyGroup>
    <GetTargetPathDependsOn>$(GetTargetPathDependsOn);GetDependencyTargetPaths</GetTargetPathDependsOn>
  </PropertyGroup>

  <Target Name="GetDependencyTargetPaths">
    <ItemGroup>
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSQLitePCLRaw_bundle_e_sqlite3)\lib\netstandard2.0\SQLitePCLRaw.batteries_v2.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSQLitePCLRaw_provider_e_sqlite3)\lib\netstandard2.0\SQLitePCLRaw.provider.e_sqlite3.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSQLitePCLRaw_lib_e_sqlite3)\runtimes\win-x64\native\e_sqlite3.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGMicrosoft_Data_Sqlite_Core)\lib\netstandard2.0\Microsoft.Data.Sqlite.dll" IncludeRuntimeDependency="false" />
    </ItemGroup>
  </Target>
</Project>

Since there isn't even transitive support, I added every nested Microsoft.Data.Sqlite package one by one, generated its property and referenced it using TargetPathWithTargetPlatformMoniker. It all works until I get to the native e_sqlite3.dll, if I use TargetPathWithTargetPlatformMoniker with it, it tries to reference it as a managed library, and it fails as expected:

4>CSC : warning CS8034: Unable to load Analyzer assembly C:\Users\meep\.nuget\packages\sqlitepclraw.lib.e_sqlite3\2.0.5-pre20210521085756\runtimes\win-x64\native\e_sqlite3.dll : PE image doesn't contain managed metadata.

So given that the path is found, is there a different tag I can use to make the main project copy the e_sqlite3.dll file so the analyzer can use it?

[Sergio0694]

Also hitting this one (had opened #63396 though it was a dupe of this). I'm trying to update my generator in ComputeSharp to remove IO (following the conversation in #63290), and I'm currently with the following structure in my NuGet:

My generator then will P/Invoke LoadLibraryW and pick the pair of native .dll-s for the correct architecture. Right now this is just causing the analyzer to fail to load completely, because Roslyn sees those native libs in the directory free of the analyzer.

I really want to stop doing IO from the generator, and I do need to bundle those .dll-s this way to do that 🥲

@chsienki if it helps, when you start working on this feel free to ping me on Discord or Teams if you wanted another test scenario to validate the changes. I'd love to help and to try out a preview build of the toolset to double check a fix for this does resolve all issues both when going through NuGet and locally when using a project reference (I'm using both) 🙂

[Sergio0694]

Sharing some more findings while testing Sergio0694/ComputeSharp#349. I believe there's two different issues related to supporting this scenario (ie. analyzers that depend on native libs bundled with the analyzer .dll), depending on how one is referencing the analyzer in question. Specifically:

1. When the analyzer is packed into a NuGet package, then Roslyn just fails to load entirely because it sees those native .dll-s, tries to load them as analyzers, and then crashes when it realizes they're not managed .dll-s. This is the issue mentioned here, where Roslyn will just crash with that "warning CS8034: Unable to load Analyzer assembly <FOO.dll> : PE image doesn't contain managed metadata." error message.
2. When the analyzer is referenced locally as a project reference, it seems Roslyn isn't copying content elements correctly. In my analyzer .csproj, I have a few <None> items (also tried with <Content>, same issue) marked as CopyToOutputDirectory (the native .dll-s). When building the solution locally Roslyn will copy the analyzer .dll into some temporary folder (eg. C:\Users\Sergio\AppData\Local\Temp\VBCSCompiler\AnalyzerAssemblyLoader\0ea7a7ff50e341df8c5ee64fc96ecadc\11), but it won't copy those native .dll-s as well, even though they're marked as content to be copied to output. As in, if I open that folder myself I can see the analyzer .dll just being there all alone (🥲). As a result, the analyzer fails to find them at runtime and will crash when trying to call LoadLibrary on them, or loading the .dll in some other way. I was kinda expecting Roslyn to also respect files that are marked as copy to output, when copying the analyzer .dll-s into its temporary folder for the ALC to use to load the analyzer.

To be clear, issue (1) is the most important one to solve, as that'd impact consumers of the library. Issue (2) can be worked around locally as it's mostly just encountered when working locally and setting up test projects and whatnot, though it'd be nice if that scenario also "just worked" out of the box, as one would normally expect items marked as CopyToOutputDirectory to always be available next to the executing assembly.

Hope this helps! 😄

cc. @RikkiGibson

[myblindy]

it won't copy those native .dll-s as well, even though they're marked as content to be copied to output

Let me blow your mind: embed the native dlls in your assembly yourself, and write them out to disk during DLL startup. If you only use run-time binding (LoadLibrary, DllImport in a separate class, etc.) the native library should already be present by the time it happens, and your scenario will work.

This is of course bordering on the ridiculous, but you're likely to get farther than waiting for the root cause of this to be fixed :(

[Sergio0694]

"Let me blow your mind: embed the native dlls in your assembly yourself, and write them out to disk during DLL startup."

My mind was not blown, as that's exactly what I've been doing for months already. But the whole point is that scenario is completely unsupported: source generators shouldn't do any IO. Not to mention it makes the setup unnecessarily complicated and ends up creating a lot of garbage in temporary folders for all the copied .dll-s.

[myblindy]

Jokes aside, my biggest problem with this issue (my issue, and the first on your list) is that if the build system did literally nothing, it would all just work.

The problem only occurs because it's trying to load every dll as a managed assembly and it crashes on native ones, but if it just ignored load problems with native assemblies, everything else would work as is: the native dlls would still be copied alongside the managed ones and be ready for function binding.

[CyrusNajmabadi]

The problem only occurs because it's trying to load every dll as a managed assembly

What does "it's" refer to here? Roslyn, or something else?

[Sergio0694]

Roslyn, yes. If it just ignored native PEs, then all would be fine 🙂

[CyrusNajmabadi]

Do you know where roslyn is loading the native dlls? can you hook the point where that happens somehow and show the stack that is doing htat?

[tmat]

@CyrusNajmabadi Msbuild targets pass all dlls in the analyzer package to the compiler as /analyzer command line params. AnalyzerAssemblyLoader then loads all these dlls. The loader could definitely check whether the dll is managed and ignore native.

[Sergio0694]

Yeah, I'd assume the issue is originating here:

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerFileReference.cs

Lines 451 to 469 in dd04453

	try
	{
	analyzerTypeNameMap = GetExtensionTypeNameMap();
	if (analyzerTypeNameMap.Count == 0)
	{
	return;
	}
	analyzerAssembly = _reference.GetAssembly();
	if (CheckAssemblyReferencesNewerCompiler(analyzerAssembly))
	{
	return;
	}
	}
	catch (Exception e)
	{
	_reference.AnalyzerLoadFailed?.Invoke(_reference, CreateAnalyzerFailedArgs(e));
	return;
	}

Where the loader just tried to get the Assembly from the input .dll, which will of course crash for a native PE. If Roslyn just ignored these .dll-s (effectively doing the same it does for a managed .dll that just contains no analyzer types, ie. doing nothing), then this would completely solve the issue here 🙂

[CyrusNajmabadi]

i'm confused. why is the native dll being added as an analyzer dll? it seems appropriate to me that if it's an analyzer dll and it's not, it should fail/crash :)

Missing tomas' explanation.

[CyrusNajmabadi]

@CyrusNajmabadi Msbuild targets pass all dlls in the analyzer package to the compiler as /analyzer command line params.

ickkkkkkkkkk. it feels like this is on msbuild. Having us try to infer that it is passing bad stuff to us and undo it seems like the wrong way to handle things. CAn we not come up with a way for them to do things differently here?

[Sergio0694]

Do you know who we should ping from the MSBuild team about this? With its behavior currently being this, and leading Roslyn to just fail to load analyzers entirely, we're a bit in a bad spot now as the only way to make it work at all currently is to manually embed and unpack libraries at runtime, which goes directly against #63290 and is explicitly unsupported 😅

[RikkiGibson]

We are supposed to receive all the analyzer dependencies in the command line. This is important for determinism (@jaredpar in case he has an FAQ handy for this :P). We could consider using another flag instead of /analyzer to give them to the compiler but it's not hugely important IMO. we just have to account, one way or another, for the possibility that an analyzer dependency is native.

[CyrusNajmabadi]

Doing random IO would be a problem, as it would represent machine state not encoded into your inputs. Using IO to effectively just load data you are shipping is likely less of a problem.

[tmat]

This is also a regression in functionality. This error today catches developers who are constructing incorrect NuPkg files. Or packaging up DLLs that are meant to be managed but are invalid due to a tool in their build. Consider as an example IL rewriters that can end up producing invalid assemblies. The proposal here is essentially removing an existing safety guard from the ecosystem by silently ignoring this type of failure.

Is reporting errors for assemblies that have incorrect CorFlags set more important for Roslyn analyzer ecosystem than simplifying loading native analyzer dependencies? Why do we care about bad IL rewriters producing bad analyzer assemblies? Seems like super edge case.

[jaredpar]

Seems like super edge case.

I agree it's an edge case. The claims being made are this is "it won't break anything". I'm pushing back on that notion. Every change has a consequence, ignoring them doesn't benefit the discussion.

[RikkiGibson]

It seems like we should be able to distinguish this doesn't have managed metadata from this has malformed managed metadata, and continue to give an error on the latter if we think that has value.

[Sergio0694]

"Using IO to effectively just load data you are shipping is likely less of a problem"

@CyrusNajmabadi Sure, and in fact that's what we're currently doing, but it's still not ideal because (1) it adds extra complexity, (2) once the new analyzers for banned APIs go online, we'll also get warnings on all those IO APIs we'll have to disable, (2) it adds overhead, and (3) it leaves a lot of garbage in temp folders for consumers 🥲

[Sergio0694]

Picking this up now that 17.4 is mostly done, have we reached a conclusion on what the correct approach (if any) should be used to solve this issue? To recap, @RikkiGibson's proposal makes sense to me:

"It seems like we should be able to distinguish this doesn't have managed metadata from this has malformed managed metadata, and continue to give an error on the latter if we think that has value."

That would completely solve the issue while also preserving existing behavior in other cases, in case that was desired 🙂

[Sergio0694]

Just wanted to mention that now that EnforceExtendedAnalyzerRules is available, it would be extra nice to get proper support for native binaries bundled with the generator, as otherwise you also have to manually suppress the new warnings (RS1035) when using banned APIs which you're currently forced to use to manually extract the embedded .dll-s into a temporary directory and then load them. Eg. in my case I have to use Path.GetDirectoryName, Path.GetTempPath, Directory.CreateDirectory and File.Open, all of which are banned. Ideally if native .dll-s were allowed in a generator bundle (as in, either in the same folder or in some subfolder), you'd just need a single LoadLibraryW call to the right relative path (or the equivalent on other platforms) 🙂

[yugabe]

Um, any progress on this? There are real scenarios where native dependencies might be required for source generators, I believe the merit is proven.

[CyrusNajmabadi]

Um, any progress on this? There are real scenarios where native dependencies might be required for source generators, I believe the merit is proven.

I think the recommended approach is to package up your native dependencies as resources in your dll. You can then extract those bits and load them on demand.

[yugabe]

I'm not sure it's possible in my case, because the dependency's static constructor throws, so it's implicitly called. Any pointers or samples I could take a look at?

[Sergio0694]

I'm doing this in ComputeSharp, for reference.

@CyrusNajmabadi worth noting that while this works, it still produces a bunch of warnings from Roslyn, as you necessarily need to use some banned IO APIs. It'd be nice to be able to just bundle libs right in the analyzer folder, so we'd no longer need to suppress those warnings (and also, no more hassle of extracting the libs manually).

[yugabe]

Thanks @Sergio0694, I'll take a look.

  <ItemGroup>
    <EmbeddedResource Include="..\..\libs\x64\dxcompiler.dll" Link="ComputeSharp\Libraries\x64\dxcompiler.dll" />
    <EmbeddedResource Include="..\..\libs\x64\dxil.dll" Link="ComputeSharp\Libraries\x64\dxil.dll" />
    <EmbeddedResource Include="..\..\libs\arm64\dxcompiler.dll" Link="ComputeSharp\Libraries\arm64\dxcompiler.dll" />
    <EmbeddedResource Include="..\..\libs\arm64\dxil.dll" Link="ComputeSharp\Libraries\arm64\dxil.dll" />
  </ItemGroup>

and DxcLibraryLoader, I assume.

[jaredpar]

Um, any progress on this?

There has been some discussion on letting them deploy as we do for normal DLLs and then differentiating native / managed by checking for the right metadata headers. That would solve the deployment bit but there are still some questions around how we'd manage them in the actual application.

Keep in mind that there is not a single way that analyzers are loaded into the compiler: there are at least half a dozen depending on the host. Have to work through the details for all of those before taking on wrok.

I believe the merit is proven.

Just because an issue has merit doesn't mean we're guaranteed to do the work. Every issue we fix is another issue that we don't fix. This has to be weighed against all of the other work that we're doing and not doing.

[yugabe]

Thanks @jaredpar. I wasn't trying to imply the work was free at all. I understand this is a small niche of a small niche, but there wasn't any activity on this issue, and I was wondering if it was even considered. Thanks for the insight.