Implement consuming pdb in package reference #4654

heng-liu · 2022-06-05T18:44:42Z

Bug

Regression? Last working version:

Description

1.Implement consuming pdb (spec): add related property into assets file.
2.Add unit tests and functional tests

The perf test results is:

Client Name	Client Version	Solution Name	Restore scenarios	Average total restore time for 80 runs (seconds) Before the change	Average total restore time for 80 runs (seconds) After the change	Time increases(%)
dotnet.exe	7.0.100-preview.4.22252.9	OrchardCore	arctic	109.2015599	109.1890121	-0.01%
dotnet.exe	7.0.100-preview.4.22252.9	OrchardCore	cold	98.74755342	99.11440515	0.37%
dotnet.exe	7.0.100-preview.4.22252.9	OrchardCore	force	25.2766464	25.62013518	1.36%
dotnet.exe	7.0.100-preview.4.22252.9	OrchardCore	noop	15.46378738	15.67183342	1.35%

dotnet.exe	7.0.100-preview.4.22252.9	NuGet	arctic	107.4796735	107.5688019	0.08%
dotnet.exe	7.0.100-preview.4.22252.9	NuGet	cold	92.29263375	93.21019074	0.99%
dotnet.exe	7.0.100-preview.4.22252.9	NuGet	force	20.02546462	20.56217407	2.68%
dotnet.exe	7.0.100-preview.4.22252.9	NuGet	noop	12.49649129	12.56054307	0.51%

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled https://github.com/NuGet/Home/blob/dev/proposed/2021/NuGetConsumePDBInPackageReference.md
- OR
- N/A

jeffkl · 2022-06-06T18:18:37Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

@@ -277,6 +290,45 @@ private IList<ContentItem> FindItemsImplementation(PatternSet definition, IEnume
            return itemsList;
        }

+        private string GetRelatedFileExtensionProperty(string assemblyPath, IEnumerable<Asset> assets)


Is it possible for this method to use file system APIs like Path.GetFileNameWithoutExtension() or Path.ChangeExtension()? The custom logic here worries me and so I'd rather see us call methods that are well tested and proven.

The tricky thing is, there might be extensions like .dll.config, .exe.config (according to the spec, or AllowedReferenceRelatedFileExtensions)
But the Path.GetFileNameWithoutExtension() only minus the last period (.) and all characters following it.

dominoFire

Some comments. Looks good.

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

dominoFire · 2022-06-06T21:54:43Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

