-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C#: Extract compilation DB entity in standalone mode #14351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C#: Extract compilation DB entity in standalone mode #14351
Conversation
176f4db
to
de45a9b
Compare
catch (Exception ex) // lgtm[cs/catch-of-all-exceptions] | ||
{ | ||
Logger.Log(Severity.Error, " Unhandled exception analyzing {0}: {1}", "compilation", ex); | ||
} |
Check notice
Code scanning / CodeQL
Generic catch clause
catch | ||
{ } |
Check notice
Code scanning / CodeQL
Poor error handling: empty catch block
catch | ||
{ } |
Check notice
Code scanning / CodeQL
Generic catch clause
DCA looks okay: There are differences in the results of |
@@ -143,5 +143,37 @@ public static string NestPaths(ILogger logger, string? outerpath, string innerpa | |||
} | |||
return nested; | |||
} | |||
|
|||
public static string GetTemporaryWorkingDirectory(out bool shouldCleanUp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes sense to put these methods in class that implements IDisposable
to ensure that cleanup happens if needed? Could TemporaryDirectory
be re-used with some extra exposed methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup of the temporary directory is somewhat error prone. At the moment we basically expect the temporary directory to be cleaned up only at (or close to) application exit. Putting it in a more reusable class would suggest that you can dispose it any time, but that would break the application, because we currently store generated source files in this folder too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
{ | ||
progressMonitor.MissingNamespace(type); | ||
} | ||
var output = FileUtils.CreateTemporaryFile(".dll", out var shouldCleanUpContainingFolder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we risk that anyone relies on us producing a csharp.dll
for traced extraction?
Maybe we need a change note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
csharp.dll
was only used for standalone extraction, so I think changing it won't cause any major issue for anyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, of course - you are right.
this.options = options; | ||
LogExtractorInfo(Extraction.Extractor.Version); | ||
SetReferencePaths(); | ||
|
||
Entities.Compilation.Settings = (Directory.GetCurrentDirectory(), Array.Empty<string>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency: Can this be put in the Run
method for the standalone extractor like it happens in the tracing extractor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't, because the Run
method is in Semmle.Extraction.CSharp.Standalone
and Compilation
is internal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
No description provided.