Self review feedback addressed

src/Compilers/CSharp/Portable/CommandLine/CSharpCompiler.cs

@@ -137,7 +137,7 @@ public override Compilation CreateCompilation(TextWriter consoleOutput, TouchedF
                 return null;
             }
 
-            var loggingFileSystem = new LoggingStrongNameFileSystem(touchedFilesLogger);
+            var loggingFileSystem = new LoggingStrongNameFileSystem(touchedFilesLogger, _tempDirectory);
 
             return CSharpCompilation.Create(
                 Arguments.CompilationName,
@@ -147,7 +147,7 @@ public override Compilation CreateCompilation(TextWriter consoleOutput, TouchedF
                     WithMetadataReferenceResolver(referenceDirectiveResolver).
                     WithAssemblyIdentityComparer(assemblyIdentityComparer).
                     WithXmlReferenceResolver(xmlFileResolver).
-                    WithStrongNameProvider(Arguments.GetStrongNameProvider(loggingFileSystem, _tempDirectory)).
+                    WithStrongNameProvider(Arguments.GetStrongNameProvider(loggingFileSystem)).
                     WithSourceReferenceResolver(sourceFileResolver));
         }
 

src/Compilers/CSharp/Test/Emit/Attributes/EmitTestStrongNameProvider.cs

 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
-// The use of SigningTestHelpers makes this necessary for now. Fix is tracked by https://github.com/dotnet/roslyn/issues/25228
-#if NET472
 using System;
 using System.Collections.Immutable;
 using System.IO;
+using Microsoft.CodeAnalysis.Test.Utilities;
 using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
 using Roslyn.Test.Utilities;
 using Xunit;
@@ -14,65 +13,16 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests
 {
     public partial class InternalsVisibleToAndStrongNameTests : CSharpTestBase
     {
-        private class TestDesktopStrongNameProvider : DesktopStrongNameProvider
-        {
-            private class TestStrongNameFileSystem : StrongNameFileSystem
-            {
-                private readonly Func<string, byte[]> _readAllBytes = null;
-
-                internal TestStrongNameFileSystem(Func<string, byte[]> readAllBytes = null)
-                {
-                    _readAllBytes = readAllBytes;
-                }
-
-                internal override byte[] ReadAllBytes(string fullPath)
-                {
-                    if (_readAllBytes != null)
-                    {
-                        return _readAllBytes(fullPath);
-                    }
-                    else
-                    {
-                        return base.ReadAllBytes(fullPath);
-                    }
-                }
-            }
-
-            internal delegate void ReadKeysFromContainerDelegate(
-                string keyContainer,
-                out ImmutableArray<byte> publicKey);
-
-            private readonly ReadKeysFromContainerDelegate m_readKeysFromContainer;
-
-            public TestDesktopStrongNameProvider(
-                Func<string, byte[]> readAllBytes = null,
-                ReadKeysFromContainerDelegate readKeysFromContainer = null) : base(ImmutableArray<string>.Empty, new TestStrongNameFileSystem(readAllBytes))
-            {
-                m_readKeysFromContainer = readKeysFromContainer;
-            }
-
-            internal override void ReadKeysFromContainer(string keyContainer, out ImmutableArray<byte> publicKey)
-            {
-                if (m_readKeysFromContainer != null)
-                {
-                    m_readKeysFromContainer(keyContainer, out publicKey);
-                }
-                else
-                {
-                    base.ReadKeysFromContainer(keyContainer, out publicKey);
-                }
-            }
-        }
-
-        [ConditionalFact(typeof(WindowsDesktopOnly), Reason = "https://github.com/dotnet/roslyn/issues/30152")]
+        [Fact]
         [WorkItem(209695, "https://devdiv.visualstudio.com/DevDiv/_workitems?id=209694")]
         public void ExceptionInReadAllBytes()
         {
             var ex = new Exception("Crazy exception you could never have predicted!");
-            var provider = new TestDesktopStrongNameProvider((_) =>
+            var fileSystem = new TestStrongNameFileSystem()
             {
-                throw ex;
-            });
+                ReadAllBytesFunc = _ => throw ex
+            };
+            var provider = new TestDesktopStrongNameProvider(fileSystem: fileSystem);
 
             var src = @"class C {}";
             var keyFile = Temp.CreateFile().WriteAllBytes(TestResources.General.snKey).Path;
@@ -86,15 +36,14 @@ public void ExceptionInReadAllBytes()
                 Diagnostic(ErrorCode.ERR_PublicKeyFileFailure).WithArguments(keyFile, ex.Message).WithLocation(1, 1));
         }
 