+        private string GetRelatedFileExtensionProperty(string assemblyPath, IEnumerable<Asset> assets)
+        {
+            //E.g. if path is "lib/net472/A.B.C.dll", the prefix will be "lib/net472/A.B.C."
+            string assemblyPrefix = assemblyPath.Substring(0, assemblyPath.LastIndexOf('.') + 1);


What if assemblyPath has the value of "a.b.", will this line work?

If assembly path doesn't have any . in it then assemblyPrefix end up string.Empy.

string assemblyPath = "C:\\winexe"; string assemblyPrefix = assemblyPath.Substring(0, assemblyPath.LastIndexOf('.') + 1); Console.WriteLine(assemblyPrefix == string.Empty);

GetRelatedFileExtensionProperty will only be applied when contentItem.Properties.ContainsKey("assembly").
And according to https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Packaging/ContentModel/ManagedCodeConventions.cs#L26-L28, only ".dll", ".winmd", ".exe" will be recognized as assembly property (and .).

jeffkl · 2022-06-07T17:16:08Z

test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/compiler/resources/uwpBlankAppV2.json

+          "ref/netcore50/System.Xml.dll": {
+            "related": ".Linq.dll;.Serialization.dll"
+          },
+          "ref/netcore50/System.dll": {


This seems incorrect, we need to either exclude .dll or have some other logic

Thanks for the great question!
In the spec, we agreed that:
NuGet will check and add all extension, no matter the extension is in AllowedReferenceRelatedFileExtensions or not. So if there is a a.random, NuGet will add random into the related property.
So we'd expect msbuild should be able to filter out what extensions they need.

What's more, if there are A.exe and A.dll, the .dll will be in the related property of the A.exe, the .exe will be in the related property of the A.dll.
If we're going to exclude files ends with .dll(A.B.dll, A.B.C.dll) when checking A.dll,
exclude files ends with .exe(A.B.exe, A.B.C.exe) when checking A.exe,
exclude files ends with .winmd(A.B.winmd, A.B.C.winmd) when checking A.winmd,
we are assuming: there will be no extensions like config.dll, somerandom.exe in the future which will be treated as valid related file extensions. Is that a right assumption?

May I know if you have any suggestions, @nkolev92 @zivkan ? Thanks!

As long as everything that consumes the assets file, doesn't try to copy these related files to the bin folder when defined only in the compile section of the assets file, then I can't think of any potential problems. If these consumers of assets files incorrectly try to copy related files from the "compile" section, rather than only the "runtime" section, of the assets file, then listing these dlls as related will break customers. But really that would be a bug of using the assets file wrong, not how nuget writes the asset file. On the other hand, things that are easy to get wrong are badly designed. It should be easy to do things right.

But if we think about a package where these dlls are under lib/, not ref/ and therefore this would be under the runtime section of the assets file, wouldn't it be confusing for the .NET SDK and NuGet.BuildTools that the same filename is listed both as an asset in its own right, but also as a related file of another asset? I imagine that it will cause duplicate MSBuild items to be created for the file during build, which at best case get de-duplicated or causes double writes, and worst case cause some kind of problem. For example, I don't know if the SDK has options that change copy-to-bin behaviour for Reference items, and the customer experience breaks in some scenarios because the related file listing caused it to be duplicated as a different items type, like Content or something. Even if there isn't really a scenario where it would cause failures, the double write issue would be significant for really huge solutions where build time is already slow. If NuGet or the .NET SDK is causing double writes that binlogs will report, but then customers have no control over fixing, it will give customers a bad experience.

Also, consider the primary goals for this feature:

pdbs get copied to bin folder, so customer debugging experiences are better

xmldoc in packages are available to intellisense, and possibly copied to bin folder so libraries like Swashbuckle can read it at app runtime.

What would the customer scenario of having dlls listed as related files be? Especially when the dll is already listed as its own asset with its own related files.

So, all of that was a long way to say that I agree with Jeff. It doesn't feel right to me. Probably an oversight in the design spec, which means learning opportunity for everyone who reviewed the spec.

Thanks for both of you @jeffkl and @zivkan !
I've changed to exclude all assemblies from related property. That is, ".exe", ".dll" or ".winmd" will be excluded from the related property.

jeffkl · 2022-06-07T17:18:26Z

test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/compiler/resources/uwpBlankAppV2.json

-          "ref/netcore50/System.Xml.dll": {},
-          "ref/netcore50/System.dll": {},
+          "ref/netcore50/System.Xml.dll": {
+            "related": ".Linq.dll;.Serialization.dll"


I think should be a JSON array instead of a semi-colon delimited list

Thanks! This is a good question!
This related property will be added in a LockFileItem and a LockFileItem is as below:

public string Path { get; } public IDictionary<string, string> Properties { get; } = new Dictionary<string, string>();

So if we want to keep the LockFileItem unchanged, the content of related property has to be a string I think.
Or else, it might be a breaking change.
The spec mentioned this at here.

From a technical point of view, LockFileItem could use a JsonConverter to put related into IReadOnlyList<string> Related { get; }, but all other properties into the Properties property. It means that the related property value isn't in the Properties dictionary, and that everything reading the assets file will also need to special case this property. But realistically, consumers are going to need have a special code path just for related anyway, since the way they treat that property is different to how they will treat other properties anyway.

I tested as following. If we change related property value from a string to an array, the old build task will have error in loading the new lockfile.
1.Create a classlib project, install newtonsoft.json package.
2.Changed the project.assets.json file from:

"targets": { "net5.0": { "Newtonsoft.Json/13.0.1": { "type": "package", "compile": { "lib/netstandard2.0/Newtonsoft.Json.dll": {} }, "runtime": { "lib/netstandard2.0/Newtonsoft.Json.dll": {} } } } },

to:

"targets": { "net5.0": { "Newtonsoft.Json/13.0.1": { "type": "package", "compile": { "lib/netstandard2.0/Newtonsoft.Json.dll": { "related": [".pdb", ".xml"] } }, "runtime": { "lib/netstandard2.0/Newtonsoft.Json.dll": { "related": [".pdb", ".xml"] } } } } },

3.Run dotnet build on this test project, failed for:

C:\Program Files\dotnet\sdk\7.0.100-preview.4.22252.9\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(267,5): error NETSDK1060: Error reading assets file: Error loading lock file 'C:\Users\henli\Source\Repos \TestPDB\TestPDB\obj\project.assets.json' : Cannot cast Newtonsoft.Json.Linq.JArray to Newtonsoft.Json.Linq.JToken. [C:\Users\henli\Source\Repos\TestPDB\TestPDB\TestPDB.csproj]

Oops, yes, sorry. I forgot about the problems with mismatched versions. It is indeed very difficult for us to make breaking changes (improvements). 😞

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

ghost · 2022-06-20T19:01:17Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

heng-liu · 2022-06-22T06:20:53Z

Thanks all for reviewing this PR!
I've addressed all current comments. Pls let me know if there is any other questions. Thanks!
//cc @nkolev92

heng-liu · 2022-06-22T17:25:10Z

Hi @dsplaisted and @rainersigwald , just want to check with you about some changes:

The related property will be extensions starts with '.' , which is consistent with Path.GetExtension method. An example is ".pdb;.xml".
The ".exe",".dll", ".winmd" will be excluded in related property. So for a package contains: A.dll, A.exe, A.pdb, A.xml, A.dll.config, A.B.dll, A.B.C.dll, the related property will be ".dll.config;.pdb;.xml"
Pls let me know if you have any different opinions. Thanks!

nkolev92

Haven't looked at the tests yet, but the implementation seems reasonable.

A few comments that should improve performance.

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

nkolev92 · 2022-06-22T20:17:13Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

@@ -43,6 +46,8 @@ public void Load(IEnumerable<string> paths)
                    }
                }
            }
