-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix analyzer conflicts on Linux #69406
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,4 +87,12 @@ All `SymbolDisplayFormat`s (predefined and user-created) now include parameter n | |
|
||
`IncrementalGeneratorRunStep.Outputs` previously contained `IncrementalStepRunReason.Modified` as `Reason` | ||
when the input to the step was modified in a way that produced a new output. | ||
Now the reason will be reported more accurately as `IncrementalStepRunReason.New`. | ||
Now the reason will be reported more accurately as `IncrementalStepRunReason.New`. | ||
|
||
# Version 4.8.0 | ||
|
||
### Changed `Assembly.Location` behavior in non-Windows | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Massive kudos for remembering to update the docs! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kudos go to @jjonescz for reminding me to do this |
||
|
||
The value of `Assembly.Location` previously held the location on disk where an analyzer or source generator was loaded from. This could be either the original location or the shadow copy location. In 4.8 this will be `""` in certain cases when running on non Windows platforms. This is due the compiler server loading assemblies using `AssemblyLoadContext.LoadFromStream` instead of loading from disk. | ||
jaredpar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
This could already happen in other load scenarios but this change moves it into mainline build scenarios. |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
|
@@ -11,19 +11,46 @@ | |
using System.IO; | ||
using System.Linq; | ||
using System.Reflection; | ||
using System.Runtime.InteropServices; | ||
using System.Runtime.Loader; | ||
|
||
namespace Microsoft.CodeAnalysis | ||
{ | ||
internal enum AnalyzerLoadOption | ||
{ | ||
/// <summary> | ||
/// Once the assembly path is chosen, load it directly from disk at that location | ||
/// </summary> | ||
LoadFromDisk, | ||
|
||
/// <summary> | ||
/// Once the assembly path is chosen, read the contents of disk and load from memory | ||
/// </summary> | ||
/// <remarks> | ||
/// While Windows supports this option it comes with a significant performance penalty due | ||
/// to anti virus scans. It can have a load time of 300-500ms while loading from disk | ||
/// is generally 1-2ms. Use this with caution on Windows. | ||
/// </remarks> | ||
LoadFromStream | ||
} | ||
|
||
internal partial class AnalyzerAssemblyLoader | ||
{ | ||
private readonly AssemblyLoadContext _compilerLoadContext; | ||
private readonly Dictionary<string, DirectoryLoadContext> _loadContextByDirectory = new Dictionary<string, DirectoryLoadContext>(StringComparer.Ordinal); | ||
private readonly AnalyzerLoadOption _loadOption; | ||
|
||
internal AssemblyLoadContext CompilerLoadContext => _compilerLoadContext; | ||
internal AnalyzerLoadOption AnalyzerLoadOption => _loadOption; | ||
|
||
internal AnalyzerAssemblyLoader(AssemblyLoadContext? compilerLoadContext = null) | ||
internal AnalyzerAssemblyLoader() | ||
: this(null, AnalyzerLoadOption.LoadFromDisk) | ||
jaredpar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
} | ||
|
||
internal AnalyzerAssemblyLoader(AssemblyLoadContext? compilerLoadContext, AnalyzerLoadOption loadOption) | ||
{ | ||
_loadOption = loadOption; | ||
_compilerLoadContext = compilerLoadContext ?? AssemblyLoadContext.GetLoadContext(typeof(AnalyzerAssemblyLoader).GetTypeInfo().Assembly)!; | ||
} | ||
|
||
|
@@ -105,7 +132,7 @@ public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader, Ass | |
if (_loader.IsAnalyzerDependencyPath(assemblyPath)) | ||
{ | ||
(_, var loadPath) = _loader.GetAssemblyInfoForPath(assemblyPath); | ||
return LoadFromAssemblyPath(loadPath); | ||
return loadCore(loadPath); | ||
} | ||
|
||
// Next prefer registered dependencies from other directories. Ideally this would not | ||
|
@@ -114,11 +141,24 @@ public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader, Ass | |
// https://github.com/dotnet/roslyn/issues/56442 | ||
if (_loader.GetBestPath(assemblyName) is string bestRealPath) | ||
{ | ||
return LoadFromAssemblyPath(bestRealPath); | ||
return loadCore(bestRealPath); | ||
} | ||
|
||
// No analyzer registered this dependency. Time to fail | ||
return null; | ||
|
||
Assembly loadCore(string assemblyPath) | ||
{ | ||
if (_loader.AnalyzerLoadOption == AnalyzerLoadOption.LoadFromDisk) | ||
{ | ||
return LoadFromAssemblyPath(assemblyPath); | ||
} | ||
else | ||
{ | ||
using var stream = File.Open(assemblyPath, FileMode.Open, FileAccess.Read, FileShare.Read); | ||
return LoadFromStream(stream); | ||
Comment on lines
+158
to
+159
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Random question: for the performance penalty on Windows due the virus scanner, is that because .NET is explicitly invoking the virus scanner on the stream data, or just because the virus scanner doesn't see this as a memory mapped file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The runtime team said it has to do with caching. When the file is on disk, even when it's copied, the scans can be cached, while when it's in memory they cannot be. Feels like you could still do a SHA-256 cache but right now this is the reality 😦 |
||
} | ||
} | ||
} | ||
|
||
protected override IntPtr LoadUnmanagedDll(string unmanagedDllName) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
using System.IO; | ||
using System.Linq; | ||
using System.Reflection; | ||
using System.Runtime.InteropServices; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis | ||
|
@@ -17,14 +18,18 @@ internal sealed class DefaultAnalyzerAssemblyLoader : AnalyzerAssemblyLoader | |
{ | ||
#if NETCOREAPP | ||
|
||
// Called from a netstandard2.0 project, so need to ensure a parameterless constructor is available. | ||
internal DefaultAnalyzerAssemblyLoader() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To @RikkiGibson's other question -- do we need this still? Because I don't quite understand the comment here that we had a net2.0 project calling something #ifdef'ed in a NETCOREAPP TFM? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a need for a parameterless ctor but I think it can be pulled out of the |
||
: this(null) | ||
{ | ||
} | ||
|
||
internal DefaultAnalyzerAssemblyLoader(System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null) | ||
: base(compilerLoadContext) | ||
internal DefaultAnalyzerAssemblyLoader(System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null, AnalyzerLoadOption loadOption = AnalyzerLoadOption.LoadFromDisk) | ||
: base(compilerLoadContext, loadOption) | ||
{ | ||
} | ||
|
||
#else | ||
|
||
internal DefaultAnalyzerAssemblyLoader() | ||
{ | ||
} | ||
|
||
|
@@ -36,5 +41,42 @@ internal DefaultAnalyzerAssemblyLoader(System.Runtime.Loader.AssemblyLoadContext | |
/// <param name="fullPath"></param> | ||
/// <returns></returns> | ||
protected override string PreparePathToLoad(string fullPath) => fullPath; | ||
|
||
/// <summary> | ||
/// Return an <see cref="IAnalyzerAssemblyLoader"/> which does not lock assemblies on disk that is | ||
/// most appropriate for the current platform. | ||
/// </summary> | ||
/// <param name="windowsShadowPath">A shadow copy path will be created on Windows and this value | ||
/// will be the base directory where shadow copy assemblies are stored. </param> | ||
internal static IAnalyzerAssemblyLoader CreateNonLockingLoader(string windowsShadowPath) | ||
{ | ||
#if NETCOREAPP | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you restructure this as: #if NETCOREAPP
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return new DefaultAnalyzerAssemblyLoader(loadOption: AnalyzerLoadOption.LoadFromStream);
}
#endif
// (the contents of createShadowLoaderWindows) You can shorten this up a bit and also remove the local function. That might make it a bit clearer that we are only doing something special on NETCOREAPP and non-Windows. |
||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
// The cost of doing stream based loading on Windows is too expensive and we must continue to | ||
// use the shadow copy loader. | ||
return createShadowLoaderWindows(); | ||
} | ||
else | ||
{ | ||
return new DefaultAnalyzerAssemblyLoader(loadOption: AnalyzerLoadOption.LoadFromStream); | ||
} | ||
#else | ||
return createShadowLoaderWindows(); | ||
#endif | ||
|
||
ShadowCopyAnalyzerAssemblyLoader createShadowLoaderWindows() | ||
{ | ||
// The shadow copy analyzer should only be created on Windows. To create on Linux we cannot use | ||
// GetTempPath as it's not per-user. Generally there is no need as LoadFromStream achieves the same | ||
// effect | ||
if (!Path.IsPathRooted(windowsShadowPath)) | ||
{ | ||
throw new ArgumentException("Must be a full path.", nameof(windowsShadowPath)); | ||
} | ||
|
||
return new ShadowCopyAnalyzerAssemblyLoader(windowsShadowPath); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ internal sealed class RemoteAnalyzerAssemblyLoader : AnalyzerAssemblyLoader | |
|
||
public RemoteAnalyzerAssemblyLoader(string baseDirectory) | ||
{ | ||
// jason should we load from stream here on Linux or is this VS only? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jasonmalinowski I wanted you to look at this. The current state is always loading directly from disk (no shadow copy) and confirm it is the correct decision. If so I'll just leave this file alone (maybe add a comment to the effect). If this runs on Linux we could consider doing a stream based load here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file isn't used on Linux, so your question may be somewhat moot. But the comment here makes me think that this kicks in if we're loading analyzers that shipped in Roslyn itself, so reading directly from disk is fine since there's not locking concerns in the first place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @genlu may also want to chime in here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the sole purpose of this is to load our own assemblies in ServiceHub process |
||
_baseDirectory = baseDirectory; | ||
} | ||
|
||
|
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.
Might want to adjust the title to say something about analyzers/generators so the context is clearer from the outset.