Add support for embedding C# source in portable pdb #12353

nguerrera · 2016-07-06T02:48:43Z

Implement #12625 for C# and portable PDBs. Native PDB and VB implementation to follow as separate change once this is approved.

Also fix #12814

tmat · 2016-07-07T20:41:07Z

@tmat

nguerrera · 2016-07-07T20:41:07Z

Reviewers: consider this PR a design proposal at this stage. I am scheduling a design review. All feedback is welcome, but you may want to hold off on reviewing the code details until that has taken place and been approved.

nguerrera · 2016-07-08T15:40:05Z

Re-tagging as PR for personal review only. I will open a separate issue for the detailed proposal for design review and will be making updates to the proposal based on offline feedback.

tmat · 2016-07-18T22:50:09Z

src/Compilers/Core/Portable/PEWriter/DebugSourceInfo.cs

+        /// <summary>
+        /// The source text to embed in the PDB. (If any, otherwise null.)
+        /// </summary>
+        public readonly SourceText EmbeddedText;


I'd name this EmbeddedTextOpt to emphasize it might be null.

nguerrera · 2016-07-19T23:19:32Z

I've moved all design notes to #12625 and updated them.

tmat · 2016-07-29T17:20:17Z

src/Compilers/Core/MSBuildTask/Shared/ManagedCompiler.cs