+
+            _assemblyRelatedExtensions = new ConcurrentDictionary<string, string>();


nit: might be helpful to initialize it line 23.

When I was looking at this my instict was why is a dictionary getting initialized in a method, can it be done in the constructor instead?

When digging deeper I found where the list is getting initialized, and that addressed my concern.

In Load method, the input is the paths, which is all asset file paths under one package. Let's take Newtonsoft.Json(11.0.1) as an example, it's a list of following:

Newtonsoft.Json assets:
.signature.p7s LICENSE.md Newtonsoft.Json.nuspec [Content_Types].xml _rels/.rels lib/net20/Newtonsoft.Json.dll lib/net20/Newtonsoft.Json.xml lib/net35/Newtonsoft.Json.dll lib/net35/Newtonsoft.Json.xml lib/net40/Newtonsoft.Json.dll lib/net40/Newtonsoft.Json.xml lib/net45/Newtonsoft.Json.dll lib/net45/Newtonsoft.Json.xml lib/netstandard1.0/Newtonsoft.Json.dll lib/netstandard1.0/Newtonsoft.Json.xml lib/netstandard1.3/Newtonsoft.Json.dll lib/netstandard1.3/Newtonsoft.Json.xml lib/netstandard2.0/Newtonsoft.Json.dll lib/netstandard2.0/Newtonsoft.Json.xml lib/portable-net40%2Bsl5%2Bwin8%2Bwp8%2Bwpa81/Newtonsoft.Json.dll lib/portable-net40%2Bsl5%2Bwin8%2Bwp8%2Bwpa81/Newtonsoft.Json.xml lib/portable-net45%2Bwin8%2Bwp8%2Bwpa81/Newtonsoft.Json.dll lib/portable-net45%2Bwin8%2Bwp8%2Bwpa81/Newtonsoft.Json.xml package/services/metadata/core-properties/096d3ae4d1ce45b083321775ef909fbc.psmdcp