-        [ConditionalFact(typeof(WindowsDesktopOnly), Reason = "https://github.com/dotnet/roslyn/issues/30152")]
+        [Fact]
         public void ExceptionInReadKeysFromContainer()
         {
             var ex = new Exception("Crazy exception you could never have predicted!");
-            var provider = new TestDesktopStrongNameProvider(readKeysFromContainer:
-                (string _1, out ImmutableArray<byte> _2) =>
+            var provider = new TestDesktopStrongNameProvider()
             {
-                throw ex;
-            });
+                ReadKeysFromContainerFunc = (string _, out ImmutableArray<byte> publicKey) => throw ex
+            };
 
             var src = @"class C {}";
             var options = TestOptions.DebugDll
@@ -108,4 +57,3 @@ public void ExceptionInReadKeysFromContainer()
         }
     }
 }
-#endif

src/Compilers/CSharp/Test/Emit/Attributes/InternalsVisibleToAndStrongNameTests.cs

@@ -182,9 +182,13 @@ public void SigningNotAvailable001()
 
             string s = String.Format("{0}{1}{2}", @"[assembly: System.Reflection.AssemblyKeyFile(@""..\", keyFileName, @""")] public class C {}");
             var syntaxTree = Parse(s, @"IVTAndStrongNameTests\AnotherTempDir\temp.cs", TestOptions.RegularWithLegacyStrongName);
