C# Change desktop dotnet assembly lookup to fall back to nuget reference assemblies #15600
Conversation
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Runtime.cs
Outdated
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/EnvironmentVariableNames.cs
Show resolved
Hide resolved
tamasvajk
force-pushed
the
buildless/no-mono-dlls
branch
from
81e1435 to
d358f8e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception e) | ||
| { | ||
| logger.LogError($"Error while searching for DLLs in '{path}': {e.Message}"); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs
Outdated
Show resolved
Hide resolved
| RemoveFrameworkNugetPackages(dllPaths); | ||
| RemoveNugetPackageReference(FrameworkPackageNames.AspNetCoreFramework, dllPaths); | ||
| RemoveNugetPackageReference(FrameworkPackageNames.WindowsDesktopFramework, dllPaths); |
There was a problem hiding this comment.
I think this doesn't work on some early .net core variants, where the framework was split up into many nuget packages, and therefore they wouldn't be identified as framework references but as standard nuget packages. But for the moment, I think it's good enough.
| if (TryRestorePackageManually(FrameworkPackageNames.LatestNetFrameworkReferenceAssemblies, null)) | ||
| { | ||
| runtimeLocation = GetPackageDirectory(FrameworkPackageNames.LatestNetFrameworkReferenceAssemblies, missingPackageDirectory); | ||
| } |
There was a problem hiding this comment.
Early commits in this PR tested this behaviour, but that's not the case any longer. Is there an easy way of covering this with an integration test?
There was a problem hiding this comment.
Is the problem that this is an extremely unhappy path that is hard to replicate (as there none of the runners that satisfy the criteria for missing paths)?
There was a problem hiding this comment.
Yes, that's correct. We'd get here if there was no mono installation on linux/macos runners, and no .net install on windows. (I've faced the original issue on a mac with no mono.)
There was a problem hiding this comment.
Hmm, yes, then we have reached the limit for what is integration testable unless we introduce some way of adding extra test configuration (we probably shouldn't do this).
Maybe accept that we can't integration test this?
There was a problem hiding this comment.
When #15650 and this PR are merged, I'll set up some manual testing to see if the extractor is really working without mono.
| | /legacypackages/Newtonsoft.Json.6.0.4/lib/portable-net45+wp80+win8+wpa81/Newtonsoft.Json.dll | | ||
| | /missingpackages/microsoft.netframework.referenceassemblies.net481/1.0.3/build/.NETFramework/v4.8.1/Accessibility.dll | |
There was a problem hiding this comment.
Why do these go into the root and not the DB scratch dir?
There was a problem hiding this comment.
They are in the scratch dir, we substring the paths in the test: https://github.com/github/codeql/pull/15600/files#diff-2f4399343179c5277fff6c12b814c1c1778b27544f0cdff6705fa21392320987
There was a problem hiding this comment.
I should probably prepend [...] to the paths. I'll do it in a followup PR, because it applies to some other integration tests too.
There was a problem hiding this comment.
I'm making this change in #15678.
|
じひのこころ28177です。いろいろと有難うございました。 |
This PR changes the fallback .net framework assembly lookup. The dependency fetching logic looks up well-known .NET framework locations on disk, such as the common installation directory on Windows or Mono paths. When no framework assemblies are located we fall back to the executing runtime. This process is now extended to download the
microsoft.netframework.referenceassemblies.net481nuget package if we couldn't find the assemblies on disk. With this change we're implementing the assumption that in case of old-style csproj files, we're better off with these reference assemblies than with the dotnet core ones that we ship with the codeql CLI.An environment variable is also added to allow users specify the path to the
mono installation.net core/framework assembly folders.Commit-by-commit review is suggested.