If we need to get all the key-value pair of assembly path and related file extensions, we will need to divide them into several groups so the files under each group will be in the same folder. What's more, we also need to identifying which files are assemblies. If we simply identify as files end with ".exe", ".dll", "winmd", then there will be more more files identified as assemblies than the actual assemblies.
E.g. the NETStandard.Library has 100+ files ending with ".dll", but those are not actual assemblies as they're under build/TFM folder.

NETStandard.Library assets:
.signature.p7s LICENSE.TXT NETStandard.Library.nuspec THIRD-PARTY-NOTICES.TXT [Content_Types].xml _rels/.rels build/NETStandard.Library.targets build/netstandard2.0/NETStandard.Library.targets build/netstandard2.0/ref/Microsoft.Win32.Primitives.dll build/netstandard2.0/ref/System.AppContext.dll build/netstandard2.0/ref/System.Collections.Concurrent.dll build/netstandard2.0/ref/System.Collections.NonGeneric.dll build/netstandard2.0/ref/System.Collections.Specialized.dll build/netstandard2.0/ref/System.Collections.dll build/netstandard2.0/ref/System.ComponentModel.Composition.dll build/netstandard2.0/ref/System.ComponentModel.EventBasedAsync.dll build/netstandard2.0/ref/System.ComponentModel.Primitives.dll build/netstandard2.0/ref/System.ComponentModel.TypeConverter.dll build/netstandard2.0/ref/System.ComponentModel.dll build/netstandard2.0/ref/System.Console.dll build/netstandard2.0/ref/System.Core.dll build/netstandard2.0/ref/System.Data.Common.dll build/netstandard2.0/ref/System.Data.dll build/netstandard2.0/ref/System.Diagnostics.Contracts.dll build/netstandard2.0/ref/System.Diagnostics.Debug.dll build/netstandard2.0/ref/System.Diagnostics.FileVersionInfo.dll build/netstandard2.0/ref/System.Diagnostics.Process.dll build/netstandard2.0/ref/System.Diagnostics.StackTrace.dll build/netstandard2.0/ref/System.Diagnostics.TextWriterTraceListener.dll build/netstandard2.0/ref/System.Diagnostics.Tools.dll build/netstandard2.0/ref/System.Diagnostics.TraceSource.dll build/netstandard2.0/ref/System.Diagnostics.Tracing.dll build/netstandard2.0/ref/System.Drawing.Primitives.dll build/netstandard2.0/ref/System.Drawing.dll build/netstandard2.0/ref/System.Dynamic.Runtime.dll build/netstandard2.0/ref/System.Globalization.Calendars.dll build/netstandard2.0/ref/System.Globalization.Extensions.dll build/netstandard2.0/ref/System.Globalization.dll build/netstandard2.0/ref/System.IO.Compression.FileSystem.dll build/netstandard2.0/ref/System.IO.Compression.ZipFile.dll build/netstandard2.0/ref/System.IO.Compression.dll build/netstandard2.0/ref/System.IO.FileSystem.DriveInfo.dll build/netstandard2.0/ref/System.IO.FileSystem.Primitives.dll build/netstandard2.0/ref/System.IO.FileSystem.Watcher.dll build/netstandard2.0/ref/System.IO.FileSystem.dll build/netstandard2.0/ref/System.IO.IsolatedStorage.dll build/netstandard2.0/ref/System.IO.MemoryMappedFiles.dll build/netstandard2.0/ref/System.IO.Pipes.dll build/netstandard2.0/ref/System.IO.UnmanagedMemoryStream.dll build/netstandard2.0/ref/System.IO.dll build/netstandard2.0/ref/System.Linq.Expressions.dll build/netstandard2.0/ref/System.Linq.Parallel.dll build/netstandard2.0/ref/System.Linq.Queryable.dll build/netstandard2.0/ref/System.Linq.dll build/netstandard2.0/ref/System.Net.Http.dll build/netstandard2.0/ref/System.Net.NameResolution.dll build/netstandard2.0/ref/System.Net.NetworkInformation.dll build/netstandard2.0/ref/System.Net.Ping.dll build/netstandard2.0/ref/System.Net.Primitives.dll build/netstandard2.0/ref/System.Net.Requests.dll build/netstandard2.0/ref/System.Net.Security.dll build/netstandard2.0/ref/System.Net.Sockets.dll build/netstandard2.0/ref/System.Net.WebHeaderCollection.dll build/netstandard2.0/ref/System.Net.WebSockets.Client.dll build/netstandard2.0/ref/System.Net.WebSockets.dll build/netstandard2.0/ref/System.Net.dll build/netstandard2.0/ref/System.Numerics.dll build/netstandard2.0/ref/System.ObjectModel.dll build/netstandard2.0/ref/System.Reflection.Extensions.dll build/netstandard2.0/ref/System.Reflection.Primitives.dll build/netstandard2.0/ref/System.Reflection.dll build/netstandard2.0/ref/System.Resources.Reader.dll build/netstandard2.0/ref/System.Resources.ResourceManager.dll build/netstandard2.0/ref/System.Resources.Writer.dll build/netstandard2.0/ref/System.Runtime.CompilerServices.VisualC.dll build/netstandard2.0/ref/System.Runtime.Extensions.dll build/netstandard2.0/ref/System.Runtime.Handles.dll build/netstandard2.0/ref/System.Runtime.InteropServices.RuntimeInformation.dll build/netstandard2.0/ref/System.Runtime.InteropServices.dll build/netstandard2.0/ref/System.Runtime.Numerics.dll build/netstandard2.0/ref/System.Runtime.Serialization.Formatters.dll build/netstandard2.0/ref/System.Runtime.Serialization.Json.dll build/netstandard2.0/ref/System.Runtime.Serialization.Primitives.dll build/netstandard2.0/ref/System.Runtime.Serialization.Xml.dll build/netstandard2.0/ref/System.Runtime.Serialization.dll build/netstandard2.0/ref/System.Runtime.dll build/netstandard2.0/ref/System.Security.Claims.dll build/netstandard2.0/ref/System.Security.Cryptography.Algorithms.dll build/netstandard2.0/ref/System.Security.Cryptography.Csp.dll build/netstandard2.0/ref/System.Security.Cryptography.Encoding.dll build/netstandard2.0/ref/System.Security.Cryptography.Primitives.dll build/netstandard2.0/ref/System.Security.Cryptography.X509Certificates.dll build/netstandard2.0/ref/System.Security.Principal.dll build/netstandard2.0/ref/System.Security.SecureString.dll build/netstandard2.0/ref/System.ServiceModel.Web.dll build/netstandard2.0/ref/System.Text.Encoding.Extensions.dll build/netstandard2.0/ref/System.Text.Encoding.dll build/netstandard2.0/ref/System.Text.RegularExpressions.dll build/netstandard2.0/ref/System.Threading.Overlapped.dll build/netstandard2.0/ref/System.Threading.Tasks.Parallel.dll build/netstandard2.0/ref/System.Threading.Tasks.dll build/netstandard2.0/ref/System.Threading.Thread.dll build/netstandard2.0/ref/System.Threading.ThreadPool.dll build/netstandard2.0/ref/System.Threading.Timer.dll build/netstandard2.0/ref/System.Threading.dll build/netstandard2.0/ref/System.Transactions.dll build/netstandard2.0/ref/System.ValueTuple.dll build/netstandard2.0/ref/System.Web.dll build/netstandard2.0/ref/System.Windows.dll build/netstandard2.0/ref/System.Xml.Linq.dll build/netstandard2.0/ref/System.Xml.ReaderWriter.dll build/netstandard2.0/ref/System.Xml.Serialization.dll build/netstandard2.0/ref/System.Xml.XDocument.dll build/netstandard2.0/ref/System.Xml.XPath.XDocument.dll build/netstandard2.0/ref/System.Xml.XPath.dll build/netstandard2.0/ref/System.Xml.XmlDocument.dll build/netstandard2.0/ref/System.Xml.XmlSerializer.dll build/netstandard2.0/ref/System.Xml.dll build/netstandard2.0/ref/System.dll build/netstandard2.0/ref/mscorlib.dll build/netstandard2.0/ref/netstandard.dll build/netstandard2.0/ref/netstandard.xml lib/netstandard1.0/_._ package/services/metadata/core-properties/20b93665d4a44e1c8242d85b05b97da3.psmdcp

