Add an option to not strip out PDB symbols from nutget feeds (#689)

.editorconfig

@@ -149,6 +149,9 @@ dotnet_diagnostic.CA2234.severity=none
 dotnet_diagnostic.CA2007.severity=none
 dotnet_diagnostic.CA1056.severity=none # URI-like properties should not be strings
 dotnet_diagnostic.CA1054.severity=none # URI parameters should not be strings
+dotnet_diagnostic.CA2249.severity=none # Cant use String.Contains as it doesn't support StringComparison type on .net framework
+dotnet_diagnostic.CA1307.severity=none # Cant use String.Replace with StringComparison type on .net framework
+dotnet_diagnostic.CA1865.severity=none # Cant use String.StartsWith with char on .net framework
 # ReSharper properties
 resharper_wrap_after_declaration_lpar=true
 

src/NuGetForUnity.Tests/Assets/Tests/Editor/NuGetTests.cs

@@ -15,6 +15,7 @@
 using NUnit.Framework;
 using UnityEditor;
 using UnityEngine;
+using UnityEngine.TestTools;
 
 public class NuGetTests
 {
@@ -1129,6 +1130,42 @@ public void NativeSettingsTest(string runtime, BuildTarget buildTarget)
             $"Native mapping for {runtime} is missing build target {buildTarget}");
     }
 
