C# Change desktop dotnet assembly lookup to fall back to nuget reference assemblies #15600
+386
−60
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tamasvajk
commented
Feb 13, 2024
|
|
||
| var dir = monoDirs.FirstOrDefault(Directory.Exists); | ||
| var monoPathEnv = Environment.GetEnvironmentVariable(EnvironmentVariableNames.MonoPath); |
There was a problem hiding this comment.
Should this path be mono specific? We could add instead CODEQL_EXTRACTOR_CSHARP_BUILDLESS_NET_FRAMEWORK_PATH, and then we could also cover the above Windows branch too.
There was a problem hiding this comment.
Yes, that is a good idea!
We should we able to "override" the entire logic in DesktopRuntimes by setting the environment variable.
| @@ -434,19 +444,19 @@ private void GenerateSourceFileFromImplicitUsings() | |||
| } | |||
|
|
|||
| // Hardcoded values from https://learn.microsoft.com/en-us/dotnet/core/project-sdk/overview#implicit-using-directives | |||
| usings.UnionWith(new[] { "System", "System.Collections.Generic", "System.IO", "System.Linq", "System.Net.Http", "System.Threading", | |||
| "System.Threading.Tasks" }); | |||
| usings.UnionWith([ "System", "System.Collections.Generic", "System.IO", "System.Linq", "System.Net.Http", "System.Threading", | |||
There was a problem hiding this comment.
Very nice with all the collection expression re-factoring!
| @@ -0,0 +1,8 @@ | |||
| namespace Semmle.Extraction.CSharp.DependencyFetching | |||
| { | |||
| internal class EnvironmentVariableNames | |||
There was a problem hiding this comment.
Yes, this is a good idea!
Maybe consider to write a small comment above each const, what the purpose of the environment variable is.
michaelnebel
approved these changes
Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 directory. This is also used in the corresponding integration test.
Commit-by-commit review is suggested.