But if we create the dictionary of assembly path + related files extensions in FindItemsImplementation, the assets files are already grouped (so they're under the same folder). What's more, we've already walked through all the assets files so we've identified which assets are real assemblies (the contentItem has a property of "assembly").

I'm not sure I understand how that's relevant to my question. :D

I was merely saying move line 50 to line 27.
There'd be no funcitonal change there.

Oh sorry, I misunderstand your comments.
Yep, fixed. Thanks!

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

dsplaisted · 2022-06-22T20:55:18Z

Hi @dsplaisted and @rainersigwald , just want to check with you about some changes:

The related property will be extensions starts with '.' , which is consistent with Path.GetExtension method. An example is ".pdb;.xml".

The ".exe",".dll", ".winmd" will be excluded in related property. So for a package contains: A.dll, A.exe, A.pdb, A.xml, A.dll.config, A.B.dll, A.B.C.dll, the related property will be ".dll.config;.pdb;.xml"
Pls let me know if you have any different opinions. Thanks!

This sounds good. Looking forward to this!

nkolev92

Great job.

Just some small comments.
Hoping we can unit test the string operation methods explicitly.

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/LockFileUtils.cs

nkolev92 · 2022-06-23T23:53:10Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

@@ -277,6 +291,51 @@ private IList<ContentItem> FindItemsImplementation(PatternSet definition, IEnume
            return itemsList;
        }

+        private string GetRelatedFileExtensionProperty(string assemblyPath, IEnumerable<Asset> assets)


I think it'd be a good idea to tweak this to make this method unit testable.

Consider making the method internal static.

I get that we have a lot of other tests, but a lot of the substring logic can be tested independently.

Thanks for the suggestion! I changed the method to internal and added a unit test class in .\NuGet.Client\test\NuGet.Core.Tests\NuGet.Packaging.Test\RelatedFileExtensionPropertyTests.cs

nkolev92 · 2022-06-23T23:54:45Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

+            foreach (Asset asset in assets)
+            {
+                if (asset.Path is not null &&
+                    Path.GetExtension(asset.Path) != string.Empty &&


Consider calling Path.GetExtension once to avoid the extra allocations.

Thanks! Fixed.

heng-liu · 2022-06-24T18:29:14Z

Hi @dsplaisted and @rainersigwald , another question, shall we make file name case sensitive?
That is, if we have lib/net50/system.dll and lib/net50/SYSTEM.pdb, shall we add .pdb as the related property of assembly lib/net50/system.dll?

nkolev92 · 2022-06-24T18:36:13Z

Can a package be created with System.dll and SYSTEM.dll? Does NuGet prevalidate this? ZIP has no issues with it, but extraction of such packages on case insensitive FS would be an issue.

dsplaisted · 2022-06-24T18:43:13Z

Hi @dsplaisted and @rainersigwald , another question, shall we make file name case sensitive? That is, if we have lib/net50/system.dll and lib/net50/SYSTEM.pdb, shall we add .pdb as the related property of assembly lib/net50/system.dll?

I would go with being case insensitive, but either way is fine. If a package has mixed cases it's likely to be broken on case-sensitive file systems.

nkolev92

LGTM.
Happy to approve once we've confirmed the sensitivity part is confirmed.

nkolev92 · 2022-06-24T18:59:48Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

@@ -43,6 +46,8 @@ public void Load(IEnumerable<string> paths)
                    }
                }
            }