+    [Test]
+    public void KeepPdbFileTest()
+    {
+        ConfigureNugetConfig(InstallMode.ApiV3Only);
+        var nugetConfigFile = ConfigurationManager.NugetConfigFile;
+        nugetConfigFile.KeepingPdbFiles = true;
+        LogAssert.ignoreFailingMessages = true;
+
+        try
+        {
+            var package = new NugetPackageIdentifier("Fody", "6.8.2") { IsManuallyInstalled = true };
+            Assume.That(
+                InstalledPackagesManager.IsInstalled(package, false),
+                Is.False,
+                "The package IS installed: {0} {1}",
+                package.Id,
+                package.Version);
+
+            NugetPackageInstaller.InstallIdentifier(package);
+            Assert.IsTrue(
+                InstalledPackagesManager.IsInstalled(package, false),
+                "The package was NOT installed: {0} {1}",
+                package.Id,
+                package.Version);
+
+            var packageDirectory = Path.Combine(ConfigurationManager.NugetConfigFile.RepositoryPath, $"{package.Id}.{package.Version}");
+
+            Assert.That(Path.Combine(packageDirectory, "netclassictask", "Mono.Cecil.pdb"), Does.Exist);
+        }
+        finally
+        {
+            LogAssert.ignoreFailingMessages = false;
+            nugetConfigFile.KeepingPdbFiles = false;
+        }
+    }
+
     private static void ConfigureNugetConfig(InstallMode installMode)
     {
         var nugetConfigFile = ConfigurationManager.NugetConfigFile;

src/NuGetForUnity/Editor/Configuration/NugetConfigFile.cs

@@ -1,5 +1,7 @@
 #pragma warning disable SA1512,SA1124 // Single-line comments should not be followed by blank line
 
+#region No ReShaper
+
 using System;
 using System.Collections.Generic;
 using System.Globalization;
@@ -12,8 +14,6 @@
 using NugetForUnity.PackageSource;
 using UnityEngine;
 
-#region No ReShaper
-
 // ReSharper disable All
 // needed because 'JetBrains.Annotations.NotNull' and 'System.Diagnostics.CodeAnalysis.NotNull' collide if this file is compiled with a never version of Unity / C#
 using SuppressMessageAttribute = System.Diagnostics.CodeAnalysis.SuppressMessageAttribute;
@@ -54,6 +54,8 @@ public class NugetConfigFile
 
         private const string PreferNetStandardOverNetFrameworkConfigKey = "PreferNetStandardOverNetFramework";
 
+        private const string KeepingPdbFilesConfigKey = "KeepingPdbFiles";
+
         private const string ProtocolVersionAttributeName = "protocolVersion";
 
         private const string PasswordAttributeName = "password";
@@ -189,10 +191,15 @@ public string RelativePackagesConfigDirectoryPath
         public int RequestTimeoutSeconds { get; set; } = DefaultRequestTimeout;
 
         /// <summary>
-        ///     Gets or sets the value indicating whether .NET Standard is preferred over .NET Framework as the TargetFramework.
+        ///     Gets or sets a value indicating whether .NET Standard is preferred over .NET Framework as the TargetFramework.
         /// </summary>
         public bool PreferNetStandardOverNetFramework { get; set; }
 
+        /// <summary>
+        ///     Gets or sets a value indicating whether PDB files included in NuGet packages should not be deleted if they can be read by Unity.
+        /// </summary>
+        internal bool KeepingPdbFiles { get; set; }
+
         /// <summary>
         ///     Gets the value that tells the system how to determine where the packages are to be installed and configurations are to be stored.
         /// </summary>
@@ -209,6 +216,10 @@ public string RelativePackagesConfigDirectoryPath
         /// <param name="filePath">The full file-path to the NuGet.config file to load.</param>
         /// <returns>The newly loaded <see cref="NugetConfigFile" />.</returns>
         [NotNull]
+        [SuppressMessage(
+            "Usage",
+            "CA2263:Prefer generic overload when type is known",
+            Justification = "When targeting .net-framework there is no generic overload.")]
         public static NugetConfigFile Load([NotNull] string filePath)
         {
             var configFile = new NugetConfigFile
@@ -238,7 +249,10 @@ public static NugetConfigFile Load([NotNull] string filePath)
                         var updateSearchBatchSizeString = add.Attribute(UpdateSearchBatchSizeAttributeName)?.Value;
                         if (!string.IsNullOrEmpty(updateSearchBatchSizeString))
                         {
-                            sourceV3.UpdateSearchBatchSize = Mathf.Clamp(int.Parse(updateSearchBatchSizeString), 1, int.MaxValue);
+                            sourceV3.UpdateSearchBatchSize = Mathf.Clamp(
+                                int.Parse(updateSearchBatchSizeString, CultureInfo.InvariantCulture),
+                                1,
+                                int.MaxValue);
                         }
 
                         var supportsPackageIdSearchFilterString = add.Attribute(SupportsPackageIdSearchFilterAttributeName)?.Value;
@@ -318,7 +332,7 @@ public static NugetConfigFile Load([NotNull] string filePath)
 
                 if (string.Equals(key, "packageInstallLocation", StringComparison.OrdinalIgnoreCase))
                 {
-                    configFile.InstallLocation = (PackageInstallLocation)Enum.Parse(typeof(PackageInstallLocation), value);
+                    configFile.InstallLocation = (PackageInstallLocation)Enum.Parse(typeof(PackageInstallLocation), value, true);
                 }
                 else if (string.Equals(key, "repositoryPath", StringComparison.OrdinalIgnoreCase))
                 {
@@ -356,6 +370,10 @@ public static NugetConfigFile Load([NotNull] string filePath)
                 {
                     configFile.PreferNetStandardOverNetFramework = bool.Parse(value);
                 }
+                else if (string.Equals(key, KeepingPdbFilesConfigKey, StringComparison.OrdinalIgnoreCase))
+                {
+                    configFile.KeepingPdbFiles = bool.Parse(value);
+                }
             }
 
             return configFile;
@@ -547,6 +565,14 @@ public void Save([NotNull] string filePath)
                 config.Add(addElement);
             }
 
+            if (KeepingPdbFiles)
+            {
+                addElement = new XElement("add");
+                addElement.Add(new XAttribute("key", KeepingPdbFilesConfigKey));
+                addElement.Add(new XAttribute("value", KeepingPdbFiles.ToString().ToLowerInvariant()));
+                config.Add(addElement);
+            }
+
             var configuration = new XElement("configuration");
             configuration.Add(packageSources);
             configuration.Add(disabledPackageSources);

src/NuGetForUnity/Editor/Helper/PortableSymbolFileHelper.cs

@@ -0,0 +1,25 @@
+using System.IO;
+
+namespace NugetForUnity.Helper
+{
+    /// <summary>
+    ///     Provides methods for working with symbol files.
+    /// </summary>
+    internal static class PortableSymbolFileHelper
+    {
+        /// <summary>
+        ///     Determines whether the specified PDB file is a portable symbol file.
+        /// </summary>
+        /// <param name="filePath">The path to the PDB file.</param>
+        /// <returns>
+        ///     <c>true</c> if the specified PDB file is a portable symbol file; otherwise, <c>false</c>.
+        /// </returns>
+        public static bool IsPortableSymbolFile(string filePath)
+        {
+            using (var stream = File.OpenRead(filePath))
+            {
+                return stream.ReadByte() == 'B' && stream.ReadByte() == 'S' && stream.ReadByte() == 'J' && stream.ReadByte() == 'B';
+            }
+        }
+    }
+}

src/NuGetForUnity/Editor/InstalledPackagesManager.cs

@@ -362,7 +362,7 @@ internal static List<INugetPackage> GetInstalledRootPackages()
             return roots;
         }
 
