Add support for suppressing warning messages #1393

JohnTortugo · 2018-11-27T01:34:18Z

Closes: #1295

With this change SignTool will support the user to specify warning messages that should be disabled in the scope of a specific file or the whole signing process. For instance, first candidate for this is the SIGN001 warning.

Changes:

SignToolTests.cs : Two new test cases added
SignToolConstants.cs : Moved definition of error messages here
Configuration.cs : Receive the new parameters and check the suppress condition.
SignToolTask.cs : Receive & parse the new parameter / attribute.

JohnTortugo · 2018-11-27T21:30:49Z

Can someone take peek?

jcagme · 2018-11-27T23:27:13Z

src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

@@ -191,7 +193,7 @@ public void Dispose()
            var signToolArgs = new SignToolArgs(_tmpDir, microBuildCorePath: "MicroBuildCorePath", testSign: true, msBuildPath: null, _tmpDir, enclosingDir: "", "");

            var signTool = new FakeSignTool(signToolArgs, task.Log);
-            var signingInput = new Configuration(signToolArgs.TempDir, itemsToSign, strongNameSignInfo, fileSignInfo, extensionsSignInfo, dualCertificates, task.Log).GenerateListOfFiles();
+            var signingInput = new Configuration(signToolArgs.TempDir, itemsToSign, strongNameSignInfo, fileSignInfo, globallySupressedMessageCodes, supressedMessageCodes, extensionsSignInfo, dualCertificates, task.Log).GenerateListOfFiles();


signToolArgs.Te [](start = 49, length = 15)

Nit: move each parameter to its own line.

jcagme · 2018-11-27T23:27:50Z

src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

        {
            var engine = new FakeBuildEngine();
            var task = new SignToolTask { BuildEngine = engine };
-            var signingInput = new Configuration(_tmpDir, itemsToSign, strongNameSignInfo, fileSignInfo, extensionsSignInfo, dualCertificates, task.Log).GenerateListOfFiles();
+            var signingInput = new Configuration(_tmpDir, itemsToSign, strongNameSignInfo, fileSignInfo, globallySupressedMessageCodes, supressedMessageCodes, extensionsSignInfo, dualCertificates, task.Log).GenerateListOfFiles();


var signingInput [](start = 12, length = 16)

Nit: same as above

jcagme · 2018-11-27T23:30:06Z

src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs

+                $@"SIGN001: Signing 3rd party library '{Path.Combine(_tmpDir, "ContainerSigning", "B306A318B3A11BF342995F6A1FC5AADF5DB4DD49F4EFF7E013D31208DD58EBDC", "lib/net461/ProjectOne.dll")}' with Microsoft certificate 'Microsoft400'. The library is considered 3rd party library due to its copyright: ''.",
+                $@"SIGN001: Signing 3rd party library '{Path.Combine(_tmpDir, "ContainerSigning", "9F8CCEE4CECF286C80916F13EAB8DF1FC6C9BED5F81E3AFF26747C008D265E5C", "lib/netcoreapp2.0/ContainerOne.dll")}' with Microsoft certificate 'Microsoft400'. The library is considered 3rd party library due to its copyright: ''.",
+                $@"SIGN001: Signing 3rd party library '{Path.Combine(_tmpDir, "ContainerSigning", "CC1D99EE8C2F627E77D019E94B06EBB6D87A4D19E65DDAEF62B6137E49167BAF", "lib/netcoreapp2.1/ProjectOne.dll")}' with Microsoft certificate 'Microsoft400'. The library is considered 3rd party library due to its copyright: ''.",
+                $@"SIGN001: Signing 3rd party library '{Path.Combine(_tmpDir, "ContainerSigning", "47F202CA51AD708535A01E96B95027042F8448333D86FA7D5F8D66B67644ACEC", "lib/netstandard2.0/ProjectOne.dll")}' with Microsoft certificate 'Microsoft400'. The library is considered 3rd party library due to its copyright: ''."


Nit: Split these in more lines so there is no need to scroll right/left in CodeFlow as in GitHub

I wouldn't do that here. The actual message is not that interesting to review.

In reply to: 236888896 [](ancestors = 236888896)

jcagme · 2018-11-27T23:32:01Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

+
+        private readonly Dictionary<ExplicitCertificateKey, HashSet<SignToolConstants.SigningToolMSGCodes>> _suppressedMessageCodes;
+
+        private readonly HashSet<SignToolConstants.SigningToolMSGCodes> _globallySupressedMessageCodes;


Nit: Most others have summaries please add them to these as well

jcagme · 2018-11-27T23:33:53Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

-            string[] dualCertificates, TaskLoggingHelper log)
+            Dictionary<ExplicitCertificateKey, string> fileSignInfo, HashSet<SignToolConstants.SigningToolMSGCodes> globallySupressedMessageCodes,
+            Dictionary<ExplicitCertificateKey, HashSet<SignToolConstants.SigningToolMSGCodes>> suppressedMessageCodes,
+            Dictionary<string, SignInfo> extensionSignInfo, string[] dualCertificates, TaskLoggingHelper log)