+
+            _assemblyRelatedExtensions = new ConcurrentDictionary<string, string>();


I'm not sure I understand how that's relevant to my question. :D

I was merely saying move line 50 to line 27.
There'd be no funcitonal change there.

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

nkolev92 · 2022-06-24T19:08:04Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/RelatedFileExtensionPropertyTests.cs

+            // Arrange
+            var collection = new ContentItemCollection();
+            string assembly = "lib/net50/system.test.dll";
+            string[] paths = new string[]


nit: You can probably use a Theory to make all these tests easier to read.

Thanks! Updated to a Theory and also changed the case sensitive test cases.

test/NuGet.Core.Tests/NuGet.Packaging.Test/RelatedFileExtensionPropertyTests.cs

heng-liu · 2022-06-28T17:41:25Z

Hi @dsplaisted and @rainersigwald, for the case sensitive question, I'd like to go with case sensitive instead of case insensitive.
So if there is a lib/net5.0/A.B.C.dll and lib/net5.0/A.b.c.pdb, the .pdb will not be added to the related property.
As on Linux (and other OSes which has case sensitive path), it's not able to get the exact related file full name.
We could make it case insensitive on Windows and case sensitive on other OSes. But considering there are 61% .NET users are on Linux , and it might be misleading/confusing if it happens to work on Windows, I'd like to go with case sensitive on all OSes option.
Pls let me know if you have different thoughts, thanks!
//cc @nkolev92