-        private static void AddPackageToInstalledInternal([NotNull] INugetPackage package, ref int manuallyInstalledPackagesNumber)
+        private static void AddPackageToInstalledInternal([NotNull] NugetPackageLocal package, ref int manuallyInstalledPackagesNumber)
         {
             var packages = InstalledPackagesDictionary;
             if (!packages.ContainsKey(package.Id))

src/NuGetForUnity/Editor/NugetPackageInstaller.cs

@@ -449,7 +449,7 @@ private static void WriteInitialExcludeAllPluginImporterConfig([NotNull] string
         }
 
         private static void TryExtractBestFrameworkSources(
-            [NotNull] [ItemNotNull] IReadOnlyDictionary<string, List<ZipArchiveEntry>> frameworks,
+            [NotNull] [ItemNotNull] Dictionary<string, List<ZipArchiveEntry>> frameworks,
             [NotNull] string sourceDirName,
             [NotNull] INugetPackage package,
             [NotNull] PackageConfig packageConfig)
@@ -477,7 +477,7 @@ private static void TryExtractBestFrameworkSources(
         }
 
         private static void FillFrameworkZipEntries(
-            IDictionary<string, List<ZipArchiveEntry>> frameworkZipEntries,
+            Dictionary<string, List<ZipArchiveEntry>> frameworkZipEntries,
             string framework,
             ZipArchiveEntry entry)
         {

src/NuGetForUnity/Editor/PackageContentManager.cs

@@ -114,7 +114,7 @@ internal static void CleanInstallationDirectory([NotNull] INugetPackageIdentifie
         /// <returns>True if the file can be skipped, is not needed.</returns>
         internal static bool ShouldSkipUnpackingOnPath([NotNull] string path, PackageInstallOperationResultEntry operationResultEntry)
         {
-            if (path.EndsWith("/"))
+            if (path.EndsWith("/", StringComparison.Ordinal))
             {
                 // We do not want to extract empty directory entries. If there are empty directories within the .nupkg, we
                 // expect them to have a file named '_._' in them that indicates that it should be extracted, usually as a
@@ -208,8 +208,9 @@ internal static bool ShouldSkipUnpackingOnPath([NotNull] string path, PackageIns
                 return true;
             }
 
-            // Skip all PDB files since Unity uses Mono and requires MDB files, which causes it to output "missing MDB" errors
-            if (path.EndsWith(".pdb", StringComparison.OrdinalIgnoreCase))
+            // If not configured skip all PDB files. Unity uses Mono and requires MDB files or portable PDB files, else it produces "missing MDB" errors.
+            if (!ConfigurationManager.NugetConfigFile.KeepingPdbFiles &&
+                (path.EndsWith(".pdb", StringComparison.OrdinalIgnoreCase) || path.EndsWith(".pdb.meta", StringComparison.OrdinalIgnoreCase)))
             {
                 return true;
             }
@@ -325,6 +326,17 @@ internal static string ExtractPackageEntry([NotNull] ZipArchiveEntry entry, [Not
 
             entry.ExtractToFile(filePath, true);
 
+            if (filePath.EndsWith(".pdb", StringComparison.OrdinalIgnoreCase) && !PortableSymbolFileHelper.IsPortableSymbolFile(filePath))
+            {
+                // Unity uses Mono and requires MDB files or portable PDB files, else it produces "missing MDB" errors.
+                new FileInfo(filePath).Delete();
+                new FileInfo($"{filePath}.meta").Delete();
+                NugetLogger.LogVerbose(
+                    "The PDB file '{0}' doesn't have the new PDB file format that can be read by Unity so we delete it.",
+                    filePath);
+                return null;
+            }
+
             if (ConfigurationManager.NugetConfigFile.ReadOnlyPackageFiles)
             {
                 var extractedFile = new FileInfo(filePath);

src/NuGetForUnity/Editor/PackageSource/NugetPackageSourceV2.cs

@@ -393,10 +393,7 @@ public void DownloadNupkgToFile(INugetPackageIdentifier package, string outputFi
         }
 
 #if TEST_GET_UPDATES_FALLBACK
-        private static void ComparePackageLists(
-            List<NugetPackage> updates,
-            List<NugetPackage> updatesReplacement,
-            string errorMessageToDisplayIfListsDoNotMatch)
+        private static void ComparePackageLists(List<NugetPackage> updates, List<NugetPackage> updatesReplacement, string errorMessageToDisplayIfListsDoNotMatch)
         {
             var matchingComparison = new StringBuilder();
             var missingComparison = new StringBuilder();
@@ -426,12 +423,7 @@ private static void ComparePackageLists(
 
             if (missingComparison.Length > 0 || extraComparison.Length > 0)
             {
-                Debug.LogWarningFormat(
-                    "{0}\n{1}\n{2}\n{3}",
-                    errorMessageToDisplayIfListsDoNotMatch,
-                    matchingComparison,
-                    missingComparison,
-                    extraComparison);
+                Debug.LogWarningFormat("{0}\n{1}\n{2}\n{3}", errorMessageToDisplayIfListsDoNotMatch, matchingComparison, missingComparison, extraComparison);
             }
         }
 #endif
@@ -528,7 +520,7 @@ private List<INugetPackage> GetUpdatesFallback(
             return updates;
         }
 
-        private void CopyIsManuallyInstalled(List<INugetPackage> newPackages, ICollection<INugetPackage> packagesToUpdate)
+        private static void CopyIsManuallyInstalled(List<INugetPackage> newPackages, ICollection<INugetPackage> packagesToUpdate)
         {
             foreach (var newPackage in newPackages)
             {

src/NuGetForUnity/Editor/PackageSource/NugetPackageSourceV3.cs

@@ -305,6 +305,17 @@ public Task<List<NugetFrameworkGroup>> GetPackageDetailsAsync(
             return ApiClient.GetPackageDetailsAsync(this, package, cancellationToken);
         }
 
+        private static void CopyIsManuallyInstalled(List<INugetPackage> newPackages, ICollection<INugetPackage> packagesToUpdate)
+        {
+            foreach (var newPackage in newPackages)
+            {
+                newPackage.IsManuallyInstalled =
+                    packagesToUpdate.FirstOrDefault(packageToUpdate => packageToUpdate.Id.Equals(newPackage.Id, StringComparison.OrdinalIgnoreCase))
+                        ?.IsManuallyInstalled ??
+                    false;
+            }
+        }
+
         [NotNull]
         private NugetApiClientV3 InitializeApiClient()
         {
@@ -318,16 +329,5 @@ private NugetApiClientV3 InitializeApiClient()
             ApiClientCache.Add(SavedPath, apiClient);
             return apiClient;
         }
-
-        private void CopyIsManuallyInstalled(List<INugetPackage> newPackages, ICollection<INugetPackage> packagesToUpdate)
-        {
-            foreach (var newPackage in newPackages)
-            {
-                newPackage.IsManuallyInstalled =
-                    packagesToUpdate.FirstOrDefault(packageToUpdate => packageToUpdate.Id.Equals(newPackage.Id, StringComparison.OrdinalIgnoreCase))
-                        ?.IsManuallyInstalled ??
-                    false;
-            }
-        }
     }
 }

src/NuGetForUnity/Editor/Ui/NugetPreferences.cs

@@ -44,6 +44,8 @@ public class NugetPreferences : SettingsProvider
 
         private const float LabelPading = 5;
 
+        private static readonly string[] SearchKeywords = { "NuGet", "Packages" };
+
         private readonly GUIContent deleteX = new GUIContent("\u2716");
 
         private readonly GUIContent downArrow = new GUIContent("\u25bc");
@@ -79,8 +81,12 @@ public class NugetPreferences : SettingsProvider
         ///     Initializes a new instance of the <see cref="NugetPreferences" /> class.
         ///     Path of packages.config file is checked here as well in case it was manually changed.
         /// </summary>
+        [SuppressMessage(
+            "Usage",
+            "CA2249:Consider using 'string.Contains' instead of 'string.IndexOf'",
+            Justification = ".Net Framework doesn't have a string contains that is case insensitive.")]
         private NugetPreferences()
-            : base(MenuItemLocation, SettingsScope.Project, new[] { "NuGet", "Packages" })
+            : base(MenuItemLocation, SettingsScope.Project, SearchKeywords)
         {
             shouldShowPackagesConfigPathWarning = ConfigurationManager.NugetConfigFile.InstallLocation == PackageInstallLocation.CustomWithinAssets &&
                                                   !UnityPathHelper.IsPathInAssets(ConfigurationManager.NugetConfigFile.PackagesConfigDirectoryPath);
@@ -129,6 +135,17 @@ public override void OnGUI([CanBeNull] string searchContext)
                 ConfigurationManager.NugetConfigFile.InstallFromCache = installFromCache;
             }
 
+            var keepingPdbFiles = EditorGUILayout.Toggle(
+                new GUIContent(
+                    "Keep PDB Files",
+                    "Do not delete PDB files included in a NuGet package if they can be read by Unity (have the new portable PDB format)."),
+                ConfigurationManager.NugetConfigFile.KeepingPdbFiles);
+            if (keepingPdbFiles != ConfigurationManager.NugetConfigFile.KeepingPdbFiles)
+            {
+                preferencesChangedThisFrame = true;
+                ConfigurationManager.NugetConfigFile.KeepingPdbFiles = keepingPdbFiles;
+            }
+
             var readOnlyPackageFiles = EditorGUILayout.Toggle("Read-Only Package Files", ConfigurationManager.NugetConfigFile.ReadOnlyPackageFiles);
             if (readOnlyPackageFiles != ConfigurationManager.NugetConfigFile.ReadOnlyPackageFiles)
             {