+        private void AddEmbeddedFilesToCommandLine(CommandLineBuilderExtension commandLine)
+        {
+            // If there were no additional files passed in, don't add any /additionalfile: switches
+            // on the command-line.


Not sure what this comment is trying to convey. Perhaps it's stale?

nguerrera · 2016-08-09T20:47:43Z

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

@@ -143,18 +153,15 @@ internal SourceText ReadFileContent(CommandLineSourceFile file, IList<Diagnostic
        /// <param name="diagnostics">Storage for diagnostics.</param>
        /// <param name="normalizedFilePath">If given <paramref name="file"/> opens successfully, set to normalized absolute path of the file, null otherwise.</param>
        /// <returns>File content or null on failure.</returns>
-        internal SourceText ReadFileContent(CommandLineSourceFile file, IList<DiagnosticInfo> diagnostics, out string normalizedFilePath)
+        internal SourceText TryReadFileContent(CommandLineSourceFile file, IList<DiagnosticInfo> diagnostics, out string normalizedFilePath)


This was for consistency re: feeback to make analogous TryReadEmbeddedFileContent express it can fail and return null. But I neglected the overload without the normalizedFilePath. I will rename that too.

gafter · 2016-08-09T21:26:26Z

I have a couple of general questions that I was too lazy to search for answers myself.

If we embed source files, I hope we read the source files only once from the disk. Correct?
Do we preserve the bytes of embedded files intact? Or do we perchance re-encode in, say, UTF-8 to help downstream tools?

nguerrera · 2016-08-09T21:41:42Z

Correct. (*)
We preserve the original bytes, which was a lot of implementation effort but deemed necessary. See Proposal: Embed sources in PDBs #12625 (comment) and follow up. Re-encoding is lossy in the general case.

(*) Caveat: if you spell source and /embed files differently on command line that happen to be the same physical file (e.g. casing difference, symbolic link, '.\foo' vs 'foo' etc. etc.), they will be processed as distinct files. We discussed that in design review quite a bit. I came to the review with a design that did not have this problem, but it had separate mechanisms for embedding-and-compiling a source file (such that path need not be repeated) vs. embedding an additional file. The room felt that we should have just one switch and one IEnumerable emit arg and it was worth the tradeoff of reading twice for the multiple spelling case.

gafter · 2016-08-10T23:05:50Z

LGTM

agocke · 2016-08-11T17:39:18Z

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

@@ -130,10 +140,10 @@ internal virtual MetadataReferenceResolver GetCommandLineMetadataReferenceResolv
        /// <param name="file">Source file information.</param>
        /// <param name="diagnostics">Storage for diagnostics.</param>
        /// <returns>File content or null on failure.</returns>
-        internal SourceText ReadFileContent(CommandLineSourceFile file, IList<DiagnosticInfo> diagnostics)
+        internal SourceText TryReadFileContent(CommandLineSourceFile file, IList<DiagnosticInfo> diagnostics)


TryReadFileContent [](start = 28, length = 18)

We sometimes use the Opt suffix for methods that may return null instead of using the Try prefix (which is more common with the bool/out pattern). I don't have strong feelings about this, though.

Since there's a precedent for this pattern and it was @tmat's suggestion from his earlier review, I'm going to stick with Try

Conflicts: src/Workspaces/Core/Portable/WorkspacesResources.resx

nguerrera · 2016-08-11T21:57:20Z

src/Compilers/Core/Portable/InternalUtilities/SpecializedCollections.Empty.Set.cs

@@ -10,8 +10,11 @@ internal partial class SpecializedCollections
        private partial class Empty
        {
            internal class Set<T> : Collection<T>, ISet<T>
+#if COMPILERCORE


These ifdefs are ugly but most expedient way to make progress against the feedback. I can fix it up by tweaking closed project to include IReadOnlySet.cs after this is merged.

Which feedback? In general we should avoid all #ifdef in the compiler (and Roslyn in general). They contribute negatively to the maintenance and experience of the code base.

#12353 (diff)

I realize now that I don't need to wait for this to be merged to fix the issue. I can submit a PR ASAP to closed/ to pull in IReadOnlySet.cs, then remove these. Will do!

I fully agree about ifdefs!

agocke · 2016-08-11T22:19:14Z

LGTM

rychlym · 2019-03-11T04:19:27Z

Hi,
could be there added a feature, that in case the all sources are embedded inside of pdb, then Url is shrink to not show the same path at the beginning?
e.g.
lets' have sources inside of following path:
c:\users\my_private_account\some_other_private_ifnfo\project\src\subdir1\file1.cs
c:\users\my_private_account\some_other_private_ifnfo\project\src\subdir1\file2.cs
...
c:\users\my_private_account\some_other_private_ifnfo\project\src\subdir_n\file_m.cs

so finally it e.g. found, the all source path starts with e.g. c:\users\my_private_account\some_other_private_ifnfo\project\src
and that is to be cut off.
so the final Url (it is actually rather meant as the name in this case) of that files embed in the pdb are like:
subdir1\file1.cs
subdir1\file2.cs
...
subdir_n\file_m.cs

I can imagine, that the feature can be introduced by a new MSBuild property EmbedAllSourcesShrinkPath, or even as an default behavior in this case the EmbedAllSources property is
set to true.

I should note it is not a critical thing - as a kind of workaround I can use subst of the path, so I can get rid off some parts of path I would like to not expose.

tmat · 2019-03-11T16:55:20Z

We actually have such feature.

Add SourceRoot mapping to your project file like so:

  <ItemGroup>
    <SourceRoot Include="$(MSBuildProjectDirectory)\" MappedPath="/_/"/>
  </ItemGroup>

and compile with /p:Deterministic=true (only needed for projects that do not import Microsoft.Net.Sdk) and /p:ContinuousIntegrationBuild=true.

Note: you should only build with /p:ContinuousIntegrationBuild=true on CI server. If you trim the paths for your dev build, the debugger won't be able to find the source files when you F5 in VS.

I'd also recommend checking out Source Link. It does the SourceRoot mapping (and more) for you.

rychlym · 2019-03-15T20:49:45Z

@tmat
Thanks a lot - that your comment saved my time! It is also a good design, that the SourceRoot is in the ItemGroup, because I also needed to add $(LocalAppData), because of .NET Standard AssemblyAttibutes.cs file located there. (To be honest, I am not sure, whether the $(LocalAppData) works for different platforms (linux, ios) - there might be an other, more accurate property, but that seems to be a different topic...)
To your note to the debugging: I am not on the CI server yet (I am using just a local NuGet repository and build manually)
I understood from answers of a different discussion, that a NuGet package can be nicely debugged from a private repo (I mean on a different machine accessing that repo and not accessing the source code) if the package contains the pdb with embeded source code. It seems to need following only properties to be set
like:

<PublishRepositoryUrl>false</PublishRepositoryUrl>    
<DebugType>portable</DebugType>
<EmbedAllSources>true</EmbedAllSources>
<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder>

and it is pretty working for me for now (some of them might not bee needed but i haven't time to check properly one by one)
(I encountered some troubles in case of package dependent on .NET Framework 3.5, which seems not to copy the pdb from the package into bin/debug folder of the consumer app, but that is also another topic).

tmat · 2019-03-15T20:56:27Z

Glad it works for you.

because of .NET Standard AssemblyAttibutes.cs file located there.

Issue and a workaround is tracked by dotnet/msbuild#1479

on .NET Framework 3.5, which seems not to copy the pdb from the package into bin/debug folder of the consumer app

Tracked here: dotnet/sdk#1458

nguerrera added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 6, 2016

dnfclas added the cla-already-signed label Jul 6, 2016

nguerrera force-pushed the embed-source-in-pdb branch 3 times, most recently from f88dee0 to c044ec8 Compare July 7, 2016 17:20

nguerrera changed the title ~~[WIP] Embed source in pdb~~ Embed source in pdb Jul 7, 2016

nguerrera removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 7, 2016

nguerrera mentioned this pull request Jul 7, 2016

Support reading embedded source dotnet/symreader-portable#16

Merged

nguerrera changed the title ~~Embed source in pdb~~ Proposal: Embed source in pdb Jul 7, 2016

mjsabby mentioned this pull request Jul 8, 2016

Proposal: Add compiler switch to embed PDB inside the assembly #12390

Closed

nguerrera added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jul 8, 2016

nguerrera changed the title ~~Proposal: Embed source in pdb~~ [WIP] Embed source in pdb Jul 8, 2016

nguerrera changed the title ~~[WIP] Embed source in pdb~~ Embed source in pdb (WIP) Jul 8, 2016

tmat reviewed Jul 18, 2016
View reviewed changes

nguerrera force-pushed the embed-source-in-pdb branch 2 times, most recently from dde8ac2 to 750d537 Compare July 19, 2016 00:22

nguerrera mentioned this pull request Jul 19, 2016

Proposal: Embed sources in PDBs #12625

Closed

3 tasks

gafter added the Area-Compilers label Jul 22, 2016

nguerrera force-pushed the embed-source-in-pdb branch from c8c374b to 8a5619f Compare July 28, 2016 22:23

tmat reviewed Jul 29, 2016
View reviewed changes

nguerrera reviewed Aug 9, 2016
View reviewed changes

gafter added the Feature Request label Aug 9, 2016

nguerrera added the Interactive-Debugging label Aug 9, 2016

This was referenced Aug 10, 2016

EncodedStringText.Decode does not handle memory stream with non-zero offset correctly #12348

Closed

Gargantuan source files can be silently treated as being much smaller #12814

Closed

Add support for embedding C# source in portable pdb

5264cfb

nguerrera force-pushed the embed-source-in-pdb branch from 21fbf99 to 5264cfb Compare August 10, 2016 22:06

agocke reviewed Aug 11, 2016
View reviewed changes

nguerrera added 2 commits August 11, 2016 13:29

Use specialized collections and avoid query syntax

fff3927

Merge branch 'master' into embed-source-in-pdb

c5d42df

Conflicts: src/Workspaces/Core/Portable/WorkspacesResources.resx

nguerrera force-pushed the embed-source-in-pdb branch from 4148cb1 to c5d42df Compare August 11, 2016 20:36

Fix build break in closed project sharing source file

fe80ed2

nguerrera reviewed Aug 11, 2016
View reviewed changes

gafter merged commit 17008be into dotnet:master Aug 11, 2016

gafter added this to the 2.0 (Preview 5) milestone Aug 11, 2016

nguerrera deleted the embed-source-in-pdb branch August 11, 2016 23:22

This was referenced Mar 4, 2017

Include PDB file in the NuGet package. npgsql/npgsql#1477

Closed

Use SourceLink npgsql/npgsql#1479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for embedding C# source in portable pdb #12353

Add support for embedding C# source in portable pdb #12353

nguerrera commented Jul 6, 2016 •

edited

Loading

tmat commented Jul 7, 2016

nguerrera commented Jul 7, 2016

nguerrera commented Jul 8, 2016

tmat Jul 18, 2016

nguerrera commented Jul 19, 2016

tmat Jul 29, 2016

nguerrera Aug 9, 2016

nguerrera Aug 10, 2016

gafter commented Aug 9, 2016

nguerrera commented Aug 9, 2016 •

edited

Loading

gafter commented Aug 10, 2016

agocke Aug 11, 2016

nguerrera Aug 11, 2016 •

edited

Loading

nguerrera Aug 11, 2016

jaredpar Aug 11, 2016

nguerrera Aug 11, 2016

nguerrera Aug 11, 2016

agocke commented Aug 11, 2016

rychlym commented Mar 11, 2019 •

edited

Loading

tmat commented Mar 11, 2019

rychlym commented Mar 15, 2019

tmat commented Mar 15, 2019

Add support for embedding C# source in portable pdb #12353

Add support for embedding C# source in portable pdb #12353

Conversation

nguerrera commented Jul 6, 2016 • edited Loading

tmat commented Jul 7, 2016

nguerrera commented Jul 7, 2016

nguerrera commented Jul 8, 2016

Choose a reason for hiding this comment

nguerrera commented Jul 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Aug 9, 2016

nguerrera commented Aug 9, 2016 • edited Loading

gafter commented Aug 10, 2016

Choose a reason for hiding this comment

nguerrera Aug 11, 2016 • edited Loading

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

agocke commented Aug 11, 2016

rychlym commented Mar 11, 2019 • edited Loading

tmat commented Mar 11, 2019

rychlym commented Mar 15, 2019

tmat commented Mar 15, 2019

nguerrera commented Jul 6, 2016 •

edited

Loading

nguerrera commented Aug 9, 2016 •

edited

Loading

nguerrera Aug 11, 2016 •

edited

Loading

rychlym commented Mar 11, 2019 •

edited

Loading