-
-            var options = TestOptions.ReleaseDll
-                .WithStrongNameProvider(GetProviderWithPath(PathUtilities.CombineAbsoluteAndRelativePaths(keyFileDir, @"TempSubDir\")));
+            var provider = new TestDesktopStrongNameProvider(
+                ImmutableArray.Create(PathUtilities.CombineAbsoluteAndRelativePaths(keyFileDir, @"TempSubDir\")),
+                new VirtualizedStrongNameFileSystem())
+            {
+                GetStrongNameInterfaceFunc = () => throw new DllNotFoundException("aaa.dll not found.")
+            };
+            var options = TestOptions.ReleaseDll.WithStrongNameProvider(provider);
 
             // verify failure
             var comp = CreateCompilation(
@@ -192,13 +196,6 @@ public void SigningNotAvailable001()
                 source: new[] { syntaxTree },
                 options: options);
 
-            var provider = (DesktopStrongNameProvider)comp.Options.StrongNameProvider;
-
-            provider.TestStrongNameInterfaceFactory = () =>
-            {
-                throw new DllNotFoundException("aaa.dll not found.");
-            };
-
             comp.VerifyEmitDiagnostics(
                 // error CS7027: Error signing output with public key from file '..\KeyPair_6187d0d6-f691-47fd-985b-03570bc0668d.snk' -- aaa.dll not found.
                 Diagnostic(ErrorCode.ERR_PublicKeyFileFailure).WithArguments("..\\" + keyFileName, "aaa.dll not found.").WithLocation(1, 1)
@@ -2662,6 +2659,34 @@ public void ConsistentErrorMessageWhenProvidingEmptyKeyFile_PublicSign(CSharpPar
                 Diagnostic(ErrorCode.ERR_PublicSignButNoKey).WithLocation(1, 1));
         }
 
+        [ConditionalFact(typeof(WindowsOnly), Reason = ConditionalSkipReason.TestExecutionHasCOMInterop)]
+        public void LegacyDoesNotUseBuilder()
+        {
+            var provider = new TestDesktopStrongNameProvider(fileSystem: new VirtualizedStrongNameFileSystem())
+            {
+                SignBuilderFunc = delegate { throw null; }
+            };
+
+            var options = TestOptions.ReleaseDll
+                .WithStrongNameProvider(provider)
+                .WithCryptoKeyFile(s_keyPairFile);
+            var other = CreateCompilation(
+            @"
+public class C
+{
+  static void Goo() {}
+}", options: options, parseOptions: TestOptions.RegularWithLegacyStrongName);
+
+            var tempFile = Temp.CreateFile();
+
+            using (var outStrm = tempFile.Open())
+            {
+                var success = other.Emit(outStrm);
+                Assert.True(success.Success);
+            }
+
+            Assert.True(IsFileFullSigned(tempFile));
+        }
         #endregion
     }
 }

src/Compilers/Core/Portable/CommandLine/CommonCommandLineArguments.cs

@@ -282,9 +282,8 @@ public CompilationOptions CompilationOptions
         /// </summary>
         public CultureInfo PreferredUILang { get; internal set; }
 
-        internal StrongNameProvider GetStrongNameProvider(
-            StrongNameFileSystem fileSystem,
-            string tempDirectory) => new DesktopStrongNameProvider(KeyFileSearchPaths, fileSystem);
+        internal StrongNameProvider GetStrongNameProvider(StrongNameFileSystem fileSystem)
+            => new DesktopStrongNameProvider(KeyFileSearchPaths, fileSystem);
 
         internal CommandLineArguments()
         {

src/Compilers/Core/Portable/CommandLine/CommonCompiler.LoggingStrongNameProvider.cs

@@ -10,7 +10,8 @@ internal sealed class LoggingStrongNameFileSystem : StrongNameFileSystem
         {
             private readonly TouchedFileLogger _loggerOpt;
 
-            public LoggingStrongNameFileSystem(TouchedFileLogger logger)
+            public LoggingStrongNameFileSystem(TouchedFileLogger logger, string tempPath)
+                : base(tempPath)
             {
                 _loggerOpt = logger;
             }

src/Compilers/Core/Portable/Compilation.EmitStream.cs

@@ -20,6 +20,11 @@ internal enum EmitStreamSignKind
             SignedWithFile,
         }
 
+        /// <summary>
+        /// This type abstracts away the legacy COM based signing implementation for PE streams. Under the hood
+        /// a temporary file must be created on disk (at the last possible moment), emitted to, signed on disk
+        /// and then copied back to the original <see cref="Stream"/>. Only when legacy signing is enabled though.
+        /// </summary>
         internal sealed class EmitStream
         {
             private readonly EmitStreamProvider _emitStreamProvider;

src/Compilers/Core/Portable/StrongName/DesktopStrongNameProvider.cs

@@ -35,9 +35,6 @@ internal sealed class ClrStrongNameMissingException : Exception
         private readonly ImmutableArray<string> _keyFileSearchPaths;
         internal override StrongNameFileSystem FileSystem { get; }
 
-        // for testing/mocking
-        internal Func<IClrStrongName> TestStrongNameInterfaceFactory;
-
         public DesktopStrongNameProvider(ImmutableArray<string> keyFileSearchPaths) : this(keyFileSearchPaths, StrongNameFileSystem.Instance)
         {
 
@@ -185,16 +182,8 @@ internal override void SignBuilder(ExtendedPEBuilder peBuilder, BlobBuilder peBl
         // public key to establish the assembly name and another to do
         // the actual signing
 
-        // internal for testing
-        internal IClrStrongName GetStrongNameInterface()
+        internal virtual IClrStrongName GetStrongNameInterface()
         {
-            var factoryCreated = TestStrongNameInterfaceFactory?.Invoke();
-
-            if (factoryCreated != null)
-            {
-                return factoryCreated;
-            }
-
             try
             {
                 return ClrStrongName.GetInstance();
@@ -251,7 +240,6 @@ private void Sign(string filePath, string keyName)
             }
         }
 
-        /// <exception cref="IOException"/>
         private unsafe void Sign(string filePath, ImmutableArray<byte> keyPair)
         {
             try

src/Compilers/VisualBasic/Portable/CommandLine/VisualBasicCompiler.vb

@@ -135,7 +135,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
             ' TODO: support for #load search paths
             Dim sourceFileResolver = New LoggingSourceFileResolver(ImmutableArray(Of String).Empty, Arguments.BaseDirectory, Arguments.PathMap, touchedFilesLogger)
 
-            Dim loggingFileSystem = New LoggingStrongNameFileSystem(touchedFilesLogger)
+            Dim loggingFileSystem = New LoggingStrongNameFileSystem(touchedFilesLogger, _tempDirectory)
 
             Return VisualBasicCompilation.Create(
                  Arguments.CompilationName,
@@ -145,7 +145,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                      WithMetadataReferenceResolver(referenceDirectiveResolver).
                      WithAssemblyIdentityComparer(assemblyIdentityComparer).
                      WithXmlReferenceResolver(xmlFileResolver).
-                     WithStrongNameProvider(Arguments.GetStrongNameProvider(loggingFileSystem, _tempDirectory)).
+                     WithStrongNameProvider(Arguments.GetStrongNameProvider(loggingFileSystem)).
                      WithSourceReferenceResolver(sourceFileResolver))
         End Function
 

src/Compilers/VisualBasic/Test/Emit/Attributes/EmitTestStrongNameProvider.vb

@@ -5,6 +5,7 @@ Imports System.Reflection.Metadata
 Imports System.Security.Cryptography
 Imports Microsoft.Cci
 Imports Microsoft.CodeAnalysis
+Imports Microsoft.CodeAnalysis.Test.Utilities
 Imports Microsoft.CodeAnalysis.VisualBasic
 Imports Microsoft.CodeAnalysis.VisualBasic.UnitTests
 Imports Roslyn.Test.Utilities
@@ -13,44 +14,14 @@ Imports Roslyn.Test.Utilities.SigningTestHelpers
 Partial Public Class InternalsVisibleToAndStrongNameTests
     Inherits BasicTestBase
 
-    Private Class StrongNameProviderWithBadInputStream
-        Inherits StrongNameProvider
-        Private _underlyingProvider As StrongNameProvider
-        Public Property ThrownException As Exception
-
-        Friend Overrides ReadOnly Property FileSystem As StrongNameFileSystem = ThrowingStrongNameFileSystem.Instance
-
-        Public Sub New(underlyingProvider As StrongNameProvider, thrownException As Exception)
-            _underlyingProvider = underlyingProvider
-            Me.ThrownException = thrownException
-        End Sub
-
-        Public Overrides Function Equals(other As Object) As Boolean
-            Return Me Is other
-        End Function
-
-        Public Overrides Function GetHashCode() As Integer
-            Return _underlyingProvider.GetHashCode()
-        End Function
-
-        Friend Overrides Function CreateKeys(keyFilePath As String, keyContainerName As String, hasCounterSigature As Boolean, messageProvider As CommonMessageProvider) As StrongNameKeys
-            Return _underlyingProvider.CreateKeys(keyFilePath, keyContainerName, hasCounterSigature, messageProvider)
-        End Function
-
-        Friend Overrides Sub SignFile(keys As StrongNameKeys, filePath As String)
-            Throw ThrownException
-        End Sub
-
-        Friend Overrides Sub SignBuilder(peWriter As ExtendedPEBuilder, peBlob As BlobBuilder, privkey As RSAParameters)
-            Throw ThrownException
-        End Sub
-    End Class
-
-    <ConditionalFact(GetType(WindowsOnly), Reason:=ConditionalSkipReason.TestExecutionNeedsWindowsTypes)>
+    <Fact>
     Public Sub BadInputStream()
         SigningTestHelpers.InstallKey()
         Dim thrownException = New IOException("This is a test IOException")
-        Dim testProvider = New StrongNameProviderWithBadInputStream(DefaultDesktopStrongNameProvider, thrownException)
+        Dim testProvider = New TestDesktopStrongNameProvider() With {
+            .SignFileFunc = Sub(a, b) Throw thrownException
+        }
+
         Dim options = TestOptions.DebugDll.WithStrongNameProvider(testProvider).WithCryptoKeyContainer("RoslynTestContainer")
 
         Dim comp = CreateCompilationWithMscorlib40(
@@ -65,7 +36,7 @@ End Class
             </compilation>, options:=options)
 
         comp.Emit(New MemoryStream()).Diagnostics.Verify(
-            Diagnostic(ERRID.ERR_PeWritingFailure).WithArguments(testProvider.ThrownException.ToString()).WithLocation(1, 1))
+            Diagnostic(ERRID.ERR_PeWritingFailure).WithArguments(thrownException.ToString()).WithLocation(1, 1))
 
     End Sub
 

src/Test/Utilities/Portable/Compilation/TestDesktopStrongNameProvider.cs

+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Immutable;
+using System.Reflection.Metadata;
+using System.Security.Cryptography;
+using Microsoft.Cci;
+using Microsoft.CodeAnalysis.Interop;
+
+namespace Microsoft.CodeAnalysis.Test.Utilities
+{
+    internal sealed class TestDesktopStrongNameProvider : DesktopStrongNameProvider
+    {
+        internal delegate void ReadKeysFromContainerDelegate(
+            string keyContainer,
+            out ImmutableArray<byte> publicKey);
+
+        internal ReadKeysFromContainerDelegate ReadKeysFromContainerFunc { get; set; }
+        internal Action<ExtendedPEBuilder, BlobBuilder, RSAParameters> SignBuilderFunc { get; set; }
+        internal Action<StrongNameKeys, string> SignFileFunc { get; set; }
+        internal Func<IClrStrongName> GetStrongNameInterfaceFunc { get; set; }
+
+        public TestDesktopStrongNameProvider(ImmutableArray<string> keyFileSearchPaths = default, StrongNameFileSystem fileSystem = null)
+            : base(keyFileSearchPaths, fileSystem)
+        {
+            ReadKeysFromContainerFunc = base.ReadKeysFromContainer;
+            SignBuilderFunc = base.SignBuilder;
+            SignFileFunc = base.SignFile;
+            GetStrongNameInterfaceFunc = base.GetStrongNameInterface;
+        }
+
+        internal override void ReadKeysFromContainer(string keyContainer, out ImmutableArray<byte> publicKey) => ReadKeysFromContainerFunc(keyContainer, out publicKey);
+
+        internal override void SignFile(StrongNameKeys keys, string filePath) => SignFileFunc(keys, filePath);
+
+        internal override void SignBuilder(ExtendedPEBuilder peBuilder, BlobBuilder peBlob, RSAParameters privateKey) => SignBuilderFunc(peBuilder, peBlob, privateKey);
+
+        internal override IClrStrongName GetStrongNameInterface() => GetStrongNameInterfaceFunc();
+    }
+}

src/Test/Utilities/Portable/Compilation/TestStrongNameFileSystem.cs

+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
+
+using System;
+
+namespace Microsoft.CodeAnalysis.Test.Utilities
+{
+    internal sealed class TestStrongNameFileSystem : StrongNameFileSystem
+    {
+        internal Func<string, byte[]> ReadAllBytesFunc { get; set; }
+
+        internal TestStrongNameFileSystem()
+        {
+            ReadAllBytesFunc = base.ReadAllBytes;
+        }
+
+        internal override byte[] ReadAllBytes(string fullPath) => ReadAllBytesFunc(fullPath);
+    }
+}

src/Test/Utilities/Portable/Platform/Custom/SigningTestHelpers.cs

@@ -35,15 +35,20 @@ internal static class SigningTestHelpers
 
         internal static object s_keyInstalledLock = new object();
 
-        // Modifies machine wide state.
+        /// <summary>
+        /// Installs the keys used for testing into the machine cache on Windows.
+        /// </summary>
         internal unsafe static void InstallKey()
         {
-            lock (s_keyInstalledLock)
+            if (ExecutionConditionUtil.IsWindows)
             {
-                if (!s_keyInstalled)
+                lock (s_keyInstalledLock)
                 {
-                    InstallKey(TestResources.General.snKey, TestContainerName);
-                    s_keyInstalled = true;
+                    if (!s_keyInstalled)
+                    {
+                        InstallKey(TestResources.General.snKey, TestContainerName);
+                        s_keyInstalled = true;
+                    }
                 }
             }
         }