Nit: move to one parameter per line

jcagme · 2018-11-27T23:36:55Z

            GetPEInfo(fullPath, out isManagedPE, out var publicKeyToken, out targetFramework, out copyright);

You could inline this here, just do out string copyright and remove line 214

Refers to: src/Microsoft.DotNet.SignTool/src/Configuration.cs:224 in 4081535. [](commit_id = 4081535, deletion_comment = False)

jcagme · 2018-11-27T23:38:03Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

+                // Check if there is any configuration to disable messages for this specific file
+                if (!_suppressedMessageCodes.TryGetValue(new ExplicitCertificateKey(fileName, publicKeyToken, targetFramework), out suppressedMessages))
+                {
+                    _suppressedMessageCodes.TryGetValue(new ExplicitCertificateKey(fileName, publicKeyToken), out suppressedMessages);


Same inline comment for supressedMessages

jcagme · 2018-11-27T23:38:27Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

+                // Check if there is any configuration to disable messages for this specific file
+                if (!_suppressedMessageCodes.TryGetValue(new ExplicitCertificateKey(fileName, publicKeyToken, targetFramework), out suppressedMessages))
+                {
+                    _suppressedMessageCodes.TryGetValue(new ExplicitCertificateKey(fileName, publicKeyToken), out suppressedMessages);


_suppressedMessageCodes.TryGetValue [](start = 20, length = 35)

What if this also returns false?

jcagme · 2018-11-27T23:42:08Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

+            // at least not by checking with PKT + TargetFramework.
+            if (suppressedMessages == null)
+            {
+                _suppressedMessageCodes.TryGetValue(new ExplicitCertificateKey(fileName), out suppressedMessages);
            }


I'd check the returned value in 243 and if its false I'd execute this code to try to keep related code close together

I can do that. I did this way to follow the pattern of the existing code.

jcagme · 2018-11-27T23:46:04Z

        if (SignToolConstants.IgnoreFileCertificateSentinel.Equals(explicitCertificateName, StringComparison.OrdinalIgnoreCase))

There is a chance this is null and I assume SignToolConstants.IgnoreFileCertificateSentinel is none, should we move this if inside the if in line 268?

Refers to: src/Microsoft.DotNet.SignTool/src/Configuration.cs:261 in 4081535. [](commit_id = 4081535, deletion_comment = False)

jcagme · 2018-11-27T23:46:33Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

-                    bool isMicrosoftLibrary = IsMicrosoftLibrary(copyright);
-                    bool isMicrosoftCertificate = !IsThirdPartyCertificate(signInfo.Certificate);
-                    if (isMicrosoftLibrary != isMicrosoftCertificate)
+                    if (!_globallySupressedMessageCodes.Contains(SigningToolMSGCodes.SIGN001) && (suppressedMessages == null || !suppressedMessages.Contains(SigningToolMSGCodes.SIGN001))) 


if (!_g [](start = 20, length = 7)

NIt: Move each condition to its own line

jcagme · 2018-11-27T23:47:22Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

-                            LogWarning("SIGN001", $"Signing 3rd party library '{fullPath}' with Microsoft certificate '{signInfo.Certificate}'. The library is considered 3rd party library due to its copyright: '{copyright}'.");
+                            if (isMicrosoftLibrary)
+                            {
+                                LogWarning(SigningToolMSGCodes.SIGN001, $"Signing Microsoft library '{fullPath}' with 3rd party certificate '{signInfo.Certificate}'. The library is considered Microsoft library due to its copyright: '{copyright}'.");


LogWarning(Si [](start = 32, length = 13)

Nit: Break these warnings down in different lines

jcagme · 2018-11-27T23:47:54Z

src/Microsoft.DotNet.SignTool/src/SignToolConstants.cs

@@ -11,6 +11,9 @@ internal static class SignToolConstants
    {
        public const string IgnoreFileCertificateSentinel = "None";

+        public enum SigningToolMSGCodes { SIGN001, SIGN002 };
+
+


Remove blank line

jcagme · 2018-11-27T23:49:04Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

@@ -163,16 +167,18 @@ public void ExecuteImpl()

            if (Log.HasLoggedErrors) return;

+            ParseFileSignInfo(out var fileSignInfo, out var supressedMessageCodes);


out var fileSignInfo, out var supressedMessageCodes [](start = 30, length = 51)

In cases like this I rather use types to understand what these objects are.

jcagme · 2018-11-27T23:49:20Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs


            if (Log.HasLoggedErrors) return;

            var signToolArgs = new SignToolArgs(TempDir, MicroBuildCorePath, TestSign, MSBuildPath, LogDir, enclosingDir, SNBinaryPath);
            var signTool = DryRun ? new ValidationOnlySignTool(signToolArgs, Log) : (SignTool)new RealSignTool(signToolArgs, Log);
-            var signingInput = new Configuration(TempDir, ItemsToSign, strongNameInfo, fileSignInfo, extensionSignInfo, dualCertificates, Log).GenerateListOfFiles();
+            var signingInput = new Configuration(TempDir, ItemsToSign, strongNameInfo, fileSignInfo, globallySuppWarnings, supressedMessageCodes, extensionSignInfo, dualCertificates, Log).GenerateListOfFiles();


var signi [](start = 12, length = 9)

Nit: split

jcagme · 2018-11-27T23:50:04Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

@@ -335,9 +362,10 @@ int getCommonPrefixLength(string[] dir1, string[] dir2)
            return map;
        }

-        private Dictionary<ExplicitCertificateKey, string> ParseFileSignInfo()
+        private void ParseFileSignInfo(out Dictionary<ExplicitCertificateKey, string> explicitCertificateNames, out Dictionary<ExplicitCertificateKey, HashSet<SignToolConstants.SigningToolMSGCodes>> suppressedMessageCodes)


private void P [](start = 8, length = 14)

Nit: split

jcagme · 2018-11-27T23:52:18Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                    {
+                        if (explicitCertificateNames.TryGetValue(key, out var existingCert))
+                        {
+                            Log.LogError($"Duplicate entries in {nameof(FileSignInfo)} with the same key ('{fileName}', '{publicKeyToken}', '{targetFramework}') for CertificateName attribute: '{existingCert}', '{certificateName}'. Keeping the previous one.");


Log.LogError($"Dupli [](start = 28, length = 20)

No need to continue here? Also, split

jcagme · 2018-11-28T00:03:29Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

-                    map.Add(key, certificateName);
+                    if (!string.IsNullOrWhiteSpace(certificateName))
+                    {
+                        if (explicitCertificateNames.TryGetValue(key, out var existingCert))


explicitCertificateNames.TryGetValue [](start = 28, length = 36)

Are you just trying to determine whether this Dictionary contains a key? If so, you could use the ContainsKey method so you don't keep an extra variable around

jcagme · 2018-11-28T00:04:08Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                            }
+                        }
+
+                        if (suppressedMessageCodes.TryGetValue(key, out var existingCert))


suppressedMessageCodes [](start = 28, length = 22)

Use ContainsKey

jcagme · 2018-11-28T00:05:23Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+
+                        if (suppressedMessageCodes.TryGetValue(key, out var existingCert))
+                        {
+                            Log.LogError($"Duplicate entries in {nameof(FileSignInfo)} with the same key ('{fileName}', '{publicKeyToken}', '{targetFramework}') for NoWarn attribute. Keeping the previous one.");


LogError [](start = 32, length = 8)

Also, is this truly an error? I feel is more of information or warning

jcagme

🕐

tmat · 2018-11-29T00:05:04Z

src/Microsoft.DotNet.SignTool/src/SignToolConstants.cs

@@ -11,6 +11,9 @@ internal static class SignToolConstants
    {
        public const string IgnoreFileCertificateSentinel = "None";

+        public enum SigningToolMSGCodes { SIGN001, SIGN002 };


public enum SigningToolMSGCodes { SIGN001, SIGN002 }; [](start = 8, length = 53)

I'd move this enum outside of the class and rename it to DiagnosticId.

tmat · 2018-11-29T00:09:26Z

src/Microsoft.DotNet.SignTool/src/SignToolTask.cs

+                {
+                    if (!Enum.TryParse<SignToolConstants.SigningToolMSGCodes>(warningCode.ItemSpec, true, out var value))
+                    {
+                        Log.LogError($"Unrecognized message code was informed in {nameof(NoWarn)} -> {warningCode}");


nrecognized message code was informed in [](start = 40, length = 41)

This doesn't make sense in English. Instead use something like "Unrecognized warning id specified in {nameof(NoWarn)}: '{warningCode}'"

tmat · 2018-11-29T00:18:47Z

src/Microsoft.DotNet.SignTool/src/Configuration.cs

+                if (!_suppressedMessageCodes.TryGetValue(new ExplicitCertificateKey(fileName, publicKeyToken, targetFramework), out suppressedMessages))
+                {
+                    _suppressedMessageCodes.TryGetValue(new ExplicitCertificateKey(fileName, publicKeyToken), out suppressedMessages);
+                }


This seems pretty complicated. Why not make the NoWarn part of the fileSignInfo map? That is Dictionary<ExplicitCertificateKey, CertificateInfo> where CertificateInfo is struct { Name; ImmutableArray<DiagnosticId> SuppressedWarnings; } .
Then when we look up a file in the map and get the certificate name we immediately see if we have any warning suppressions.

tmat

🕐

JohnTortugo · 2018-11-29T18:23:57Z

Thank you all for reviewing. I'm going to close this PR until we get further evidence that we indeed need this.

Add support for supressing warning messages.

4081535

JohnTortugo self-assigned this Nov 27, 2018

JohnTortugo requested review from tmat, natemcmaster, ericstj and jcagme November 27, 2018 01:34

jcagme reviewed Nov 27, 2018

View reviewed changes

jcagme reviewed Nov 28, 2018

View reviewed changes

jcagme suggested changes Nov 28, 2018

View reviewed changes

tmat reviewed Nov 29, 2018

View reviewed changes

tmat requested changes Nov 29, 2018

View reviewed changes

JohnTortugo closed this Nov 29, 2018

JohnTortugo deleted the SupressWarnings branch January 18, 2019 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for suppressing warning messages #1393

Add support for suppressing warning messages #1393

JohnTortugo commented Nov 27, 2018

JohnTortugo commented Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

tmat Nov 28, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme commented Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

JohnTortugo Nov 27, 2018

jcagme commented Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 27, 2018

jcagme Nov 28, 2018

jcagme Nov 28, 2018

jcagme Nov 28, 2018

jcagme left a comment

tmat Nov 29, 2018 •

edited

Loading

tmat Nov 29, 2018

tmat Nov 29, 2018 •

edited

Loading

tmat left a comment

JohnTortugo commented Nov 29, 2018


		private readonly Dictionary<ExplicitCertificateKey, HashSet<SignToolConstants.SigningToolMSGCodes>> _suppressedMessageCodes;

		private readonly HashSet<SignToolConstants.SigningToolMSGCodes> _globallySupressedMessageCodes;

		@@ -163,16 +167,18 @@ public void ExecuteImpl()

		if (Log.HasLoggedErrors) return;

		ParseFileSignInfo(out var fileSignInfo, out var supressedMessageCodes);

Add support for suppressing warning messages #1393

Add support for suppressing warning messages #1393

Conversation

JohnTortugo commented Nov 27, 2018

JohnTortugo commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcagme commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcagme commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcagme left a comment

Choose a reason for hiding this comment

tmat Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

tmat left a comment

Choose a reason for hiding this comment

JohnTortugo commented Nov 29, 2018

tmat Nov 29, 2018 •

edited

Loading

tmat Nov 29, 2018 •

edited

Loading