nkolev92

Great job @heng-liu!

rainersigwald · 2022-06-28T19:08:39Z

I think case sensitive is fine but we should try to listen to see if it's causing problems in the real world. As Daniel mentioned, it seems likely that any such package is already broken on Linux . . .

heng-liu · 2022-06-28T21:20:55Z

I think case sensitive is fine but we should try to listen to see if it's causing problems in the real world. As Daniel mentioned, it seems likely that any such package is already broken on Linux . . .

Thanks @rainersigwald and @dsplaisted for your suggestion! Yes, we're checking packages with those issues (unmatched prefix if comparing case sensitively). The rough number is 0.03% of all packages(id + version) on nuget.org. (Thanks for @dominoFire 's and @joelverhagen 's help on this!)
We'll merge the PR first and look into the package list for more details later.

This reverts commit 60b3c34.

heng-liu · 2022-06-30T16:59:39Z

Hi @rainersigwald and @dsplaisted , here are some insights about the packages on nuget.org:

There are 46 packages (at least one version) having mismatched prefix between .pdb and .dll. Some have fixed the mismatched in their latest version. Only 16 packages still have mismatched prefix between .pdb and .dll in their latest version.

There are about 630 packages (at least one version) having mismatched prefix between .xml and .dll. Some have fixed the mismatched in their latest version. So there are about 380 packages still having mismatched prefix between .xml and .dll in their latest version.

So seems there won't be many problematic packages when consuming pdb.
//cc @nkolev92

heng-liu requested a review from a team as a code owner June 5, 2022 18:44

jeffkl reviewed Jun 6, 2022

View reviewed changes

dominoFire reviewed Jun 6, 2022

View reviewed changes

jeffkl reviewed Jun 7, 2022

View reviewed changes

erdembayar mentioned this pull request Jun 7, 2022

Draft of Consuming pdb in package reference #4637

Closed

8 tasks

erdembayar reviewed Jun 7, 2022

View reviewed changes

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs Show resolved Hide resolved

erdembayar reviewed Jun 8, 2022

View reviewed changes

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs Outdated Show resolved Hide resolved

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jun 20, 2022

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jun 21, 2022

heng-liu added 5 commits June 21, 2022 15:59

work

692dc87

cache relatedproperty

f7800f6

use ConcurrentDictionary

5b3fe24

address comments

ecbb9fc

remove assembly from related property

9966f8c

heng-liu force-pushed the dev-hengliu-consumPDB-test4 branch from cb8829f to 9966f8c Compare June 21, 2022 22:59

heng-liu requested review from dominoFire, zivkan, erdembayar and jeffkl June 22, 2022 06:19

nkolev92 reviewed Jun 22, 2022

View reviewed changes

address comments to add null into dictionary

b8f5a7b

heng-liu requested a review from nkolev92 June 23, 2022 18:18

nkolev92 reviewed Jun 24, 2022

View reviewed changes

address comments

2b89d6d

heng-liu requested a review from nkolev92 June 24, 2022 18:18

nkolev92 reviewed Jun 24, 2022

View reviewed changes

heng-liu added 2 commits June 24, 2022 12:41

address comments

7d4da1f

do not ignore case when comparing prefix

23e098d

heng-liu requested a review from nkolev92 June 28, 2022 18:01

jeffkl approved these changes Jun 28, 2022

View reviewed changes

nkolev92 approved these changes Jun 28, 2022

View reviewed changes

heng-liu merged commit 60b3c34 into dev Jun 28, 2022

heng-liu deleted the dev-hengliu-consumPDB-test4 branch June 28, 2022 21:22

pragnya17 added a commit that referenced this pull request Jun 29, 2022

Revert "Implement consuming pdb in package reference (#4654)"

09a5a20

This reverts commit 60b3c34.

kartheekp-ms added a commit that referenced this pull request Jun 29, 2022

Revert "Implement consuming pdb in package reference (#4654)"

0c95933

This reverts commit 60b3c34.

kartheekp-ms added a commit that referenced this pull request Jun 29, 2022

Revert "Implement consuming pdb in package reference (#4654)"

344f8a8

This reverts commit 60b3c34.

heng-liu mentioned this pull request Jun 29, 2022

Consume pdbs from packages in PackageReference - add Debug Symbols to LockFileTargetLibrary NuGet/Home#5926

Closed

3 tasks

heng-liu mentioned this pull request Jan 10, 2023

design spec for version property in project reference NuGet/Home#12348

Draft

Implement consuming pdb in package reference #4654

Implement consuming pdb in package reference #4654

Conversation

heng-liu commented Jun 5, 2022 • edited Loading

Bug

Description

PR Checklist

Choose a reason for hiding this comment

heng-liu Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

dominoFire left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heng-liu Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 20, 2022

heng-liu commented Jun 22, 2022

heng-liu commented Jun 22, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heng-liu Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted commented Jun 22, 2022

nkolev92 left a 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

heng-liu commented Jun 24, 2022

nkolev92 commented Jun 24, 2022

dsplaisted commented Jun 24, 2022

nkolev92 left a 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

heng-liu commented Jun 28, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

rainersigwald commented Jun 28, 2022

heng-liu commented Jun 28, 2022

heng-liu commented Jun 30, 2022 • edited Loading

heng-liu commented Jun 5, 2022 •

edited

Loading

heng-liu Jun 6, 2022 •

edited

Loading

heng-liu Jun 21, 2022 •

edited

Loading

heng-liu Jun 23, 2022 •

edited

Loading

heng-liu commented Jun 30, 2022 •

edited

Loading